409514 - "({}).__proto__ = XPCSafeJSObjectWrapper.prototype;" halts JS execution

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

"({}).__proto__ = XPCSafeJSObjectWrapper.prototype;" halts JS execution.  It should either work or throw an exception that can be caught.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

The underlying problem here is that we have to do a security check on the right hand side of the __proto__ setting line. However, in this case, the right hand side is XPCSafeJSObjectWrapper.prototype, which does not have a parent. This means that we cannot get its object principals and caps gives up. I *could* make it stash its original principals somewhere and use those, but I don't think it's worth the effort. Instead, this patch just ensures that if we're unable to find object principals off of an object, we fall down into the code below that actually reports the error, allowing it to be caught.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 294490 [details] [diff] [review]
Proposed fix

r=jst.

I noticed as I was looking through this that further down in this function we'll get to the code that does this:

    nsCOMPtr<nsISecurityCheckedComponent> checkedComponent =
        do_QueryInterface(aObj);

    nsXPIDLCString objectSecurityLevel;
    if (checkedComponent)
    {
        ...
    }
    rv = CheckXPCPermissions(aObj, objectSecurityLevel);

and that code will override the rv that you set here, presumably with a different error code (though it might be the same code too, come to think of it). Either way, it seems more correct, and more performant in some cases, to move the call to CheckXPCPermissions() inside the if (checkedComponent) block.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

So, I don't think I can do that because CheckXPCPermissions is what does the UniversalXPConnect check. In fact, I think it makes sense to let UnviersalXPConnect scripts access objects that don't have principals.

Comment 4

[Johnny Stenback (:jst)]

Ah, I see. Fair enough.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 294490 [details] [diff] [review]
Proposed fix

Make that r+sr=jst

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.