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

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
x86
Mac OS X
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
Created attachment 294336 [details]
testcase

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

Comment 1

11 years ago
Created attachment 294490 [details] [diff] [review]
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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #294490 - Flags: superreview?(dveditz)
Attachment #294490 - Flags: review?(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.
Attachment #294490 - Flags: review?(jst) → review+
(Assignee)

Comment 3

11 years ago
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.
Ah, I see. Fair enough.
Comment on attachment 294490 [details] [diff] [review]
Proposed fix

Make that r+sr=jst
Attachment #294490 - Flags: superreview?(dveditz) → superreview+
Attachment #294490 - Flags: approval1.9+
(Assignee)

Comment 6

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.