Security hole in XPCSafeJSObjectWrapper

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: jst)

Tracking

({regression})

Trunk
regression
Points:
---
Bug Flags:
wanted1.8.1.x ?
wanted1.8.0.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] required by SJsOW)

Attachments

(15 attachments)

539 bytes, text/html
Details
393 bytes, text/html
Details
1.74 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
403 bytes, text/html
Details
704 bytes, text/html
Details
661 bytes, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
775 bytes, text/html
Details
5.51 KB, patch
Details | Diff | Splinter Review
6.26 KB, patch
Details | Diff | Splinter Review
6.29 KB, patch
Details | Diff | Splinter Review
12.53 KB, patch
Details | Diff | Splinter Review
12.42 KB, patch
jst
: review+
Details | Diff | Splinter Review
202 bytes, text/html
Details
721 bytes, patch
jst
: review+
Details | Diff | Splinter Review
2.85 KB, patch
brendan
: review+
brendan
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
I've tested on the fx-win32-tbox-trunk 2007-01-31-22 hourly build.

The issue described in bug 355766 comment #7 is still existing.

And, the hole can be used not only to perform XSS attacks, but also to run
arbitrary code with chrome privileges.

  <marquee id="m">m</marquee>
  var sw1 = new XPCSafeJSObjectWrapper(window);
  var sw2 = new XPCSafeJSObjectWrapper(m.init.eval);
  window.x = Components.lookupMethod(location, "href");
  sw1.x.call(sw2, "javascript:alert(Components.stack);");
(Reporter)

Comment 1

11 years ago
Created attachment 253578 [details]
testcase 1 - XSS
(Reporter)

Comment 2

11 years ago
Created attachment 253579 [details]
testcase 2 - arbitrary code execution
(Assignee)

Comment 3

11 years ago
Created attachment 253702 [details] [diff] [review]
Fix for both issues.

This blocks an XPCSafeJSObjectWrapper from ever directly wrapping a function (eval() or any other function), and adds a security check to WrapFunction() to make sure the caller can access the function being wrapped.
Attachment #253702 - Flags: superreview?(brendan)
Attachment #253702 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Comment on attachment 253702 [details] [diff] [review]
Fix for both issues.

r+sr=me, who should have noticed the !enabled case not throwing last time!

/be
Attachment #253702 - Flags: superreview?(brendan)
Attachment #253702 - Flags: superreview+
Attachment #253702 - Flags: review?(brendan)
Attachment #253702 - Flags: review+
(Assignee)

Comment 5

11 years ago
Thanks for testing this moz_bug_r_a4, can you please re-test and keep beating at this? :)

Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 6

11 years ago
The both fixes can be circumvented.

Content can get a privileged object and create an XPCSafeJSObjectWrapper with
it.

  var sw2 = new XPCSafeJSObjectWrapper(m.init.eval.prototype);

location.href setter that the caller can access can be used to perform XSS
attack.

  <iframe src="victim site"/>
  <iframe/>
  var sw1 = new XPCSafeJSObjectWrapper(window);
  var sw2 = new XPCSafeJSObjectWrapper(frames[0]);
  window.x = Components.lookupMethod(frames[1].location, "href");
  sw1.x.call(sw2, "data:text/html,<script>...</script>");
(Reporter)

Comment 7

11 years ago
Created attachment 253729 [details]
testcase 3 - arbitrary code execution
(Reporter)

Comment 8

11 years ago
Created attachment 253730 [details]
testcase 4 - XSS
(Reporter)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

11 years ago
Created attachment 253816 [details] [diff] [review]
Fix for both new issues.

This fixes the latest problems by making sure at XPCSafeJSObjectWrapper creation time that the caller has access to the object he's trying to create a wrapper for.
Assignee: nobody → jst
Status: REOPENED → ASSIGNED
Attachment #253816 - Flags: superreview?(brendan)
Attachment #253816 - Flags: review?(brendan)
Comment on attachment 253816 [details] [diff] [review]
Fix for both new issues.

This is proof of what we already knew (some of the time, for some value of "we"): XBL is unsound because it mixes chrome and content principals in the same window. XBL (and XPConnect) clone function objects across trust domains, which is necessary and sufficient to carry a different parent link to a distinct principal-owning global object.

But a cloned function object's __proto__ is the clone-parent, which is a chrome function object for XBL loaded from chrome://....

We need a bug on file for that problem -- is there one already?

/be
Attachment #253816 - Flags: superreview?(brendan)
Attachment #253816 - Flags: superreview+
Attachment #253816 - Flags: review?(brendan)
Attachment #253816 - Flags: review+
(Assignee)

Comment 11

11 years ago
Fix checked in (with s/unsafeObj/objToWrap/ fixed, which crept in through some last-minute cut-n-pasting before attaching the diff).

moz_bug_r_a4, please give this a spin and reopen this bug if you find more problems. Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

11 years ago
There is a way to circumvent the last fix to perform an XSS attack.

1. Load about:blank into a subframe.
2. Create a safe wrapper for the subframe.
3. Load a victim site into the subframe.
(Reporter)

Comment 13

11 years ago
Created attachment 254011 [details]
testcase 5 - XSS
(Reporter)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Would it help if XPC_SJOW_FunctionWrapper did a CanCallerAccess check on unsafeObj?  Seems to me like it should....
(Reporter)

Comment 17

11 years ago
By the way, 'return' seems to be needed at line 560.

556   NS_NAMED_LITERAL_CSTRING(funScript,
557                            "var args = [];"
558                            "for (var i = 1; i < arguments.length; i++)"
559                            "args.push(arguments[i]);"
560                            "arguments[0].apply(this, args);");
Yikes, sorry I missed that missing return (or didn't make JS an expression language).

Why isn't the wrapping deep?  Each get (.x, then .y from the value of .x) should be wrapped.

/be
Hmm...  So would it work to check whether the unsafe object we got it from can access the thing we're wrapping when we go to wrap it?
(Assignee)

Comment 21

11 years ago
Something like that could be good to do, yes, and in addition I think it's worth looking into some sort of principal propagation from wrapper to access to properties on that wrapper.

What's happening in the last case in this bug is that chrome calls unsafe code through a wrapper and gets back a wrapper for a chrome object, then calls a method on that wrapper which will make the wrapper code compile a scripted function with chrome principals to call through, which is obviously not what we want. So I'm thinking of making wrappers be able to hold on to the principals of the code that created them, and that way never change to principals that the original principal can't access etc. I haven't sat down to figure out the details here yet, but those are the lines that I'm thinking along here for now.
(Assignee)

Comment 22

11 years ago
Created attachment 254451 [details] [diff] [review]
Fix for new issues since last patch.

This does what I talked about in my previous comment. I.e. if we call through a wrapper and get back an object that's not accessable by that wrapper (i.e. chrome calling unsafe code that returns a chrome object), use the principals of the unsafe object where the new object came from when compiling sciripted functions for the new wrapper.

This also adds the missing return and the security check in XPC_SJOW_FunctionWrapper().
Attachment #254451 - Flags: superreview?(brendan)
Attachment #254451 - Flags: review?(bzbarsky)
Comment on attachment 254451 [details] [diff] [review]
Fix for new issues since last patch.

>+      // Check to see if the new object we just wrapped is accessable

Nit: accessible misspelled.

>+      // from the unsafe object we got the new object through. If not,
>+      // force the new wrapper to use the principal of the unsafe
>+      // object we got the new object from.
>+      nsCOMPtr<nsIPrincipal> newObjPrincipal;

Nit: valObjPrincipal might be a better name, or childObjPrincipal -- even better if you name JSVAL_TO_OBJECT(val) accordingly.

>@@ -794,6 +866,15 @@ XPC_SJOW_NewResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags,
> JS_STATIC_DLL_CALLBACK(void)
> XPC_SJOW_Finalize(JSContext *cx, JSObject *obj)
> {
>+  // Release the reference to the cached principal if we have one.
>+  jsval v;
>+  ::JS_GetReservedSlot(cx, obj, XPC_SJOW_SLOT_PRINCIPAL, &v);

Check JS_GetReservedSlot failure, or initialize v = JSVAL_VOID.

>+
>+  if (!JSVAL_IS_VOID(v)) {
>+    nsIPrincipal *principal = (nsIPrincipal *)JSVAL_TO_PRIVATE(v);
>+
>+    NS_IF_RELEASE(principal);
>+  }
> }
> 
> JS_STATIC_DLL_CALLBACK(JSBool)

sr=me with that and bz's r+.

/be
Comment on attachment 254451 [details] [diff] [review]
Fix for new issues since last patch.

>diff --git a/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp 

>@@ -316,6 +321,48 @@ WrapJSValue(JSContext *cx, JSObject *obj, jsval val, jsval *rval)
>+      // Check to see if the new object we just wrapped is accessable

"accessible"

I think we need to root safeObj here, since we're going to be calling into random code (nsIScriptObjectPrincipal implementations, etc).  The simplest way is probably to set *rval before checking principals, and rely on the false returns.  Or use one of XPConnect's auto-markers, I guess.

>+      nsresult rv = FindPrincipals(cx, obj, getter_AddRefs(newObjPrincipal),
>+                                   nsnull, nsnull);
...
>+      rv = FindPrincipals(cx, JSVAL_TO_OBJECT(val),
>+                          getter_AddRefs(unsafeObjPrincipal), nsnull, nsnull);

|obj| is the unsafe object, not the new object.  |val| is the new object.  Sadly, for subsumes() this matters.

>@@ -413,7 +460,26 @@ GetScriptedFunction(JSContext *cx, JSObject *obj, JSObject *unsafeObj,
>+    // Check to see if we have a cached principal or not.

"whether we have a cached principal".

I'm not sure why there are null-checks of the principal in GetScriptedFunction() and XPC_SJOW_Finalize().  We can't get a null principal plus success back from FindPrincipals(), so it seems to me like the principal pointer should be non-null if the private val was not void.

r=bzbarsky with those issues addressed.
Attachment #254451 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 25

11 years ago
(In reply to comment #24)
> >+      nsresult rv = FindPrincipals(cx, obj, getter_AddRefs(newObjPrincipal),
> >+                                   nsnull, nsnull);
> ...
> >+      rv = FindPrincipals(cx, JSVAL_TO_OBJECT(val),
> >+                          getter_AddRefs(unsafeObjPrincipal), nsnull, nsnull);
> 
> |obj| is the unsafe object, not the new object.  |val| is the new object. 
> Sadly, for subsumes() this matters.

Duh. The code does do the right thing, but yeah, the naming is messed up. I was juggling those back n' fourth trying to find names that made sense but obviously got lost in the process.

Fixed in the upcoming diff.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 26

11 years ago
Created attachment 254455 [details] [diff] [review]
Fix issues in previous patch.

This fixes the issues that bz found, and initializes a JSPrincipall pointer to nsnull, just in case.
Attachment #254455 - Flags: superreview?(brendan)
(Assignee)

Updated

11 years ago
Attachment #254451 - Flags: superreview?(brendan)
No need to do the *rval setting at the end of the function now.
(Assignee)

Comment 28

11 years ago
Fixed locally.
(Assignee)

Comment 29

11 years ago
Comment on attachment 254455 [details] [diff] [review]
Fix issues in previous patch.

Clearing sr request as brendan already sr'ed above. I completely missed that comment :(

New patch with all above issues addressed coming up (and being checked in).
Attachment #254455 - Flags: superreview?(brendan)
(Assignee)

Comment 30

11 years ago
Created attachment 254467 [details] [diff] [review]
Updated fix that fixes the last two testcases.
(Assignee)

Comment 31

11 years ago
Fix landed. moz_bug_r_a4, please beat hard on this if you can, and reopen if you're still able to get around this!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 34

11 years ago
Created attachment 255417 [details] [diff] [review]
Make functions be XPCSafeJSObjectWrappers as well.

This fixes the last issue by making functions no longer be special in that they themselves now also get wrapped by XPCSafeJSObjectWrapper. This way we avoid ever using an unsafe Function.prototype. To do this I ended up having to add a call() hook to XPCSafeJSObjectWrapper, which means that it's now usable with other callable objects as well (of which there are very few in Mozilla), and it also means that w.foo instanceOf Function won't ever be true, whereas before it was true. I think that's a tradeoff that we can live with though.

Brendan, please have a look at this and see what you think. Especially look around the two XXX comments in the diff.
Attachment #255417 - Flags: superreview?(brendan)
Attachment #255417 - Flags: review?(brendan)
(Assignee)

Comment 35

11 years ago
Created attachment 256106 [details] [diff] [review]
Same as above w/o XXX comments.

I got verbal r+sr=brendan for this, I'm checking this in now.
Attachment #256106 - Flags: superreview+
Attachment #256106 - Flags: review+
(Assignee)

Comment 36

11 years ago
Fix landed. moz_bug_r_a4, please test with tomorrows nightly or later again. Anything relating to eval and function wrapping is worth beating on again with this update.
(Assignee)

Comment 37

11 years ago
And marking this fixed. Please reopen if more issues surface from the last change.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 38

11 years ago
549 XPC_SJOW_CallWrapper(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
550                      jsval *rval)
551 {
552   return ::JS_CallFunctionValue(cx, obj, argv[0], argc - 1, argv + 1, rval);
553 }

A script can get XPC_SJOW_CallWrapper and call it with no argument.  That
causes crash.

  window.f = function() {
    arguments.callee.caller();
  };
  new XPCSafeJSObjectWrapper(window).f();

or

  Function.prototype.apply = function() {
    this();
  };
  new XPCSafeJSObjectWrapper(window).alert();
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 39

11 years ago
Created attachment 256544 [details]
testcase 8 - crash
(Assignee)

Comment 40

11 years ago
Created attachment 256547 [details] [diff] [review]
Fix for the crash.

This adds a trivial check to make sure that at least one argument is passed to XPC_SJOW_CallWrapper(), if not, we throw.

shaver says r+sr=shaver
Attachment #256547 - Flags: superreview+
Attachment #256547 - Flags: review+
(Assignee)

Comment 41

11 years ago
Fix landed on the trunk.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Comment on attachment 255417 [details] [diff] [review]
Make functions be XPCSafeJSObjectWrappers as well.

I'm not marking this patch obsolete, but if it should be, jst can you do it?

/be
Attachment #255417 - Flags: superreview?(brendan)
Attachment #255417 - Flags: review?(brendan)
(Assignee)

Comment 43

11 years ago
That patch already landed, with verbal r+sr=brendan :)
(Reporter)

Comment 44

11 years ago
"Make indirect safe wrapper function calls work" patch caused an issue.

+  } else {
+    // A function wrapped in an XPCSafeJSObjectWrapper is being called
+    // indirectly off of an object that's not a safe wrapper
+    // (i.e. foo.bar = safeObj.fun; foo.bar()), set obj to be the safe
+    // wrapper for the function, and use the object passed in as the
+    // unsafe object.
+    unsafeObj = obj;

It seems that "use the object passed in as the unsafe object" stuff breaks the
safe wrapper's security model.

If |foo| is an object created in chrome, a scripted function is compiled with
the system principal.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 46

11 years ago
Created attachment 259153 [details] [diff] [review]
Fix for latest exploit.

This blocks the latest exploit. The problem was that it was possible to trick the function calling code to use the principals off of an unsafe object from the callers origin even when calling into a safe wrapper from a different origin.

This patch separates the management of the "unsafe object" from the code that figures out what object to pass as the "this" parameter to the call we're about to make. And this throws in an additional security check as well, to make sure the caller can access the object we're about to pass in as the "this" parameter.
Attachment #259153 - Flags: superreview?(brendan)
Attachment #259153 - Flags: review?(brendan)
Comment on attachment 259153 [details] [diff] [review]
Fix for latest exploit.

>+  unsafeObj = GetUnsafeObject(cx, obj);
>+  if (!unsafeObj) {
>+    return ThrowException(NS_ERROR_UNEXPECTED, cx);
>+  }
>+
>+  if (!callThisObj) {
>+    callThisObj = unsafeObj;
>+  }
>+

Unless I missed something, unsafeObj is needed only if (callThisObj), so you could move the unsafeObj = GetUnsafeObject(cx, obj) and error check into the if (!callThisObj) -- you could even get rid of unsafeObj (or callThisObj, by using unsafeObj instead, but the name's not as precise). True?

/be
(Assignee)

Comment 48

11 years ago
No, I can't get rid of unsafeObj nor is it only needed in the case where callThisObj is non-null, unsafeObj is used for getting the scripted functions in all cases, so it is needed.
Comment on attachment 259153 [details] [diff] [review]
Fix for latest exploit.

Sorry, had "Match case" set in my find toolbar by mistake!

/be
Attachment #259153 - Flags: superreview?(brendan)
Attachment #259153 - Flags: superreview+
Attachment #259153 - Flags: review?(brendan)
Attachment #259153 - Flags: review+
testcases 7 and 9 "work" in FF2 as well, but that's expected, right? There's no safe wrapper for the testcase to bypass, and the console is already chrome privileged.
Whiteboard: [sg:critical] post 1.8 branch
(Reporter)

Comment 51

11 years ago
(In reply to comment #50)

Yes, that's right.
(Assignee)

Comment 52

11 years ago
Fix checked in, marking FIXED. Please reopen or file new bugs if more problems are found with XPCSafeJSObjectWrapper. Thanks!
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Group: security
Flags: in-testsuite?
We'll want this on the 1.8 branch when we land the SafeJSObjectWrappers themselves.
Blocks: 355766
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13+
Keywords: regression
Whiteboard: [sg:critical] post 1.8 branch → [sg:critical]
moving out a release following the SJsOW bug (bug 355766)
Flags: blocking1.8.1.13+ → blocking1.8.1.14+
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15+
Whiteboard: [sg:critical] → [sg:critical] required by SJsOW
You need to log in before you can comment on or make changes to this bug.