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

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Veditz [:dveditz]]

Bobby: how bad is this assertion in terms of security bugs? Wonder if moz_bug_r_a4 can have some fun with this one.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Andrew McCreight [:mccr8]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 8

[Andrew McCreight [:mccr8]]

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...

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Andrew McCreight [:mccr8]]

I have a little bit of a comment in there already.  Should I specifically mention that we aren't handling wrapped reflectors?

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Andrew McCreight [:mccr8]]

Added the test, added a more explicit comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42e44601363

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b42e44601363

Comment 14

[Daniel Veditz [:dveditz]]

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?

Comment 15

[Andrew McCreight [:mccr8]]

(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 16

[Andrew McCreight [:mccr8]]

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

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

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

Comment 19

[u279076]

Should this testcase be put in-testsuite?

Comment 20

[Andrew McCreight [:mccr8]]

Not until 15 is released.

Comment 21

[u279076]

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

Comment 22

[Andrew McCreight [:mccr8]]

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.

Comment 23

[u279076]

I'll remove in-testsuite for now since this looks like it needs to be verified manually before Firefox 15 is released.

Comment 24

[u279076]

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