355766 - (SJsOW) Provide safe way to access XPCNativeWrapper.wrappedJSObject.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Johnny Stenback (:jst)]

As seen in bug 352124 and related bugs, we really need a mechanism for safe access to unwrapped JS objects, including XBL binding methods/properties etc. I've discussed this briefly with Brendan and Jonas, and it seems worth giving this a shot. What we'd do is to provide yet another wrapper that can be used on all JS objects and would provide automatic mirroring of all functions and properties on a JS object and do so in such a way that accessing those functions and properties would run the function and/or getter/setter functions with the principals of the wrapped object (after of course verifying that the called indeed has access to the wrapped object).

I've started hacking on this a bit, but not real code to show yet. More as I have more time for this.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Adding Mark and John here, for their edification.  I think it'd be a huge boon to add-on security.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Implement XPCSafeJSObjectWrapper.

This patch makes XPConnect expose one more JS class, XPCSafeJSObjectWrapper (yes, one more wrapper), and makes XPCNativeWrapper.wrappedJSObject be unconditionally wrapped in one of these new safe wrappers.

This new wrapper is a safe layer between the caller and the callee. It does this by wrapping all function calls and property get/set in scripted functions that are complied with the principals of the callee, and it only permits the safe wrapper to be used at all as long as the caller has access to the callee. This still needs more testing and we still need to figure out exactly how we want this exposed, and whether we need to expose what todays XPCNativeWrapper.wrappedJSObject exposes (though that would need to be exposed with a name that explicitly states that any access to that object is unsafe).

I'd like to start with getting this new JSClass reviewed and landed, and hopefully along the road we can figure out how we do or don't want to expose this to developers.

This new class isn't written to be extremely performant or memory efficient, mostly because I don't think it needs to be. But if that turns out to be something we want to improve on it's relatively easy to add object and smarter scripted function caching to cut down on the numer of objects this creates when used.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this review until at least sometime next week, probably.

Comment 4

[Johnny Stenback (:jst)]

A Win32 build with this patch applied is now available at

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/experimental/XPCSafeJSObjectWrapper/

Thanks to rhelmer for providing the builds!

Anyone who's interested in testing whether this plugs or opens any security holes, please do so!

Comment 5

[moz_bug_r_a4]

Tested with that patch.

Chrome cannot access the safe wrapper's properties, since when chrome accesses
content, ssm->CheckSameOriginPrincipal(subjPrincipal, objPrincipal) fails.

  In Error Console:
  new XPCNativeWrapper(top.opener.content).wrappedJSObject.alert;

  Error: uncaught exception: [Exception... "Access to property denied"  code:
  "1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)"  location:
  "javascript:
  new%20XPCNativeWrapper(top.opener.content).wrappedJSObject.alert%3B Line: 1"]

+  obj = FindSafeObject(cx, obj);
+
+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);

FindSafeObject can return null.

  new XPCSafeJSObjectWrapper(window).toString.call({});
  --> Crash

  new XPCSafeJSObjectWrapper(window).focus.call({});
  --> Crash

+  NS_NAMED_LITERAL_CSTRING(funScript,
+                           "var args = [];"
+                           "for (var i = 1; i < arguments.length; i++)"
+                           "args.push(argument[i]);"
+                           "arguments[0].apply(this, args);");

typo 'argument[i]', and 'return' is missing.

Tested after I fixed the typo.  There is an xss hole.

  <iframe src="victim site"/>

  var sw1 = new XPCSafeJSObjectWrapper(window);
  var sw2 = new XPCSafeJSObjectWrapper(frames[0]);
  sw1.eval.call(sw2, "alert(document.cookie);", frames[0]);

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #5)
> Tested with that patch.
> 
> Chrome cannot access the safe wrapper's properties, since when chrome accesses
> content, ssm->CheckSameOriginPrincipal(subjPrincipal, objPrincipal) fails.
> 
>   In Error Console:
>   new XPCNativeWrapper(top.opener.content).wrappedJSObject.alert;
> 
>   Error: uncaught exception: [Exception... "Access to property denied"  code:
>   "1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)"  location:
>   "javascript:
>   new%20XPCNativeWrapper(top.opener.content).wrappedJSObject.alert%3B Line: 1"]

Fixed locally, last minute goofup in changing the API used to do the same origin checking.

> +  obj = FindSafeObject(cx, obj);
> +
> +  JSObject *unsafeObj = GetUnsafeObject(cx, obj);
> 
> FindSafeObject can return null.
> 
>   new XPCSafeJSObjectWrapper(window).toString.call({});
>   --> Crash
> 
>   new XPCSafeJSObjectWrapper(window).focus.call({});
>   --> Crash

Fixed.

> typo 'argument[i]', and 'return' is missing.

Duh, thanks.

> Tested after I fixed the typo.  There is an xss hole.
> 
>   <iframe src="victim site"/>
> 
>   var sw1 = new XPCSafeJSObjectWrapper(window);
>   var sw2 = new XPCSafeJSObjectWrapper(frames[0]);
>   sw1.eval.call(sw2, "alert(document.cookie);", frames[0]);
> 

The way I'm going to try to fix this is to simply prevent eval and new Script() from ever being used in any way in conjunction with XPCSafeJSObjectWrapper. Keep your eyes on this bug for a new patch, probably tomorrow unless I get to it later tonight.

Comment 7

[moz_bug_r_a4]

(In reply to comment #6)
> The way I'm going to try to fix this is to simply prevent eval and new Script()
> from ever being used in any way in conjunction with XPCSafeJSObjectWrapper.

An attacker can abuse some of DOM functions (e.g. location.assign,
document.write) to perform an XSS attack.

I think that this could be fixed by doing the same origin check in
XPC_SJOW_FunctionWrapper.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Updated XPCSafeJSObjectWrapper diff.

This fixes all the above issues, it blocks the eval exploit by blocking eval (or new Script) from ever being usable through a XPCSafeJSObjectWrapper.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 252648 [details] [diff] [review]
Updated XPCSafeJSObjectWrapper diff.

Feel free to ignore the first changes in this diff (to FeedWriter.js), they're leftover testing code.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Interdiff between the above two diffs

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

>+IsSameOrigin(JSContext *cx, JSObject *unsafeObj)

Possibly better named CanAccessObject or CanCallerAccess or AccessAllowed or something?  It's not really a same-origin check due to the UniversalXPConnect clause.

> +  XPCCallContext ccx(JS_CALLER);

I think we should pass |cx| to that constructor.  I don't really want this code falling into the "look at the JS stack" logic in XPCCallContext(), especially since we have a perfectly good JSContext that we feel reflects the subject, right?

>+  nsCOMPtr<nsIScriptSecurityManager> ssm(do_QueryInterface(sm));

Are we paranoid enough to bail if this is null?  Or are we pretty sure it's never null?

>+  nsCOMPtr<nsIPrincipal> subjPrincipal, objPrincipal;
>+  ssm->GetSubjectPrincipal(getter_AddRefs(subjPrincipal));
>+  ssm->GetObjectPrincipal(cx, unsafeObj, getter_AddRefs(objPrincipal));
>+
>+  if (!subjPrincipal || !objPrincipal) {
>+    return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
>+  }

So I guess if I'm doing a JS_GetProperty from c++ on one of these safe wrappers I'll end up with a sane subjPrincipal (if possible) because ccx will push |cx| on the stack?

>+  nsresult rv = ssm->CheckSameOriginPrincipal(subjPrincipal, objPrincipal);
>+
>+  if (NS_FAILED(rv)) {
>+    PRBool enabled = PR_FALSE;
>+    rv = ssm->IsCapabilityEnabled("UniversalXPConnect", &enabled);
>+
>+    return NS_FAILED(rv) ? ThrowException(rv, cx) : enabled;
>+  }

So I was just thinking about this...

subjPrincipal->Subsumes(objPrincipal) seems like a better thing to call here.  That differs from CheckSameOriginPrincipal only if the subject is a null principal or chrome (in which case it does what we want here) and when subjPrincipal is a certificate principal (in which case it compares certs, instead of codebases, but maybe we should change it to compare both?).  Which seems more correct to me.

If we only care about chrome at that point, we could even remove the IsCapabiltyEnabled call; if we do want to allow UniversalXPConnect stuff, we should keep it.

I could live with doing this as a separate followup patch in general (going over our CheckSameOriginPrincipal callers), I guess.  But I do think it's the right thing to do here.

>+FindObjectPrincipals(JSContext *cx, JSObject *obj)

Is it worth factoring out the "find subject and object principal" code from here and the previous method into a third method?  With a possibly null nsIPrincipal** for the subject if the caller just wants the object?

If we use Subsumes() as above, I think that would work.

>+#define XPC_SJOW_SLOT_IS_RESOLVING           0
>+#define XPC_SJOW_SLOT_SCRIPTED_GETSET        1
>+#define XPC_SJOW_SLOT_SCRIPTED_FUN           2
>+#define XPC_SJOW_SLOT_SCRIPTED_TOSTRING      3

Could you add comments explaining what's stored in those slots and when the value is valid?  I see that we don't init all the slots on object creation, e.g.

>+WrapFunction(JSContext* cx, JSObject* funobj, jsval *rval)

>+  JSFunction *funWrapper =
>+    ::JS_NewFunction(cx, XPC_SJOW_FunctionWrapper, 0, 0, funobj,

I just realized that this applies to the XPCWrappedNative version of this too... do we want to try to get the nargs param right?  It seems worthwhile, but I'd like to do it without a JS_GetProperty on the funobj.  Perhaps a followup bug for jsapi for this?

>+WrapJSValue(JSContext *cx, JSObject *obj, jsval val, jsval *rval)

>+    JSNative native =
>+      ::JS_GetFunctionNative(cx, ::JS_ValueToFunction(cx, val));

As long as you have that here, might as well do the check against XPC_SJOW_FunctionWrapper too, no?  As it is, we get the native again in WrapFunction.

Alternately, push this check down into WrapFunction?

>+      ::JS_ConstructObjectWithArguments(cx, &sXPC_SJOW_JSClass.base, nsnull,
>+                                        JSVAL_TO_OBJECT(val), 1, &val);

It doesn't actually matter that we pass JSVAL_TO_OBJECT(val) for parent here, since Construct will just reget the right parent from the arg, right?  Perhaps document that?

>+    // Propagate cached scripted functions to the new safe object.
>+    jsval rsval;
>+    if (!::JS_GetReservedSlot(cx, obj, XPC_SJOW_SLOT_SCRIPTED_GETSET,
>+                              &rsval) ||
>+        !::JS_SetReservedSlot(cx, safeObj, XPC_SJOW_SLOT_SCRIPTED_GETSET,
>+                              rsval) ||

Do we need to worry about what the globals for these various functions are?  For example, if I have a SJOW for a JSObject that has global1 on its parent chain and I get a property which happens to be a JSObject that has global2 on its parent chain (global1 != global2), we'll copy over the cached functions compiled against global1.  Does that matter?

>+FindSafeObject(JSContext *cx, JSObject *obj)

May be worth reusing IsXPCSafeJSObjectWrapperClass in here, but either way.

I'm up to UnwrapJSValue.  More tomorrow...

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> I just realized that this applies to the XPCWrappedNative version of this
> too... do we want to try to get the nargs param right?  It seems worthwhile,
> but I'd like to do it without a JS_GetProperty on the funobj.  Perhaps a
> followup bug for jsapi for this?

JS_GetFunctionArity has already been added for bug 337389.

Comment 13

[Boris Zbarsky [:bzbarsky]]

>+static JSBool
>+GetScriptedFunction(JSContext *cx, JSObject *obj, JSObject *unsafeObj,
>+                    uint32 slotIndex, const nsAFlatCString& funScript,
>+                    jsval *scriptedFunVal)

Please document what the args mean?  Especially the difference between |obj| and |unsafeObj|?

>+  if (JSVAL_IS_VOID(*scriptedFunVal)) {

So... if the parent chain of the unsafe object changes (e.g. adoptNode somewhere along the line), we should update the cached functions, right?

>+    if (scriptedFunVal == JSVAL_NULL ||

*scriptedFunVal here

Again, is it worth passing in the right thing for nargs here?  I'm guessing it's not worth the bother, since the functions created by this method are wholly internal.

>+XPC_SJOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)

>+  obj = FindSafeObject(cx, obj);

Do we need to worry about this returning null?

>+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);

Likewise.

>+XPC_SJOW_DelProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
>+{
>+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);

And here.

>+XPC_SJOW_FunctionWrapper(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>+  // Function body for wrapping function calls in a scripted
>+  // caller. This functions first argument is the function to call and
>+  // the second argument is the object on which to call the
>+  // function. The rest of the arguments are the argument to pass in
>+  // to the function being called.

"This function's first argument ...", right?

The stuff about the second argument doesn't match the actual function body.  I think the code is right and the comment is wrong.

>+    args = (jsval *)nsMemory::Alloc((argc + 2) * sizeof(jsval *));

argc+1 for the size?

>+  for (uintN i = 0; i < argc; ++i) {
>+    args[i + 1] = UnwrapJSValue(cx, argv[i]);
>+  }

I guess we don't need to root anything because argv is rooted and parents and protos get marked?

>+  jsval val;
>+  JSBool ok = ::JS_CallFunctionValue(cx, unsafeObj, scriptedFunVal, argc + 1,
>+                                     args, &val);

So this makes unsafeObj be the |this| for this function invocation, no matter what object the function was compiled again, right?

>+XPC_SJOW_GetOrSetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp,
>+  obj = FindSafeObject(cx, obj);
>+
>+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);

Again, do we need to worry about those returning null?

>+  // Function body for wrapping property get/set in a scripted
>+  // caller. This functions first argument is the object to get/set
>+  // the property on, the second argument is the property to get. If
>+  // the operation is a set operation, the third argument will be the
>+  // value to set the property to

Again, this comment doesn't match the code; I believe the code is correct.

>+  return ok && WrapJSValue(cx, obj, val, vp);

I think I like this more than the current code at the end of XPC_SJOW_FunctionWrapper.  Make them both look like that?

>+XPC_SJOW_Enumerate(JSContext *cx, JSObject *obj)

>+  obj = FindSafeObject(cx, obj);

Do we need to worry about this being null?

>+  // XXXjst: What if __iterator__ has a getter that'll run? Do we need
>+  // to wrap this somehow, or do we simply not support enumeration? Or
>+  // is it ok to call an untrusted setter in this case? Or should this
>+  // be wrapped in a scripted iteration function?

Check with mrbkap or brendan?  I don't know the iterator stuff well enough yet.  :(

>+    // Let OBJ_LOOKUP_PROPERTY, in particular XPC_SJOW_NewResolve, figure
>+    // out whether this id should be bypassed or reflected.

There's no bypassing going on here, is there?  That's just XPCNativeWrapper stuff...

>+XPC_SJOW_NewResolve(JSContext *cx, JSObject *obj, jsval id, uintN flags,

>+  obj = FindSafeObject(cx, obj);

Do we need to worry about null there?

Does this function need to make an access check to see whether the caller is allowed to access unsafeObj?

>+XPC_SJOW_CheckAccess(JSContext *cx, JSObject *obj, jsval id,
>+                     JSAccessMode mode, jsval *vp)
>+  // Forward to the checkObjectAccess hook in the runtime, if any.
>+  if (cx->runtime->checkObjectAccess &&
>+      !cx->runtime->checkObjectAccess(cx, obj, id, mode, vp)) {
>+    return JS_FALSE;
>+  }

Should we also call the runtime hook for unsafeObj, perhaps?  Or is there a reason this is enough?

>+XPC_SJOW_Construct(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,

>+  if (JS_GET_CLASS(cx, objToWrap) == &js_ScriptClass) {
>+    return ThrowException(NS_ERROR_INVALID_ARG, cx);
>+  }

Do we still need that in light of bug 355820?

I guess on branches we do....  ok.

>+  if (!sEvalNative) {

So this is getting the "eval" property off of Object.prototype in the scope of objToWrap, right?  Is Object.prototype.eval not modifyable by script?

>+XPC_SJOW_Equality(JSContext *cx, JSObject *obj, jsval v, JSBool *bp)

>+    *bp = (obj == other ||
>+           GetUnsafeObject(cx, obj) == other);

We don't need to worry about |obj == GetUnsafeObject(cx, other)| ?  Why not?  That said, though, should this perhaps call GetIdentityObject()?  Otherwise two different SJOW for two different XPCWrappedNatives for the same underlying nsISupports will not test equal, and I think we want them to.  Or do we?

>+XPC_SJOW_toString(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>+                  jsval *rval)
>+{
>+  obj = FindSafeObject(cx, obj);
>+  if (!obj) {
>+    return ThrowException(NS_ERROR_INVALID_ARG, cx);
>+  }
>+
>+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);

Do we need to worry about null?

For that matter, do we want to throw if someone calls toString on SJOW.prototype?  Or return something like "[object XPCSafeJSObjectWrapper]" or whatnot?

As far as our cache functions in the slots go, what keeps them alive?  I don't see us marking them anywhere.  We should be, no?

>diff --git a/js/src/xpconnect/src/xpcwrappednativejsops.cpp b/js/src/xpconnect/src/xpcwrappednativejsops.cpp

>@@ -773,9 +777,19 @@ XPC_WN_Equality(JSContext *cx, JSObject *obj, jsval v, JSBool *bp)

>+            v = OBJECT_TO_JSVAL(XPC_SJOW_GetUnsafeObject(cx,
>+                                                         JSVAL_TO_OBJECT(v)));

|v| can be null at this point.  Need to handle that.

That should be it, I think.  In general, this looks pretty good, but I do think that mrbkap or brendan need to also look over it, esp. the iteration stuff.

Sorry this took so long!

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #11)
> >+  nsCOMPtr<nsIScriptSecurityManager> ssm(do_QueryInterface(sm));
> 
> Are we paranoid enough to bail if this is null?  Or are we pretty sure it's
> never null?

If there's no security manager I don't mind crashing. If an embedding wants to run w/o a security manager, they should stub one out. There's other places in the code where we make this assumption, and IMO we should go through the code that doesn't, and make it assume the same too.

> So I guess if I'm doing a JS_GetProperty from c++ on one of these safe wrappers
> I'll end up with a sane subjPrincipal (if possible) because ccx will push |cx|
> on the stack?

As sane as can be, yes.

> >+  if (NS_FAILED(rv)) {
> >+    PRBool enabled = PR_FALSE;
> >+    rv = ssm->IsCapabilityEnabled("UniversalXPConnect", &enabled);
> >+
> >+    return NS_FAILED(rv) ? ThrowException(rv, cx) : enabled;
> >+  }
> 
> So I was just thinking about this...
> 
> subjPrincipal->Subsumes(objPrincipal) seems like a better thing to call here. 
> That differs from CheckSameOriginPrincipal only if the subject is a null
> principal or chrome (in which case it does what we want here) and when
> subjPrincipal is a certificate principal (in which case it compares certs,
> instead of codebases, but maybe we should change it to compare both?).  Which
> seems more correct to me.
> 
> If we only care about chrome at that point, we could even remove the
> IsCapabiltyEnabled call; if we do want to allow UniversalXPConnect stuff, we
> should keep it.

Agreed. And I decided to leave the UniversalXPConnect stuff in there, in case someone cares. It got a bit ugly to split the principal lookup code due to that, but I can live with it. Let me know if you can't :)

> >+FindSafeObject(JSContext *cx, JSObject *obj)
> 
> May be worth reusing IsXPCSafeJSObjectWrapperClass in here, but either way.

If it was inlined I would, but it's not so I decided to leave it alone.

(In reply to comment #13)
> So... if the parent chain of the unsafe object changes (e.g. adoptNode
> somewhere along the line), we should update the cached functions, right?

Yes, indeed. I thought about that as I was coding this up, but forgot to do anything about it :(

I fixed this by always checking for scope differences between the cached functions and the unsafe object, and if there's a difference we'll re-compile. This, in combination with the fix to only propagate scripted functions to new safe wrappers when old and new are from the same scope brought out the fact that we're often dealing with the different scopes here when working with a window and an object inside that window. In such a case we can end up with wrappers in the outer's and inner's scope, and we now end up not sharing functions between those, which I believe is what we want anyways.

> Again, is it worth passing in the right thing for nargs here?  I'm guessing
> it's not worth the bother, since the functions created by this method are
> wholly internal.

Here I don't see how it makes enough of a difference to care.

> >+XPC_SJOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> 
> >+  obj = FindSafeObject(cx, obj);
> 
> Do we need to worry about this returning null?

I added assertions here, but unless I'm completely wrong, obj is guaranteed to be non-null here since this is a direct JSClass hook, and we wouldn't ever get here unless a safe object is somewhere on the prototype chain.

> >+  JSObject *unsafeObj = GetUnsafeObject(cx, obj);
> 
> Likewise.

This however is a potential problem (potential only becuase the class object is sealed, so noone can add properties etc), fixed.

> >+  for (uintN i = 0; i < argc; ++i) {
> >+    args[i + 1] = UnwrapJSValue(cx, argv[i]);
> >+  }
> 
> I guess we don't need to root anything because argv is rooted and parents and
> protos get marked?

Should be all safe, yes. And runs with TOO_MUCH_GC.

> >+  jsval val;
> >+  JSBool ok = ::JS_CallFunctionValue(cx, unsafeObj, scriptedFunVal, argc + 1,
> >+                                     args, &val);
> 
> So this makes unsafeObj be the |this| for this function invocation, no matter
> what object the function was compiled again, right?

Yes, AFAICT it does. Brendan?

> Should we also call the runtime hook for unsafeObj, perhaps?  Or is there a
> reason this is enough?

Sure, might as well.

> >+  if (JS_GET_CLASS(cx, objToWrap) == &js_ScriptClass) {
> >+    return ThrowException(NS_ERROR_INVALID_ARG, cx);
> >+  }
> 
> Do we still need that in light of bug 355820?
> 
> I guess on branches we do....  ok.

Yes, and until that bug lands...

> >+  if (!sEvalNative) {
> 
> So this is getting the "eval" property off of Object.prototype in the scope of
> objToWrap, right?  Is Object.prototype.eval not modifyable by script?

I moved this code to where it should've been in the first place, and was, except it seemed at the time like that was too early. I figured out what to do to make it possible to do this there and moved the code.

> As far as our cache functions in the slots go, what keeps them alive?  I don't
> see us marking them anywhere.  We should be, no?

The JS engine marks the reserved slots for us AFAIK. Brendan?

> >@@ -773,9 +777,19 @@ XPC_WN_Equality(JSContext *cx, JSObject *obj, jsval v, JSBool *bp)
> 
> >+            v = OBJECT_TO_JSVAL(XPC_SJOW_GetUnsafeObject(cx,
> >+                                                         JSVAL_TO_OBJECT(v)));
> 
> |v| can be null at this point.  Need to handle that.

Yes it can, but that's ok. The Equality() hooks are responsible of not matching to primitives etc.

> Sorry this took so long!

No problem, thanks for taking a close look! I'll attach a new patch and inter-diff with fixes for all of the above.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Interdiff for the last changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 252985 [details] [diff] [review]
Interdiff for the last changes.

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

>+// Find the subject and object principal. The argument
>+// subjectPrincipal can be null if the caller doesn't care about the
>+// subject principal.
>+FindPrincipals(JSContext *cx, JSObject *obj, nsIPrincipal **objectPrincipal,

Add |secMgr| to the comment, since that too can be null?

>+  if (secMgr) {
>+    ssm.swap(*secMgr);

Null out *secMgr first?

>@@ -738,6 +808,13 @@ XPC_SJOW_CheckAccess(JSContext *cx, JSObject *obj, jsval id,

>+  // Forward the unsafe object to the checkObjectAccess hook in the
>+  // runtime too, if any.
>+  if (cx->runtime->checkObjectAccess &&
>+      !cx->runtime->checkObjectAccess(cx, obj, id, mode, vp)) {

s/obj/unsafeObj/ here.

> Yes, and until that bug lands...

Hasn't it, on trunk?  Rev 3.43 of jsconfig.h and rev 3.130 of jsscript.c

In any case, dealing with this on trunk can be a followup; I do agree we want to test this as-is.

r=bzbarsky; just make sure to do that s/obj/unsafeObj/

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 252985 [details] [diff] [review]
Interdiff for the last changes.

sr=me with obj=>unsafeObj fix.

/be

Comment 19

[Johnny Stenback (:jst)]

Attachment: Complete diff of what was checked in.

This fixes the issues pointed out by bz, and also mangles the name GetIdentityObject a bit (made it XPC_GetIdentityObject, and defined it in xpcprivate.h) per suggestion from brendan.

Comment 20

[Johnny Stenback (:jst)]

Marking bug fixed.

Comment 21

[Daniel Veditz [:dveditz]]

Is this something we want back on the 1.8 branches?

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Part of me (a lot of me!) wants this on the branches, but it's technically an incompatible change.  We'd probably have to bump to 2.0.1.x for it, unless I'm wrong and it's only incompatible in the cases where content tried to abuse chrome -- in which case let slip the dogs of wrapping.

Comment 23

[Johnny Stenback (:jst)]

Attachment: 1.8 branch version of the above patch.

This is a patch against a 1.8 branch tree, and this patch also includes the fixes that went in on the trunk for bug 368945. I unfortunately didn't have a chance to test this patch yet, but the changes apply cleanly from trunk to branch so I wouldn't expect any problems with this.

Comment 24

[Johnny Stenback (:jst)]

Attachment: Updated 1.8 branch patch.

Comment 25

[:Gijs (he/him)]

This *is* an incompatible change, in such a sense that ChatZilla is now somewhat broken on trunk. It's working fine on 2.0.0.2, so yes, I think the version number should be upped a notch if this is going to be on branches. See also the blocking bug I just filed. 

If there is a fix from the xpc side for the problem we're having, which allows us to function 'normally' again without code changes on our end, then I suppose it would be ok to land on branch as far as I'm concerned.

Comment 26

[James Ross]

There's a few possible bugs, like:

  typeof safeJSObject.func == "object"  (should be "function")
  safeJSObject.func.call(randomObj)     (probably should fail?)

I strongly object to wrappedJSObject now being something other than the real object, but clearly you have reasons (which I can't access). The biggest issue, however, is that it provides no way (that I can see - there isn't any documentation yet) to access the real JS object any more.

Comment 27

[Boris Zbarsky [:bzbarsky]]

So... that call() thing really shouldn't be working.  In particular, if foo.func() fails, foo.func.call(foo) really shouldn't work, but apparently does?

Comment 28

[Brendan Eich [:brendan]]

Two thoughts:

1. I've been designing a new security model that pushes mandatory access control down into the JS engine and does it universally. I think this is doable for 1.9, but more in a bit. If it is, though, then we should not need to break backward compatibility for XPCNativeWrapper.wrappedJSObject.

2. Even in the short run, to help trunk developers burned by the workaround, we could disable the safe wrapper by default and let those testing it turn it back on. This would be handy for 1, too -- to ensure equivalent or better security with the new security model.

jst, what do you think?

/be

Comment 29

[James Ross]

bz, yeah. The issue is that while
  randomObj.func = safeJSFoo.func;
  randomObj.func();
fails, both
  randomObj.func.call(randomObj);
and
  randomObj.func.apply(randomObj);
work.

Please note that ChatZilla is now employing this bug to get around the more serious issues with safeJSObject, so it would be appreciated if it wasn't fixed without any other workaround available. :)

Comment 30

[Johnny Stenback (:jst)]

(In reply to comment #26)
> There's a few possible bugs, like:
> 
>   typeof safeJSObject.func == "object"  (should be "function")

Yes, this was a necessary change, unfortunately. But how is this a problem if I may ask?

>   safeJSObject.func.call(randomObj)     (probably should fail?)

Why?

> I strongly object to wrappedJSObject now being something other than the real
> object, but clearly you have reasons (which I can't access). The biggest issue,
> however, is that it provides no way (that I can see - there isn't any
> documentation yet) to access the real JS object any more.

That's right, there's no way to get to the real underlying object. And until there's a strong and real need for that I'd rather not expose that. Given access to a content object from chrome code means that any code that does more or less anything with that object is potentially being exploited.

What I would like to know is what the real underlying object is needed for. Given that, we can hopefully fix the wrapper to work in those cases. If there's a need to expose the underlying JS object then I'd rather expose that through a new property whose name makes it extremely clear that any access to it is potentially unsafe.

Comment 31

[James Ross]

> >   typeof safeJSObject.func == "object"  (should be "function")
> Yes, this was a necessary change, unfortunately. But how is this a problem if I
> may ask?

It's not a problem unless you're debugging the damn thing, and have no idea at all why typeof (a function) is giving "object". :)  It was just one of many problems we had in working out why seemingly good code was getting totally wacky errors.

> >   safeJSObject.func.call(randomObj)     (probably should fail?)
> Why?

See comment 29, basically.

At least at the high-level, the following are equal:
  foo.func();
  foo.func.call(foo);
  foo.func.apply(foo);

It would thus make sense that the same held even when foo.func was a safeJSwrapped function, i.e.
  var foo = new Object();
  foo.func = safeJSObject.func;
  foo.func();           // FAILS.
  foo.func.call(foo);   // Works.
  foo.func.apply(foo);  // Works.

I can only assume that the call/apply cases are not going through the safe JS wrapper code, or going through a different part/way. That doesn't seem intended.

> What I would like to know is what the real underlying object is needed for.

ChatZilla loads a page of the user's choice into the main display area; by default, it is a chrome: page. Due to the huge list of bugs and issues with making this display a non-content browser, it is a content one. That means we get smacked by all the wrappers you guys come up. For logistical reasons, turning xpcnativewrappers off is currently not a viable option.

ChatZilla makes various calls into the display area's loaded page, to update the header, and a few other things. This API is fixed, and has been so for many years - the 'this' object for all calls is the view object from ChatZilla's object model, rather than the page's window or similar. These APIs are imported via a getter onto the objects, so:
  chatzillaObject.updateHeader()
will call into the display's function.

Obviously, this import needed to unwrap via wrappedJSObject if it is there, since XPCNativeWrappers doesn't have the JS-defined functions. This new wrapper is now causing it to fail because the 'this' object is the chatzillaObject, not the safeJSObject from the window.

In the strict sense, ChatZilla does not *need* the raw object - it just needs its use of the wrapper to *work*. For XPCNW, the only option was unwrapping; for SJOW, we're exploiting the issue mentioned in comment 29 for now.

Comment 32

[:Gijs (he/him)]

Attachment: Patch to add a simple mochitest that shows the code failing to work consistently.

This teensy patch adds a chrome mochikit test that checks whether both apply() and "normal" calls fail. Right now this test fails, because the function "unCallable" in the test data uri cannot be called directly, but does get called through the apply() call. (the data URI, fwiw, contains just: "<script>function unCallable(){}</script>")

Now, as James said, it would seem odd if this was intensional.

To run the test, apply the patch, run make in $OBJDIR/toolkit/content/tests/chrome and $OBJDIR/toolkit/content/tests/, and then run:

cd $OBJDIR/_tests/testing/mochitest/
perl runtests.pl --chrome

and whack the big "Run Chrome tests" button at the top.

Comment 33

[:Gijs (he/him)]

Comment on attachment 256851 [details] [diff] [review]
Patch to add a simple mochitest that shows the code failing to work consistently.

Oops.

Comment 34

[Johnny Stenback (:jst)]

(In reply to comment #31)
> > >   typeof safeJSObject.func == "object"  (should be "function")
> > Yes, this was a necessary change, unfortunately. But how is this a problem if I
> > may ask?
> 
> It's not a problem unless you're debugging the damn thing, and have no idea at
> all why typeof (a function) is giving "object". :)  It was just one of many
> problems we had in working out why seemingly good code was getting totally
> wacky errors.

Ok, unfortunate, but good to know :)

> > >   safeJSObject.func.call(randomObj)     (probably should fail?)
> > Why?
> 
> See comment 29, basically.
> 
> At least at the high-level, the following are equal:
>   foo.func();
>   foo.func.call(foo);
>   foo.func.apply(foo);
> 
> It would thus make sense that the same held even when foo.func was a
> safeJSwrapped function, i.e.

Indeed.

>   var foo = new Object();
>   foo.func = safeJSObject.func;
>   foo.func();           // FAILS.
>   foo.func.call(foo);   // Works.
>   foo.func.apply(foo);  // Works.
> 
> I can only assume that the call/apply cases are not going through the safe JS
> wrapper code, or going through a different part/way. That doesn't seem
> intended.

Hmm, interesting indeed. Turns out that the calls coming in through .call() and .apply() do go through the exact right code paths, but the direct call (direct indirect call, really) ends up bailing in the XPC_SJOW_Call() hook since it can't find a reference to *any* safe object wrapper. That's easily fixable.

[...]
> In the strict sense, ChatZilla does not *need* the raw object - it just needs
> its use of the wrapper to *work*. For XPCNW, the only option was unwrapping;
> for SJOW, we're exploiting the issue mentioned in comment 29 for now.

Assuming the issue in comment 29 is fixed (already fixed locally, patch to follow), are there any other issues (beyond debuggability concerns etc) that makes it hard for chatzilla to use XPCSafeJSObjectWrapper?

Comment 35

[James Ross]

No, the only issue was that foo.func() [in comment 31] was failing. I look forward to the patch. :)

Comment 36

[Johnny Stenback (:jst)]

Attachment: Make indirect safe wrapper function calls work.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 256866 [details] [diff] [review]
Make indirect safe wrapper function calls work.

>+  if (tmp) {
>+    // A function wrapped in an XPCSafeJSObjectWrapper is being called
>+    // directly (i.e. safeObj.fun()), set obj to be the safe object
>+    // wrapper, and get the unsafe object from it.
>+    obj = tmp;
>+
>+    unsafeObj = GetUnsafeObject(cx, obj);

Aren't these two assignments equivalent to |unsaveObj = obj; obj = tmp;| ?

/be

Comment 38

[Johnny Stenback (:jst)]

(In reply to comment #28)
> Two thoughts:
> 
> 1. I've been designing a new security model that pushes mandatory access
> control down into the JS engine and does it universally. I think this is doable
> for 1.9, but more in a bit. If it is, though, then we should not need to break
> backward compatibility for XPCNativeWrapper.wrappedJSObject.

That'd be great, yes.

> 2. Even in the short run, to help trunk developers burned by the workaround, we
> could disable the safe wrapper by default and let those testing it turn it back
> on. This would be handy for 1, too -- to ensure equivalent or better security
> with the new security model.
> 
> jst, what do you think?


We could do that, but I'd rather see it left on and the problems reported and dealt with than to turn it on in hope that some developers turn it on and find problems. It'd be pretty easy to add a pref check or what not to see if safe wrappers are enabled or not, but I'd rather not go there... 

Comment 39

[Johnny Stenback (:jst)]

(In reply to comment #37)
> (From update of attachment 256866 [details] [diff] [review])
> >+  if (tmp) {
> >+    // A function wrapped in an XPCSafeJSObjectWrapper is being called
> >+    // directly (i.e. safeObj.fun()), set obj to be the safe object
> >+    // wrapper, and get the unsafe object from it.
> >+    obj = tmp;
> >+
> >+    unsafeObj = GetUnsafeObject(cx, obj);
> 
> Aren't these two assignments equivalent to |unsaveObj = obj; obj = tmp;| ?

No, obj = tmp; sets obj to be what FindSafeObject() returned (i.e. the safe object, the XPCSafeJSObjectWrapper, which is likely to be obj as passed to this function, if not, it's one of original obj's prototypes). unsafeObj = GetUnsafeObject(...) gets the *un*safe object, which would be the safe object wrappers parent.

Comment 40

[Boris Zbarsky [:bzbarsky]]

If we take the approach in that patch, should we perhaps always call GetScriptedFunction with the safe obj gotten off JSVAL_TO_OBJECT(argv[-2]) ?  It seems like that would be more consistent, somehow....

Comment 41

[Johnny Stenback (:jst)]

(In reply to comment #40)
> If we take the approach in that patch, should we perhaps always call
> GetScriptedFunction with the safe obj gotten off JSVAL_TO_OBJECT(argv[-2]) ? 
> It seems like that would be more consistent, somehow....

It'd be more consistent, yes, but not as efficient as far as caching principals and scripted functions goes. Either way the outcome is the same, but I'd rather do what the patch does.

Comment 42

[Brendan Eich [:brendan]]

Comment on attachment 256866 [details] [diff] [review]
Make indirect safe wrapper function calls work.

Sorry for the late review.

/be

Comment 43

[Johnny Stenback (:jst)]

Fix landed. James, can you play with this a bit and see if there's still something that's still making it hard for ChatZilla to deal with the presence of XPCSafeJSObjectWrapper objects?

Comment 44

[James Ross]

Certainly; I wont be able to test until Friday, though. If for any reason you need an answer sooner, #chatzilla should be able to help.

Comment 45

[Eric Shepherd [:sheppy]]

Is there a code sample for this I can take advantage of while working on docs?

Comment 46

[Daniel Veditz [:dveditz]]

1.8.1.12 is probably too close to the end now to land this safely, but we want this in 1.8.1.13 for sure to fix mrbkap's branch bugs that depend on this fix.

Comment 47

[chris hofmann]

re: comment 43

check up on https://bugzilla.mozilla.org/show_bug.cgi?id=412207 to follow investigation of possible chatzilla leaks or other errors that came up durning extension leak testing of chatzilla.

Comment 48

[chris hofmann]

I guess the question is, is this error that appears whenever a chatzilla channel joined and new tab openned up expected, or is it a problem...

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Permission denied to get property XULElement.accessKey' when
calling method: [nsIDOMXULLabelElement::accessKey]"  nsresult: "0x8057001e
(NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame ::
XPCSafeJSObjectWrapper.cpp :: anonymous :: line 446"  data: no]
************************************************************

Comment 49

[Daniel Veditz [:dveditz]]

Too late to land safely in 1.8.1.13, _please_ land early in 1.8.1.14

Comment 50

[Eric Shepherd [:sheppy]]

Looks like this has already been documented in:

http://developer.mozilla.org/en/docs/XPCNativeWrapper

and

http://developer.mozilla.org/en/docs/XPConnect_wrappers

Marking this as dev-doc-complete.

Comment 51

[Daniel Veditz [:dveditz]]

I guess we're not going to get this on the 1.8 branch, upgrade to Firefox 3 to get this feature.