Closed Bug 882997 Opened 6 years ago Closed 6 years ago
"Assertion failure: !a
GCThing" with document .write (after using document .all and expando)
153 bytes, text/html
13.18 KB, text/plain
236.70 KB, text/html
2.73 KB, patch
|Details | Diff | Splinter Review|
Assertion failure: !aGCThing, at js/xpconnect/src/XPCJSRuntime.cpp:339 Might be a regression from bug 877277.
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.
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.
(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.
(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+
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+
You need to log in before you can comment on or make changes to this bug.