882997 - "Assertion failure: !aGCThing" with document.write (after using document.all and expando)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: !aGCThing, at js/xpconnect/src/XPCJSRuntime.cpp:339

Might be a regression from bug 877277.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Attachment: stack+context

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

That assertion indicates that the object holds a pointer to a JS thing when it is being dropped, which in theory could be chained with something else to cause a UAF.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Hmm, so DOMProxyHandler::GetAndClearExpandoObject does a DROP without clearing anything, and it isn't clear that it actually can.  Bug 874679 would deal with this, but maybe it can be fixed otherwise?

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

That code was introduced in bug 871849, which is on 23 and 24, so we can't really backport 874679 to 23.

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Okay, so this really is just a regression from bug 877277, or at least this particular case.  Prior to that, when we hit this code, the only JS pointer the nsISupports object had was the wrapper cache, which is manually cleared by the code.  With the addition of bug 877277, suddenly we have an additional JS pointer that isn't cleared.

Peter, are documents the only thing that can be in this native thing at the moment?  And prior to 877277, do none of them hold any JS pointers aside from the wrapper cache?  If both of those hold, we should be okay on 23, and I can fix this on 24 using bug 874679, unless anybody else has a better idea.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Peter, are documents the only thing that can be in this native thing at the
> moment?

What native thing?

> And prior to 877277, do none of them hold any JS pointers aside
> from the wrapper cache?

I don't know. But afaik the DropJSObjects call in ReleaseWrapper has been there for a really long time, and we never took care of clearing any other JS pointers at that point.

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #7)
> What native thing?
Sorry, I meant |native| in DOMProxyHandler::GetAndClearExpandoObject.

> I don't know. But afaik the DropJSObjects call in ReleaseWrapper has been
> there for a really long time, and we never took care of clearing any other
> JS pointers at that point.

Right, but I think we only call ReleaseWrapper in Unlink and the destructor.  We've always cleared these pointers in Unlink, and Olli added code to be more consistent about doing it in the destructor.  The latter is mostly just to keep the assertions from triggering.

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> (In reply to Peter Van der Beken [:peterv] from comment #7)
> > What native thing?
> Sorry, I meant |native| in DOMProxyHandler::GetAndClearExpandoObject.

For now only HTML documents (which includes ImageDocument). Bug 841442 is planning to add form elements. And yes, I do think that prior to bug 877277 they only held a wrapper.

I wonder if we could make ReparentWrapper just not drop, since we're really replacing one expando object with another. But this is all hairy code, so it might not work.

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: don't DROP or clear wrapper cache flag in GetAndClearExpandoObject

I did what Peter suggested, and it seems to work pretty well.  When GetAndClear returns, it still has the preserved wrapper flag set, and it is held.  I still need to think about whether this is okay a little bit more.  My main concern is that early returns from ReparentWrapper will leave the wrapper in a weird state, where it is preserved, but it points to undefined.  That doesn't seem de facto unsafe to me, but maybe it would violate some invariants somewhere.

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 764274 [details] [diff] [review]
don't DROP or clear wrapper cache flag in GetAndClearExpandoObject

How does this look to you, Peter?  My main concern here is that if we fail in ReparentWrapper then we end up with a preserved, held wrapper, but the wrapper is just undefined.  We could get in a weird state somehow, though I don't think anything particularly unsafe, just crashy.

https://tbpl.mozilla.org/?tree=Try&rev=c21852be5d44

Comment 12

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 764274 [details] [diff] [review]
don't DROP or clear wrapper cache flag in GetAndClearExpandoObject

[Security approval request comment]
How easily could an exploit be constructed based on the patch? not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? 24

If not all supported branches, which bug introduced the flaw? 877277

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? easy, no risk

How likely is this patch to cause regressions; how much testing does it need? unlikely

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 764274 [details] [diff] [review]
don't DROP or clear wrapper cache flag in GetAndClearExpandoObject

sec-approval+ for trunk. Let's get an aurora patch attached with approval requested too after it lands.

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/411c60295786

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/411c60295786

Comment 16

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 764274 [details] [diff] [review]
don't DROP or clear wrapper cache flag in GetAndClearExpandoObject

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 877277 
User impact if declined: sec bug
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f5d65c04085