Closed Bug 761831 Opened 8 years ago Closed 8 years ago

"ASSERTION: obj must be a wrapper" with document.open during DOMNodeRemoved event

Categories

(Core :: XPConnect, defect)

x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox14 --- affected
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
firefox-esr10 15+ verified

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [advisory-tracking+])

Attachments

(3 files)

Attached file testcase
###!!! ASSERTION: obj must be a wrapper: 'IS_WRAPPER_CLASS(js::GetObjectClass(obj)) || js::GetObjectClass(obj) == &XPC_WN_Tearoff_JSClass', file js/xpconnect/src/XPCQuickStubs.cpp, line 374
Attached file stack trace
Bobby: how bad is this assertion in terms of security bugs? Wonder if moz_bug_r_a4 can have some fun with this one.
I'm guessing this is the quickstubs version of bug 760887, which I'm hoping to look at soon. Might be a CPG regression, might not be. Not sure about the severity.
Passing over to Andrew since I think this is the same as bug 760887, which he's working on.

Andrew, if it's not, feel free to bounce this back to me.
Assignee: nobody → continuation
Once again, bholley's prediction about bugs being related is wrong. ;)

From looking at the test case and the code in nsINode::RemoveChild, I think what is happening is that we begin to remove a child of the document, which triggers the DOMNodeRemoved event, which resets the document using open().  Then we look for the child, but it isn't there any more, which causes us to return an error.  So far, that seems okay to me.  Olli, can you confirm that is what is supposed to happen here?

Anyways, once that happens the code tries to handle the error, and we end up in xpc_qsThrowMethodFailed to GetMethodInfo to GetMemberInfo.  The argument to GetMemberInfo is the document pointer of a window object, and is a proxy for the actual HTMLDocument reflector.  The code checks if the argument is a reflector, and it isn't, so it fails the assertion.

GetMemberInfo is pretty dusty looking code that is only called on error paths, so my guess would be that it just hasn't been updated for CPG and it needs to do some kind of unwrap thing.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Once again, bholley's prediction about bugs being related is wrong. ;)
> 
> From looking at the test case and the code in nsINode::RemoveChild, I think
> what is happening is that we begin to remove a child of the document, which
> triggers the DOMNodeRemoved event, which resets the document using open(). 
> Then we look for the child, but it isn't there any more, which causes us to
> return an error.  So far, that seems okay to me.  Olli, can you confirm that
> is what is supposed to happen here?
Sounds ok (I mean, that is what our implementation is supposed to do. There isn't a specification for that part).
I talked with bholley about this on IRC, and he said that we shouldn't just unwrap for no good reason, so instead what I do here is check for the simple case, which is that our object is a reflector.  Otherwise, we just return "Unknown".  Basically, I turn the assertion into a dynamic check.

The existing assertion didn't fail for tearoffs, but in that case the existing code would end up casting a XPCWrappedNativeTearOff to an XPCWrappedNative, which is not okay, so I just don't bother generating a detailed name in that case.

I'm not exactly sure how this result is used, as I don't see "unknown" printed to the error console anywhere, but whatever. 

I removed the bogus comment at the start of the function (as far as I can see, this has nothing in common with either mentioned function).

The code once you get the proto should be the same.
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

I'm try-running this, but I doubt it will find bugs, if there are any...
Attachment #642934 - Flags: review?(bobbyholley+bmo)
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

Please add a comment and check in the testcase. r=bholley with that.
Attachment #642934 - Flags: review?(bobbyholley+bmo) → review+
I have a little bit of a comment in there already.  Should I specifically mention that we aren't handling wrapped reflectors?
(In reply to Andrew McCreight [:mccr8] from comment #10)
> I have a little bit of a comment in there already.  Should I specifically
> mention that we aren't handling wrapped reflectors?

Yes, mention that we're specifically bouncing for security wrappers.
Added the test, added a more explicit comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42e44601363
https://hg.mozilla.org/mozilla-central/rev/b42e44601363
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Given Andrew's guess at the end of comment 5 that this is related to cpg can we safely say ESR-10 is unaffected, or do we need to test it?
(In reply to Daniel Veditz [:dveditz] from comment #14)
> Given Andrew's guess at the end of comment 5 that this is related to cpg can
> we safely say ESR-10 is unaffected, or do we need to test it?

The code in this function is pretty junky, and has an assertion that is too weak.  It is probably a good idea to land my patch on ESR10.  I'd guess that this particular test case wouldn't affect it, but a variant might.
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This code is pretty old.
If this is not a sec:{high,crit} bug, please state case for ESR consideration: I'm not sure exactly how bad this is, but it involves some incorrect casts, so it could be bad.
User impact if declined: possible security problems
Fix Landed on Version: 17
Risk to taking this patch (and alternatives if risky): low.  The patch looks big, but it really just adds a single guard. This is also modifying error message generation code, so it is probably not very common.
String or UUID changes made by this patch: none
Attachment #642934 - Flags: approval-mozilla-esr10?
Attachment #642934 - Flags: approval-mozilla-beta?
Attachment #642934 - Flags: approval-mozilla-aurora?
Comment on attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

[Triage Comment]
Low risk fix for a security issue with amorphous fallout, and we're early in the cycle. Let's get this landed on all branches.
Attachment #642934 - Flags: approval-mozilla-esr10?
Attachment #642934 - Flags: approval-mozilla-esr10+
Attachment #642934 - Flags: approval-mozilla-beta?
Attachment #642934 - Flags: approval-mozilla-beta+
Attachment #642934 - Flags: approval-mozilla-aurora?
Attachment #642934 - Flags: approval-mozilla-aurora+
Whiteboard: [advisory-tracking+]
Flags: in-testsuite?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Should this testcase be put in-testsuite?
Not until 15 is released.
(In reply to Andrew McCreight [:mccr8] from comment #20)
> Not until 15 is released.

Is there a patch ready to go? If not, I'm willing to help provided some guidance.
The test would just be loading the test case and seeing if anything bad happens, preferably in a debug build.  I removed the actual assertion that was tripped, so it may not be super useful.
I'll remove in-testsuite for now since this looks like it needs to be verified manually before Firefox 15 is released.
Flags: in-testsuite?
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa+]
Keywords: verifyme
Whiteboard: [advisory-tracking+][qa+] → [advisory-tracking+]
Confirmed reproducible with 2012-06-05 mozilla-central debug build.

Verified fixed with:
 * 2012-08-27 Firefox 17.0a1 Debug
 * 2012-08-27 Firefox 16.0a2 Debug
 * 2012-08-25 Firefox 15.0 Debug
 * 2012-08-25 Firefox 10.0.7esr Debug
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: anthony.s.hughes
Group: core-security
You need to log in before you can comment on or make changes to this bug.