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

VERIFIED FIXED in Firefox 15

Status

()

Core
XPConnect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla17
x86_64
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox14 affected, firefox15 verified, firefox16 verified, firefox17 verified, firefox-esr1015+ verified)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 630346 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 630348 [details]
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
(Assignee)

Comment 5

5 years ago
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.

Comment 6

5 years ago
(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).
(Assignee)

Comment 7

5 years ago
Created attachment 642934 [details] [diff] [review]
only generate error messages for simple cases

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

Comment 8

5 years ago
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+
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 12

5 years ago
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
Last Resolved: 5 years ago
status-firefox17: --- → fixed
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?
tracking-firefox-esr10: --- → ?
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
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+

Updated

5 years ago
tracking-firefox-esr10: ? → 15+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/6ac9f90b1f93
https://hg.mozilla.org/releases/mozilla-beta/rev/10322e173818
https://hg.mozilla.org/releases/mozilla-aurora/rev/69c8c2360d62
status-firefox-esr10: --- → fixed
status-firefox14: --- → affected
status-firefox15: --- → fixed
status-firefox16: --- → fixed
Whiteboard: [advisory-tracking+]
Flags: in-testsuite?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Should this testcase be put in-testsuite?
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 22

5 years ago
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
status-firefox-esr10: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → 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.