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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- wontfix
firefox24 + verified
firefox25 + verified
firefox26 + verified
firefox-esr17 24+ verified
b2g18 24+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: reuben, Assigned: mccr8)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main24+][adv-esr1709+])

Attachments

(3 files, 2 obsolete files)

var req = mozContacts.getAll({});
req.blabla = function() {};

req is a DOMCursor.

Assertion failure: !aGCThing, at /xpcom/base/CycleCollectedJSRuntime.cpp:831
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)
Also this seems like something fuzzers should be catching.
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)
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!
Attached file Stack —
Flags: needinfo?(reuben.bmo)
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.
Looks like DOMRequest::UnrootResultVal() needs to clear the wrapper cache?
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.
> 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?
Yes, go to about:config?filter=mozContact and flip all three prefs.
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...
Attached patch patch for mochitest — — Splinter Review
Here's a patch to an existing mochitest that triggers this issue, for easier testing.
> 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: nobody → continuation
Attached patch remove mRooted (obsolete) — — Splinter Review
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.
Attached patch remove mRooted (obsolete) — — Splinter Review
(forgot to remove an unused declaration)

https://tbpl.mozilla.org/?tree=Try&rev=a562ab7f674b
Attachment #789213 - Attachment is obsolete: true
Attachment #789240 - Flags: review?(bugs)
I filed bug 904285 for removing mRooted from IndexedDB, the only remaining user of mRooted.
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-
Oh, I forgot about that.  We need to fix document then.
I filed bug 904661 for nsDocument.
Attached patch remove mRooted — — Splinter Review
Attachment #789240 - Attachment is obsolete: true
Attachment #790359 - Flags: review?(bugs)
Attachment #790359 - Flags: review?(bugs) → review+
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?
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+
Flags: needinfo?(release-mgmt)
https://hg.mozilla.org/mozilla-central/rev/a1933883ecd5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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)
(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"
Tracking flag for ESR17 wasn't set so this wasn't showing up on radar. We probably need to take this there.
Flags: needinfo?(release-mgmt)
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)
I took bajaj's comment to be tantamount to approval.  Sorry if I misunderstood.
Flags: needinfo?(continuation)
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?
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?
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
Attachment #790359 - Flags: approval-mozilla-b2g18?
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+
Needs a branch-specific patch for the esr17 uplift.
Flags: needinfo?(continuation)
(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.
Attachment #790359 - Flags: approval-mozilla-b2g18+
I'll upload patches for esr17 and b2g18 later today when I'm sure they build.
Flags: needinfo?(continuation)
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
Likewise.  I'll mark these as fixed when the first compile goes through...

https://hg.mozilla.org/releases/mozilla-b2g18/rev/506daccfda5a
ESR17 is missing some bootlegging of nsContentUtils.h apparently
https://hg.mozilla.org/releases/mozilla-esr17/rev/00fa61bdd586
ESR17 also uses mRooted in FileRequest...
https://hg.mozilla.org/releases/mozilla-esr17/rev/6973f5c12100
Looks like B2G18 also needs the include fix, but from inspection not the FileRequest fix.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c5937a004145
Confirmed assertion on m-c 2013-07-10.
Verified no assertion on FF17esr, FF24, FF25, FF26, using builds from 2013-09-11.
Whiteboard: [adv-main24+][adv-esr1709+]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.