Closed
Bug 980537
Opened 10 years ago
Closed 10 years ago
mozJSComponentLoader::mThisObjects is not properly rooted
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
People
(Reporter: jandem, Assigned: bholley)
References
Details
(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main29+][adv-esr24.5+][qa-])
Attachments
(3 files)
8.43 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
9.14 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
khuey
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr24+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
I was just looking at a topcrash, bug 950653 (see bug 950653 comment 106 for more info). I noticed that the mozJSComponentLoader::mThisObjects script/object hash table is not traced explicitly. There are two places where we add new items: (1) mozJSComponentLoader::ObjectForLocation. Here we do: JS_AddNamedObjectRoot(cx, aObject, *aLocation); JS_AddNamedScriptRoot(cx, aTableScript, *aLocation); (2) mozJSComponentLoader::NoteSubScript, which does not do anything like this. Is it possible that (2) is not correctly keeping the script/object alive, and we can run into use-after-free bugs? Am I missing something? Also CC'ing Terrence to make sure he's aware of this table for GGC.
Flags: needinfo?(bobbyholley)
Reporter | ||
Comment 1•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0) > I was just looking at a topcrash, bug 950653 Er, intermittent orange. (Maybe also a topcrash, I don't know.)
Assignee | ||
Comment 2•10 years ago
|
||
Yes, that looks wrong. Kyle wrote it apparently. ObjectForLocation could also use some RAII stack guards.
Flags: needinfo?(bobbyholley) → needinfo?(khuey)
I clearly don't know what the right thing to do here is :)
Flags: needinfo?(khuey)
Assignee | ||
Comment 4•10 years ago
|
||
Ok, then let's let terrence fix this up the way he wants for GGC.
Flags: needinfo?(terrence)
Updated•10 years ago
|
Keywords: csectype-uaf,
sec-critical
Assignee | ||
Updated•10 years ago
|
Summary: Is mozJSComponentLoader::mThisObjects properly rooted? → mozJSComponentLoader::mThisObjects is not properly rooted
Comment 5•10 years ago
|
||
Looks like this landed in Fx19. So at least b2g18 is unaffected.
status-b2g18:
--- → unaffected
status-firefox27:
--- → wontfix
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox-esr24:
--- → affected
I'm 99% sure we backported it to b2g 18.
Comment 7•10 years ago
|
||
Hah, right you are...
Updated•10 years ago
|
blocking-b2g: --- → 1.3?
Comment 8•10 years ago
|
||
This fixes ModuleEntry to work with moving GC. Although we currently only add roots for the ModuleEntry gc thing pointers after a GC, I think this is probably not a problem currently because the objects in question should be held live by Rooted instances. Once we start moving objects, however, this pattern will break horribly. I'll continue looking for the root cause of the crashes, but we should check this in anyway in case I missed a way the current situation could lead to badness.
Updated•10 years ago
|
blocking-b2g: 1.3? → 1.3+
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #8) > I'll continue looking for the root cause of the crashes, but we should check > this in anyway in case I missed a way the current situation could lead to > badness. The root cause is what I described in comment 0, mThisObjects not being rooted, right?
Comment 10•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #9) > (In reply to Terrence Cole [:terrence] from comment #8) > > I'll continue looking for the root cause of the crashes, but we should check > > this in anyway in case I missed a way the current situation could lead to > > badness. > > The root cause is what I described in comment 0, mThisObjects not being > rooted, right? Maybe? Probably? That table /should/ only have global objects, which /should/ be kept live by the compartment, and they /should/ be removed from this table before the compartment gets brought down. That said, it's the browser so who knows what's actually happening. It's an unnecessary, sharp edge in any case. I've got a patch almost working to mark mThisObjects, but I'm worried that it will trigger the leak detector. I am going to try to polish that up and do a Try run this afternoon.
Flags: needinfo?(terrence)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #10) > (In reply to Jan de Mooij [:jandem] from comment #9) > > (In reply to Terrence Cole [:terrence] from comment #8) > > > I'll continue looking for the root cause of the crashes, but we should check > > > this in anyway in case I missed a way the current situation could lead to > > > badness. > > > > The root cause is what I described in comment 0, mThisObjects not being > > rooted, right? > > Maybe? Probably? That table /should/ only have global objects Not on b2g, right?
On b2g mThisObjects will have no global objects. That's kinda the entire point here.
Comment 13•10 years ago
|
||
Comment on attachment 8387946 [details] [diff] [review] fix_moduleentry_for_moving_gc-v0.diff Review of attachment 8387946 [details] [diff] [review]: ----------------------------------------------------------------- I'm going blind. You don't want to know how many times I read through this patch, wondering how anything was rooted now, before finally noticing the addition of PersistentRooted. A very nice use for it.
Attachment #8387946 -
Flags: review?(sphink) → review+
Comment 14•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > On b2g mThisObjects will have no global objects. That's kinda the entire > point here. Well... shit. Has this totally unrooted code leaked into an actual b2g release yet? If so we're going to want to land these under a cover bug or the patches will be a giant, neon "insert exploit here".
Attachment #8388871 -
Flags: review?(sphink)
Comment 15•10 years ago
|
||
Should we make my earlier comment on the original bug (https://bugzilla.mozilla.org/show_bug.cgi?id=950653#c43) private as well or would that just attract unwanted attention and is vague enough anyway?
Comment 16•10 years ago
|
||
Comment on attachment 8388871 [details] [diff] [review] fix_mozComponentLoader_for_moving_gc-v0.diff Review of attachment 8388871 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/loader/mozJSComponentLoader.cpp @@ +346,5 @@ > +{ > + auto loader = mozJSComponentLoader::Get(); > + MOZ_ASSERT_IF(loader, data == loader); > + if (loader) > + loader->Trace(trc); Needs moar braces. (So you might want to put the assert inside too.) @@ +356,5 @@ > + for (ThisObjectsMap::Range r = mThisObjects.all(); !r.empty(); r.popFront()) { > + JS_CallScriptTracer(trc, const_cast<JSScript**>(&r.front().key()), > + "mozJSComponentLoader::mThisObjects key"); > + JS_CallHeapObjectTracer(trc, &r.front().value(), > + "mozJSComponentLoader::mThisObjects key"); "mozJSComponentLoader::mThisObjects value" @@ +594,5 @@ > MOZ_CRASH(); > } > > + if (!mThisObjects.put(aScript, aThisObject)) { > + MOZ_CRASH(); Isn't that fun to do? :-)
Attachment #8388871 -
Flags: review?(sphink) → review+
Comment 17•10 years ago
|
||
I need to know if this has been in any B2G releases before landing here. Should I open a cover bug?
Flags: needinfo?(khuey)
Comment 18•10 years ago
|
||
I'd like to get this in today, so I went ahead and opened cover bug 983399.
Flags: needinfo?(khuey)
This has been in every b2g release.
Comment 20•10 years ago
|
||
The trivial approach of simply marking the things that are presumably alive -- we are storing a pointer to the thing in the system after all -- seems to have made Mochitest-bc OOM. https://tbpl.mozilla.org/?tree=Try&rev=8027af0c37dd This strongly implies that some portion of the pointers in this table are actually dead. I'd guess we happen not to access these script globals because if the compartment is gone, so are the scripts which would normally be touching the script's global. This unfortunately makes the correct tool a weakmap, which we only started exposing this release. As this is only a problem on B2G, maybe the current patch in bug 983399 would be good enough to uplift to b2g branches?
Comment 21•10 years ago
|
||
We can't use a weakmap, because the weakmap must be in the same compartment of all of its contained items. It turns out this isn't really a problem though. We only ever read from the map on B2G, so we can just use the same filter when adding to the map as when reading from the map and we end up with the same semantics we have now. I'm going to push with that addition in the cover bug.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #21) > We can't use a weakmap, because the weakmap must be in the same compartment > of all of its contained items. Why is this a problem? We can just store CCWs to the objects and unwrap them when necessary.
Updated•10 years ago
|
status-firefox31:
--- → affected
Updated•10 years ago
|
Comment 23•10 years ago
|
||
Terrence, what's the status here? Do we understand how to move forward here?
Updated•10 years ago
|
Flags: needinfo?(terrence)
Comment 24•10 years ago
|
||
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #23) > Terrence, what's the status here? Do we understand how to move forward here? No, I have no idea. If we root the map we OOM, if we don't root we have a use after free. I have no idea when it would be safe to remove items from the map, so someone more familiar with this code is going to have to take it from here.
Assignee: terrence → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(terrence)
Comment 26•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #25) > /me will have a look at this Thanks bholley! Feel free to use pine for debug purposes: https://tbpl.mozilla.org/?tree=Pine&showall=1
Comment 27•10 years ago
|
||
Seems like the landing of bug 967516 caused a new spike here.
Updated•10 years ago
|
tracking-firefox31:
--- → +
Assignee | ||
Comment 28•10 years ago
|
||
I just dug into this. There's a bit of confusion and misinformation in this bug, so I'm going to clarify what's actually going on. There are actually two hazards here. :-( mThisObjects is a map added by Kyle in bug 810987. This structure maps from JSScript* -> JSObject* instances representing the scope object for each script (either a bonafide global or a FakeBackstagePass). This is necessary to make the special b2g-style scoping transitive in the case of recursive imports and subscript loads. In both cases, GetTargetObject checks to see if the enclosing scope of the caller leads to a FakeBackstagePass object. If it is, it returns that (to be used as a scope object for the import/load). If it isn't, it just returns the caller's global. FakeBackstagePass instances are only created in module loading, not subscript loading. This happens in ObjectForLocation, which creates either a new global or a new FakeBackstagePass, roots it, and stashes it both in a module and the mThisObjects map. In this path, the stuff that goes into mThisObjects is always rooted. Later on, in bug 811784, Kyle realized that subscripts needed to account for mThisObjects as well - if a b2g-ified module loads a subscript, the subscript loader finds its caller's scope object in the mThisObject, and everything is properly imported to the right scope. But if _that_ subscript loads _another_ subscript, we'll end up scoping the inner subscript to the global unless we have a mapping from the outer subscript to the FakeBackstagePass. So the patch in bug 811784 stashes a mapping between subscripts and their |this|-objects into mThisObjects. In the case where the |this| object is a FakeBackstagePass, everything is peachy because the |this| object was found in the map, and is therefore already rooted. But in the case where the |this| object is just some random global doing loadSubScript, we put a totally unrooted entry into the map. The key (the JSScript*) is guaranteed to go away before the value (its global), so in general we just end up with a dead entry in the map. _Except_ in the case where the same piece of memory is re-used for a different JSScript* later on - which is presumably what's happening in bug 950653. We compile a script, do a lookup for a |this| object, hit a key corresponding to a long-dead JSScript*, and try to use its long-dead global. This, incidentally, is why rooting in NotSubScript doesn't work - it effectively leaks any global that does Cu.import for the lifetime of the browser, causing us to OOM. The second hazard comes from the fact that mThisObjects is totally oblivious to the concept of module unloading - indeed, most of the code surrounding it seems based on the assumption that modules never go away. Thankfully, mThisObjects is only consulted on b2g, and it looks like Cu.unload is never used on b2g. But this is a pretty big footgun. And it's a hard problem to fix, given how the code works. In summary, we can fix the first issue (which actually manifests itself in the wild) by making sure to only store FakeBackstagePass instances in mThisObjects. For the second issue, I think we should probably just get rid of this whole mechanism altogether. I'll file a bug.
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8398569 -
Flags: review?(khuey)
Updated•10 years ago
|
status-b2g-v1.1hd:
--- → wontfix
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
Attachment #8398569 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8398569 [details] [diff] [review] Only store FakeBackstagePass instances in mThisObjects. v1 [Security approval request comment] How easily could an exploit be constructed based on the patch? It's a GC hazard. Tricky to exploit, but possible. 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? Anything b2g. If not all supported branches, which bug introduced the flaw? bug 811784. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backport should all be the same. How likely is this patch to cause regressions; how much testing does it need? Probably doesn't need too much testing.
Attachment #8398569 -
Flags: sec-approval?
Updated•10 years ago
|
tracking-firefox29:
--- → +
tracking-firefox30:
--- → +
Comment 31•10 years ago
|
||
Comment on attachment 8398569 [details] [diff] [review] Only store FakeBackstagePass instances in mThisObjects. v1 sec-approval+. Let's get this on branches too.
Attachment #8398569 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 32•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5aa27163c49d
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #32) > https://tbpl.mozilla.org/?tree=Try&rev=5aa27163c49d Green, modulo browserchromepocalypse. https://hg.mozilla.org/integration/mozilla-inbound/rev/c9585852983b
Assignee | ||
Comment 34•10 years ago
|
||
Comment on attachment 8398569 [details] [diff] [review] Only store FakeBackstagePass instances in mThisObjects. v1 This only really affects b2g, but it can't hurt to take it in other places. I'm not sure which b2g approval I need, so I'm just going to request approval everywhere. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 811784 User impact if declined: GC hazard on b2g Testing completed (on m-c, etc.): Just pushed to m-i Risk to taking this patch (and alternatives if risky): Low risk String or IDL/UUID changes made by this patch: None
Attachment #8398569 -
Flags: approval-mozilla-esr24?
Attachment #8398569 -
Flags: approval-mozilla-beta?
Attachment #8398569 -
Flags: approval-mozilla-aurora?
Comment 35•10 years ago
|
||
landed on m-c https://hg.mozilla.org/mozilla-central/rev/c9585852983b
Target Milestone: --- → mozilla31
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Attachment #8398569 -
Flags: approval-mozilla-esr24?
Attachment #8398569 -
Flags: approval-mozilla-esr24+
Attachment #8398569 -
Flags: approval-mozilla-beta?
Attachment #8398569 -
Flags: approval-mozilla-beta+
Attachment #8398569 -
Flags: approval-mozilla-aurora?
Attachment #8398569 -
Flags: approval-mozilla-aurora+
Comment 36•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2bcd64e5fe7 https://hg.mozilla.org/releases/mozilla-beta/rev/9933fa36efa5 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5db3a2eefc70 https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8a7035577141 https://hg.mozilla.org/releases/mozilla-esr24/rev/1c444c37109f
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [adv-main29+][adv-esr24.5+]
Comment 37•10 years ago
|
||
Marking [qa-] due to lack of test case. Feel free to provide one if you'd like verification. Thanks.
Whiteboard: [adv-main29+][adv-esr24.5+] → [adv-main29+][adv-esr24.5+][qa-]
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•