Closed Bug 980537 Opened 10 years ago Closed 10 years ago

mozJSComponentLoader::mThisObjects is not properly rooted

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 + fixed
firefox30 + fixed
firefox31 + fixed
firefox-esr24 --- fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jandem, Assigned: bholley)

References

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main29+][adv-esr24.5+][qa-])

Attachments

(3 files)

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)
(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.)
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)
Ok, then let's let terrence fix this up the way he wants for GGC.
Flags: needinfo?(terrence)
Blocks: 950653
Summary: Is mozJSComponentLoader::mThisObjects properly rooted? → mozJSComponentLoader::mThisObjects is not properly rooted
Looks like this landed in Fx19.  So at least b2g18 is unaffected.
I'm 99% sure we backported it to b2g 18.
Hah, right you are...
blocking-b2g: --- → 1.3?
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.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8387946 - Flags: review?(sphink)
blocking-b2g: 1.3? → 1.3+
(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?
(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)
(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 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+
(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)
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 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+
I need to know if this has been in any B2G releases before landing here. Should I open a cover bug?
Flags: needinfo?(khuey)
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.
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?
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.
(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.
Terrence, what's the status here? Do we understand how to move forward here?
Flags: needinfo?(terrence)
(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)
/me will have a look at this
Assignee: nobody → bobbyholley
(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
Seems like the landing of bug 967516 caused a new spike here.
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.
Depends on: 989373
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?
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+
(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
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?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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+
Whiteboard: [adv-main29+][adv-esr24.5+]
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-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: