Closed
Bug 889193
Opened 11 years ago
Closed 11 years ago
Assertion failure: !aGCThing after setting expando property on a DOMCursor
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: reuben, Assigned: mccr8)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main24+][adv-esr1709+])
Attachments
(3 files, 2 obsolete files)
2.99 KB,
text/plain
|
Details | |
1.11 KB,
patch
|
Details | Diff | Splinter Review | |
6.63 KB,
patch
|
smaug
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr17+
akeybl
:
approval-mozilla-b2g18+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
var req = mozContacts.getAll({}); req.blabla = function() {}; req is a DOMCursor. Assertion failure: !aGCThing, at /xpcom/base/CycleCollectedJSRuntime.cpp:831
Group: core-security
The assertion that is firing is checking to make sure that we don't have any JS that needs to be traced when we unroot ourselves. If we do we'll fail to trace it, it might be GCd, and we'll get an sg:crit. The problem here is that we have a subclass that NS_HOLD/DROPs itself with no regard for what the base class needs. Andrew, didn't we see a similar problem with a preserved wrapper on XHR?
Flags: needinfo?(continuation)
Perhaps I'm thinking of bug 848535?
Also this seems like something fuzzers should be catching.
Assignee | ||
Comment 4•11 years ago
|
||
Bug 882997 hit the same assertion. Bug 883037 was about a subclass of HTMLDocument getting added with the wrong participant. This isn't a security problem by itself, because if everything is working properly, when an object is unlinked, it isn't reachable. But combined with another problem, like we've seen in the past, and it is a UAF. Maybe mozContacts aren't being fuzzed? Because fuzzers are quite good at finding this particular kind of problem. ;)
Flags: needinfo?(continuation)
Keywords: sec-high
Reuben, can you post the stack you had as an attachment here? IIRC there was no unlinking involved. It was a more complicated test case though.
Flags: needinfo?(reuben.bmo)
Assignee | ||
Comment 6•11 years ago
|
||
But yeah we've seen this before with XHR. I don't remember what bug it was, I think it was one before that one, Kyle. In fact, I have a bug on file about fixing up DOMRequest (bug 852205), because it was doing this weird manual rooting and unrooting via flags that is troublesome, but I didn't think it was actually dangerous. Oops, guess I was wrong!
Reporter | ||
Comment 7•11 years ago
|
||
Flags: needinfo?(reuben.bmo)
Assignee | ||
Comment 8•11 years ago
|
||
I looked into cleaning up how this was handled before, but there are a number of subclasses that do weird things, and their behavior depends on the value of the rooted flag in an odd way, IIRC.
Assignee | ||
Comment 9•11 years ago
|
||
Looks like DOMRequest::UnrootResultVal() needs to clear the wrapper cache?
Assignee | ||
Comment 10•11 years ago
|
||
Well, that's not right, because then you'll lose the wrapper. I think UnrootResultVal needs to drop the call to NS_DROP_JS_OBJECTS(this, DOMRequest), then somehow it has to be dealt with in the destructor.
Yeah, it's clearly broken.
Comment 12•11 years ago
|
||
> Maybe mozContacts aren't being fuzzed? Because fuzzers are quite good at finding
> this particular kind of problem. ;)
Can I enable mozContacts it in a desktop build?
Reporter | ||
Comment 13•11 years ago
|
||
Yes, go to about:config?filter=mozContact and flip all three prefs.
Assignee | ||
Comment 14•11 years ago
|
||
My patch in bug 874679 turns this into Assertion failure: !PreservingWrapper() (Clearing a preserved wrapper!), at /Users/amccreight/mz/cent3/dom/base/nsWrapperCache.h:102 Which maybe is less bad...
Assignee | ||
Comment 15•11 years ago
|
||
Here's a patch to an existing mochitest that triggers this issue, for easier testing.
Comment 16•11 years ago
|
||
> Yes, go to about:config?filter=mozContact and flip all three prefs.
Is it worth testing mixed sets, or only all true and all false?
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 18•11 years ago
|
||
This fixes the problem by removing mRooted from DOMRequest entirely, because it is a recipe for badness. Instead, when we go to stick something in a request, we preserve the wrapper. This means requests will stick around until a CC, which could be bad. I could probably do a HOLD instead if it is a big deal, though that might be trickier. Some of the logic that checked for mRooted seemed kind of bogus to me. Instead, I check if mResult is JSVAL_VOID, which seems no more bogus.
Assignee | ||
Comment 19•11 years ago
|
||
(forgot to remove an unused declaration) https://tbpl.mozilla.org/?tree=Try&rev=a562ab7f674b
Attachment #789213 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #789240 -
Flags: review?(bugs)
Assignee | ||
Comment 20•11 years ago
|
||
I filed bug 904285 for removing mRooted from IndexedDB, the only remaining user of mRooted.
Comment 21•11 years ago
|
||
Comment on attachment 789240 [details] [diff] [review] remove mRooted As far as I see, this could lead to https://bugzilla.mozilla.org/show_bug.cgi?id=848535#c0
Attachment #789240 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 22•11 years ago
|
||
Oh, I forgot about that. We need to fix document then.
Assignee | ||
Comment 23•11 years ago
|
||
I filed bug 904661 for nsDocument.
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #789240 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #790359 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #790359 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•11 years ago
|
status-b2g18:
--- → affected
status-firefox23:
--- → wontfix
status-firefox24:
--- → affected
status-firefox25:
--- → affected
status-firefox26:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-b2g18:
--- → ?
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
tracking-firefox26:
--- → ?
Assignee | ||
Comment 25•11 years ago
|
||
Comment on attachment 790359 [details] [diff] [review] remove mRooted [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably not too difficult, for somebody who knows what to look for. 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? I think all. If not all supported branches, which bug introduced the flaw? It is a bit hard to tell. The problem was either caused by the initial landing of nsDOMDeviceStorageCursor (bug 717103) and DOMCursor (bug 837917), or whatever converted EventTarget to be wrapper cached. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backports will be easy. How likely is this patch to cause regressions; how much testing does it need? It doesn't seem too risky to me, but the original behavior is a little weird, so it is hard to know how things may have come to depend on it.
Attachment #790359 -
Flags: sec-approval?
Assignee | ||
Updated•11 years ago
|
Updated•11 years ago
|
Comment 26•11 years ago
|
||
Comment on attachment 790359 [details] [diff] [review] remove mRooted Sec-approval+ for trunk. I want release management input before we take this on Aurora or Beta though since you call out some unknown risk.
Attachment #790359 -
Flags: sec-approval? → sec-approval+
Updated•11 years ago
|
Flags: needinfo?(release-mgmt)
Assignee | ||
Comment 27•11 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=ef9284fca0eb
Assignee | ||
Comment 28•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1933883ecd5
Comment 29•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a1933883ecd5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•11 years ago
|
Comment 30•11 years ago
|
||
Given we have a few more beta's and after having an offline chat with :mccr8, the risk should be mangeable. Request to land help land asap to get this into today's meeting.
Flags: needinfo?(release-mgmt)
Comment 31•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #30) > Given we have a few more beta's and after having an offline chat with > :mccr8, the risk should be mangeable. > > Request to land help land asap to get this into today's meeting. err, I meant "today's beta"
Assignee | ||
Comment 32•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/952b2b87cc07 https://hg.mozilla.org/releases/mozilla-aurora/rev/a68ce4ee08ed
Comment 33•11 years ago
|
||
Tracking flag for ESR17 wasn't set so this wasn't showing up on radar. We probably need to take this there.
tracking-firefox-esr17:
--- → ?
Updated•11 years ago
|
Flags: needinfo?(release-mgmt)
Comment 34•11 years ago
|
||
Yes, let's get an uplift nomination for ESR branch as well - ooc why is there no patch approval for beta/aurora uplifts?
Flags: needinfo?(release-mgmt) → needinfo?(continuation)
Assignee | ||
Comment 35•11 years ago
|
||
I took bajaj's comment to be tantamount to approval. Sorry if I misunderstood.
Flags: needinfo?(continuation)
Assignee | ||
Comment 36•11 years ago
|
||
Comment on attachment 790359 [details] [diff] [review] remove mRooted [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: sec bug Fix Landed on Version: 24+ Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #790359 -
Flags: approval-mozilla-esr17?
Assignee | ||
Comment 37•11 years ago
|
||
Comment on attachment 790359 [details] [diff] [review] remove mRooted NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): old User impact if declined: sec bug Testing completed: it has been on central, aurora, beta for a while. Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none
Attachment #790359 -
Flags: approval-mozilla-b2g18?
Comment 38•11 years ago
|
||
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
Updated•11 years ago
|
Attachment #790359 -
Flags: approval-mozilla-b2g18?
Comment 39•11 years ago
|
||
Comment on attachment 790359 [details] [diff] [review] remove mRooted Alright - next time let's do the proper flags & nomination form. I'll mark it for data sanity.
Attachment #790359 -
Flags: approval-mozilla-esr17?
Attachment #790359 -
Flags: approval-mozilla-esr17+
Attachment #790359 -
Flags: approval-mozilla-beta+
Attachment #790359 -
Flags: approval-mozilla-aurora+
Comment 40•11 years ago
|
||
Needs a branch-specific patch for the esr17 uplift.
Flags: needinfo?(continuation)
Keywords: branch-patch-needed
Comment 41•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38) > b2g18 is frozen for non-leo+ blockers. Please request blocking status if you > feel that this must land there. I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.
Updated•11 years ago
|
Attachment #790359 -
Flags: approval-mozilla-b2g18+
Updated•11 years ago
|
Assignee | ||
Comment 42•11 years ago
|
||
I'll upload patches for esr17 and b2g18 later today when I'm sure they build.
Flags: needinfo?(continuation)
Assignee | ||
Comment 43•11 years ago
|
||
Well, I can't build ESR17 locally, so I'll use ESR17 TBPL as my try server... https://hg.mozilla.org/releases/mozilla-esr17/rev/e3a032dfb8b5
Assignee | ||
Comment 44•11 years ago
|
||
Likewise. I'll mark these as fixed when the first compile goes through... https://hg.mozilla.org/releases/mozilla-b2g18/rev/506daccfda5a
Assignee | ||
Comment 45•11 years ago
|
||
ESR17 is missing some bootlegging of nsContentUtils.h apparently https://hg.mozilla.org/releases/mozilla-esr17/rev/00fa61bdd586
Assignee | ||
Comment 46•11 years ago
|
||
ESR17 also uses mRooted in FileRequest... https://hg.mozilla.org/releases/mozilla-esr17/rev/6973f5c12100
Assignee | ||
Comment 47•11 years ago
|
||
Looks like B2G18 also needs the include fix, but from inspection not the FileRequest fix. https://hg.mozilla.org/releases/mozilla-b2g18/rev/c5937a004145
Assignee | ||
Updated•11 years ago
|
Comment 48•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/506daccfda5a https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c5937a004145
Comment 49•11 years ago
|
||
Confirmed assertion on m-c 2013-07-10. Verified no assertion on FF17esr, FF24, FF25, FF26, using builds from 2013-09-11.
Updated•11 years ago
|
Whiteboard: [adv-main24+][adv-esr1709+]
Updated•10 years ago
|
Group: core-security
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•