368945 - Security hole in XPCSafeJSObjectWrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[moz_bug_r_a4]

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);");

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - XSS

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - arbitrary code execution

Comment 3

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Johnny Stenback (:jst)]

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

Fix checked in.

Comment 6

[moz_bug_r_a4]

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>");

Comment 7

[moz_bug_r_a4]

Attachment: testcase 3 - arbitrary code execution

Comment 8

[moz_bug_r_a4]

Attachment: testcase 4 - XSS

Comment 9

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 10

[Brendan Eich [: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

Comment 11

[Johnny Stenback (:jst)]

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!

Comment 12

[moz_bug_r_a4]

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.

Comment 13

[moz_bug_r_a4]

Attachment: testcase 5 - XSS

Comment 14

[Boris Zbarsky [:bzbarsky]]

Would it help if XPC_SJOW_FunctionWrapper did a CanCallerAccess check on unsafeObj?  Seems to me like it should....

Comment 17

[moz_bug_r_a4]

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);");

Comment 18

[Brendan Eich [:brendan]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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?

Comment 21

[Johnny Stenback (:jst)]

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.

Comment 22

[Johnny Stenback (:jst)]

Attachment: 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().

Comment 23

[Brendan Eich [:brendan]]

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 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Johnny Stenback (:jst)]

(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.

Comment 26

[Johnny Stenback (:jst)]

Attachment: Fix issues in previous patch.

This fixes the issues that bz found, and initializes a JSPrincipall pointer to nsnull, just in case.

Comment 27

[Boris Zbarsky [:bzbarsky]]

No need to do the *rval setting at the end of the function now.

Comment 28

[Johnny Stenback (:jst)]

Fixed locally.

Comment 29

[Johnny Stenback (:jst)]

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).

Comment 30

[Johnny Stenback (:jst)]

Attachment: Updated fix that fixes the last two testcases.

Comment 31

[Johnny Stenback (:jst)]

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!

Comment 34

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 35

[Johnny Stenback (:jst)]

Attachment: Same as above w/o XXX comments.

I got verbal r+sr=brendan for this, I'm checking this in now.

Comment 36

[Johnny Stenback (:jst)]

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.

Comment 37

[Johnny Stenback (:jst)]

And marking this fixed. Please reopen if more issues surface from the last change.

Comment 38

[moz_bug_r_a4]

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();

Comment 39

[moz_bug_r_a4]

Attachment: testcase 8 - crash

Comment 40

[Johnny Stenback (:jst)]

Attachment: 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

Comment 41

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 42

[Brendan Eich [:brendan]]

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

Comment 43

[Johnny Stenback (:jst)]

That patch already landed, with verbal r+sr=brendan :)

Comment 44

[moz_bug_r_a4]

"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.

Comment 46

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 47

[Brendan Eich [: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

Comment 48

[Johnny Stenback (:jst)]

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 49

[Brendan Eich [:brendan]]

Comment on attachment 259153 [details] [diff] [review]
Fix for latest exploit.

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

/be

Comment 50

[Daniel Veditz [:dveditz]]

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.

Comment 51

[moz_bug_r_a4]

(In reply to comment #50)

Yes, that's right.

Comment 52

[Johnny Stenback (:jst)]

Fix checked in, marking FIXED. Please reopen or file new bugs if more problems are found with XPCSafeJSObjectWrapper. Thanks!

Comment 53

[Daniel Veditz [:dveditz]]

We'll want this on the 1.8 branch when we land the SafeJSObjectWrappers themselves.

Comment 54

[Daniel Veditz [:dveditz]]

moving out a release following the SJsOW bug (bug 355766)