Closed Bug 882997 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(5 keywords)

Attachments

(4 files)

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

Might be a regression from bug 877277.
Attached file stack
Attached file stack+context
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.
Keywords: csec-uaf, sec-high
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?
That code was introduced in bug 871849, which is on 23 and 24, so we can't really backport 874679 to 23.
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.
Assignee: nobody → continuation
Blocks: 877277
Flags: needinfo?(peterv)
Keywords: regression
(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.
Flags: needinfo?(peterv)
(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.
(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.
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 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
Attachment #764274 - Flags: review?(peterv)
Attachment #764274 - Flags: review?(peterv) → review+
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
Attachment #764274 - Flags: sec-approval?
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.
Attachment #764274 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/411c60295786
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #764274 - Flags: approval-mozilla-aurora?
Attachment #764274 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.