Closed
Bug 986843
Opened 9 years ago
Closed 9 years ago
AutoHoldZone is a hazard with IGC
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: bholley, Assigned: jonco)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [qa-][adv-main29+][adv-esr24.5+])
Attachments
(2 files, 1 obsolete file)
7.29 KB,
patch
|
jonco
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
2.07 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
I spent a few hours mulling over bug 960828 with sfink, terrence, and jonco yesterday. We were still stuck when we left off, but I had an epiphany this morning, and think I have this figured out. The key point here is that the global doesn't actually get collected (that's why we never see the finalizer running). But but the global's _compartment_ does. So global->global() (the culprit, as identified in bug 960828 comment 72), which compiles down to global()->compartment()->maybeGlobal(), returns garbage, rather than identity. The scenario goes like this. XPConnect invokes JS_NewGlobalObject, whose first act of business is to invoke js::NewCompartment. This creates a compartment, and cannot GC. Then, before the call to GlobalObject::create (which can GC), we instantiate an AutoHoldZone instance. Once the global object is created, it is rooted (by virtue of either being on the stack or being in a Rooted<>, depending on the rooting configuration), and therefore its compartment is held alive too. But AutoHoldZone isn't actually good enough. It prevents the zone (and therefore the compartment) from being swept, but it doesn't actually mark anything. So if we happen to begin a GC at the very beginning of GlobalObject::create (before the global itself is actually allocated), we'll do the marking phase, and fail to mark the freshly-allocated compartment. We then return from JS_NewGlobalObject (at which point the AutoHoldZone is out of scope), and at some point later finish up the incremental sweeping. SweepZones invokes SweepCompartments, which finds the compartment unmarked, and deletes it. Boom. We could mark AutoHoldZone smarter, but it might be simpler to just manually GC (if we're getting close) at the beginning of JS_NewGlobalObject, and then use an AutoLockGC for the rest of the call, since this is touchy stuff. GC hackers - thoughts? Once we agree on the approach, I'll go ahead and write the patch.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jcoppeard)
Updated•9 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Keywords: csectype-uaf,
sec-high
OS: Mac OS X → All
Hardware: x86 → All
Comment 1•9 years ago
|
||
Nice investigation, Bobby! Once there's a patch ready, we probably want to land this everywhere, as it might help with various very intermittent JS crashes touching dead objects, like bug 984101.
Comment 2•9 years ago
|
||
FWIW, AutoHoldZone was added in bug 639270.
Comment 3•9 years ago
|
||
Jon and I discussed this IRL last night and we think we should have |class AutoCompartmentRooter : public CustomAutoRooter<JSCompartment*>| and just forward the trace method. Seems much less difficult than messing with GC timings.
Reporter | ||
Comment 4•9 years ago
|
||
Sounds good. I'll write a patch.
Assignee | ||
Comment 5•9 years ago
|
||
bholley, sorry I didn't see your last comment and wrote this patch! I wasn't able to come up with a testcase to reproduce this though -- if anyone has any ideas it would be great if we could test this. I was trying creating new globals with different frequency values for gczeal 9.
Attachment #8395667 -
Flags: review?(terrence)
Flags: needinfo?(jcoppeard)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8395667 [details] [diff] [review] bug986843-removeAutoHoldZone Review of attachment 8395667 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.cpp @@ +2494,3 @@ > Rooted<GlobalObject *> global(cx); > { > + AutoCompartmentRooter hold(cx, compartment); This scoping worries me. How about just giving AutoCompartmentRooter an operator JSCompartment*() doing: AutoCompartmentRooter compartment(cx, NewCompartment(...))?
Comment 7•9 years ago
|
||
Comment on attachment 8395667 [details] [diff] [review] bug986843-removeAutoHoldZone Review of attachment 8395667 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: js/src/jsapi.cpp @@ -2410,5 @@ > { > public: > - explicit AutoHoldZone(Zone *zone > - MOZ_GUARD_OBJECT_NOTIFIER_PARAM) > - : holdp(&zone->hold) We should be able to kill JSCompartment::hold now too.
Attachment #8395667 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Patch updated with comments. Also I renamed the confusing one argument overload of JSCompartment::mark() to markRoots() leaving the no-argument version to set the compartment's mark flag.
Attachment #8395667 -
Attachment is obsolete: true
Attachment #8395785 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05b578dcd6d
Whiteboard: [leave open]
Comment 10•9 years ago
|
||
This should have had sec-approval before landing no? https://wiki.mozilla.org/Security/Bug_Approval_Process Also, does this affect B2G?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 12•9 years ago
|
||
Ah, I'm sorry, I didn't realise that! It's worth pointing out that patch this doesn't necessarily fix the problem, but it's a nice cleanup anyway. Here's the approval request details: How easily could an exploit be constructed based on the patch? I think it would be hard to create an exploit based on this as the GC would have to happen at exactly the right point. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch is a tidy up and removes some unnecessary functionality, so it's not obvious that this is a security fix. Which older supported branches are affected by this flaw? All of them - this code was introduced in 2011 in bug 639270. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Flags: needinfo?(jcoppeard)
Comment 13•9 years ago
|
||
It looks like bug 960828 hasn't happened on inbound since it landed yesterday, which is promising.
Comment 14•9 years ago
|
||
Yeah, bug 960828 hasn't happened anywhere since this happened, so it looks like this worked. Awesome! Can you nominate for landing this on branches, Jon? Thanks. It seems like you can also close both bugs.
Assignee: bobbyholley → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8395785 [details] [diff] [review] bug986843-removeAutoHoldZone [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 639270 User impact if declined: Potential security issue Testing completed: On mozilla-central for a week. Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: None
Attachment #8395785 -
Flags: approval-mozilla-esr24?
Attachment #8395785 -
Flags: approval-mozilla-beta?
Attachment #8395785 -
Flags: approval-mozilla-b2g28?
Attachment #8395785 -
Flags: approval-mozilla-b2g26?
Attachment #8395785 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e05b578dcd6d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-firefox28:
--- → wontfix
status-firefox-esr24:
--- → affected
Keywords: leave-open
Target Milestone: --- → mozilla31
Comment 18•9 years ago
|
||
Comment on attachment 8395785 [details] [diff] [review] bug986843-removeAutoHoldZone Per https://wiki.mozilla.org/Release_Management/B2G_Landing, this will land on the B2G branches once approved and landed on the other Gecko branches.
Attachment #8395785 -
Flags: approval-mozilla-b2g28?
Attachment #8395785 -
Flags: approval-mozilla-b2g26?
Updated•9 years ago
|
Attachment #8395785 -
Flags: approval-mozilla-beta?
Attachment #8395785 -
Flags: approval-mozilla-beta+
Attachment #8395785 -
Flags: approval-mozilla-aurora?
Attachment #8395785 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 19•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18) > Comment on attachment 8395785 [details] [diff] [review] > bug986843-removeAutoHoldZone > > Per https://wiki.mozilla.org/Release_Management/B2G_Landing, this will land > on the B2G branches once approved and landed on the other Gecko branches. Does the "will land" part here mean that this process happens automatically by virtue of having landed on aurora/beta?
Comment 20•9 years ago
|
||
From the link: "sec-high and sec-critical patches have automatic approval to land if the fix has landed on all affected Firefox branches" And the bottom of the page has the bug queries that are looked at daily for such uplifts ;)
Updated•9 years ago
|
Attachment #8395785 -
Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Updated•9 years ago
|
tracking-firefox-esr24:
--- → 29+
Assignee | ||
Comment 21•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=f2fcf4c36ee9
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2fcf4c36ee9 https://hg.mozilla.org/releases/mozilla-beta/rev/bee9c026e827
Updated•9 years ago
|
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr24/rev/42531d5e6ca2
Updated•9 years ago
|
Comment 24•9 years ago
|
||
FYI, Ryan will do landings on older branches for you. He has some queries that look for things with aurora approval set, so you don't even need to set checkin-needed. If you do want to land stuff yourself, then you need to set the status once you land (except on inbound).
Comment 25•9 years ago
|
||
> aurora approval
and similarly for all the other branches, obviously...
Comment 26•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #23) > https://hg.mozilla.org/releases/mozilla-esr24/rev/42531d5e6ca2 Whilst this has an intermittent present, there seems to be other failures too: https://tbpl.mozilla.org/php/getParsedLog.php?id=37140911&tree=Mozilla-Esr24 have retriggered.
Comment 27•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/998d84030f70 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ab6f3500bce2
Assignee | ||
Comment 28•9 years ago
|
||
I've backed this out of mozilla-esr24 as the test failures seem related to the patch: https://hg.mozilla.org/releases/mozilla-esr24/rev/32160fd34f6b
Comment 29•9 years ago
|
||
Same failures on b2g26. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c3527c157a13
Comment 30•9 years ago
|
||
And b2g28. https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f2f9966908cd
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 31•9 years ago
|
||
The assertion that's triggering is complaining that a compartment is not being destroyed even though the zone contains no allocated GC things. The compartment is not being destroyed because it is marked by AutoCompartmentRooter. This is in fact an allowable state now - the point of adding AutoCompartmentRooter was to mark the compartment in case we start a GC before we manage to create its global. It's not happening on recent versions because the code that creates new type objects now suppresses GC for part of its operation and allocates GC things into the zone without the possibility of triggering GC. I think the fix for this is to also not destroy zones if they contain any marked compartments, even if they don't contain any GC things. I'm currently testing a patch for this. I also think it's worth applying this change to mozilla-central (and everywhere else) since it's only an accident of the current arrangement of the object creation code that this doesn't happen there.
Assignee | ||
Comment 32•9 years ago
|
||
Patch to change SweepZones() not destroy a zone if it contains any marked compartments, even if that zone doesn't contain any GC things.
Attachment #8401750 -
Flags: review?(terrence)
Comment 33•9 years ago
|
||
Comment on attachment 8401750 [details] [diff] [review] bug986843-dontSweepMarkedZones Review of attachment 8401750 [details] [diff] [review]: ----------------------------------------------------------------- r=me Clear ownership semantics for compartments? What novelty!
Attachment #8401750 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 34•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded76bd93630
Comment 35•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ded76bd93630
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 36•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/70aebd1f91b2 https://hg.mozilla.org/releases/mozilla-beta/rev/ed9793adc2c7 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/836f5a97d1fd https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5a9b4748be6a https://hg.mozilla.org/releases/mozilla-esr24/rev/219fdd8ca2c6
Updated•9 years ago
|
QA Contact: andrei.vaida
Updated•9 years ago
|
QA Contact: andrei.vaida
Comment 37•9 years ago
|
||
Does this fix require manual QA? If that is the case, could you please advise on how to verify it manually?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 38•9 years ago
|
||
I don't know what the criteria are for requiring manual QA, but this would be hard to test manually as it was very intermittent in the first place, so I'd say not.
Flags: needinfo?(jcoppeard)
Comment 39•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #38) > I don't know what the criteria are for requiring manual QA, but this would > be hard to test manually as it was very intermittent in the first place, so > I'd say not. Thanks for the quick reply, Jon! Marking this as [qa-] based on Comment 38.
Whiteboard: [qa-]
Updated•9 years ago
|
status-b2g-v1.3T:
--- → fixed
Updated•9 years ago
|
Group: dom-core-security → core-security
Updated•9 years ago
|
Whiteboard: [qa-] → [qa-][adv-main29+][adv-esr24.5+]
Updated•8 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•