Closed Bug 754989 Opened 12 years ago Closed 12 years ago

Nuke dead cross-compartment wrappers during brain transplant

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 --- wontfix
firefox14 + fixed
firefox15 + fixed
firefox16 --- fixed
firefox-esr10 14+ fixed

People

(Reporter: billm, Assigned: billm)

Details

(Keywords: sec-critical, Whiteboard: [js:p1:fx15][sg:critical][needs-branches][advisory-tracking+][qa?])

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
The basic way we do cross-compartment brain transplants is as follows:

For each wrapper W that points to the old object, wrap the new object in W's compartment (which returns a new wrapper W'), and swap the guts of W and W'.

When we're done with this, W' now holds the guts of W. The problem is that W' is still a cross-compartment wrapper, and it's no longer in the cross-compartment wrapper map. Normally everything works okay, because W' is unreachable and we'll never trace it again.

However, the conservative stack scanner could cause us to trace it again. And in that case, we can get into a lot of trouble. If we trace it after the thing it points to was collected in a compartment GC, then we're tracing into bad memory.

I found this because incremental GC seems to find a lot more of these dead wrappers and trace through them. I'm guessing they're getting caught in a write barrier somewhere.

I took advantage of some code that Kyle wrote to kill off a cross-compartment wrapper. I'm pretty sure that the nuke call for origobj is unnecessary, since I haven't ever seen it be a cross-compartment wrapper. But unless you know better, I figure it can't hurt.
Attachment #623782 - Flags: review?(luke)
(In reply to Bill McCloskey (:billm) from comment #0)
> When we're done with this, W' now holds the guts of W. The problem is that
> W' is still a cross-compartment wrapper, and it's no longer in the
> cross-compartment wrapper map.

Do you mean "The problem is that W is still a cross-compartment wrapper" ?
Comment on attachment 623782 [details] [diff] [review]
patch

Oops, nm, W' was right.
Attachment #623782 - Flags: review?(luke) → review+
Attached patch patch for wider issue (obsolete) — Splinter Review
This assertion uncovered another problem, related to the double wrapping of location objects. Let's say we need a cross-compartment wrapper for a location object L. First we call GetSameCompartmentSecurityWrapper on the location object, which gives us some proxy object P. Then we wrap that with a cross-compartment wrapper W. Crucially, when we put this wrapper in the cross-compartment map, we add an entry for P -> W (not L -> W).

Now let's say we call wrap on the location object again, for the same compartment. We check the cross-compartment map to see if it's already wrapped, but we look up with L as the key. So we don't find anything. Then we go through the wrap process again, getting the same proxy P and a new wrapper W'. We add this entry, P -> W', to the cross-compartment map. It replaces the old P -> W entry.

Now we're in a bad situation. If the new wrapper W' dies, then we remove the P -> W' entry from the map. We still have the original wrapper W, and there's no cross-compartment wrapper map entry for it. So a compartment GC may collect P and leave W as a dangling wrapper. The next time we collect it's compartment, we may crash.

I made a patch that works around this problem. It's pretty gross. Bobby, can you think of a better way? I don't understand this code well at all.
Attachment #623880 - Flags: feedback?(bobbyholley+bmo)
Also, Bobby, forget all the nuking stuff in the patch I just posted. I accidentally folded the patch for you together with the one Luke reviewed.
One more thing: this is blocking incremental GC landing right now.
Glad to see this stuff is useful ;-)
Comment on attachment 623880 [details] [diff] [review]
patch for wider issue

(In reply to Bill McCloskey (:billm) from comment #3)
> This assertion uncovered another problem, related to the double wrapping of
> location objects. Let's say we need a cross-compartment wrapper for a
> location object L. First we call GetSameCompartmentSecurityWrapper on the
> location object, which gives us some proxy object P. Then we wrap that with
> a cross-compartment wrapper W. Crucially, when we put this wrapper in the
> cross-compartment map, we add an entry for P -> W (not L -> W).

We do? I don't see that happening, but if so, that's pretty wrong, I think. L is the identity object, so it should be the key in the cross-compartment wrapper map. The key we use is the object that we get back from preWrap, and we don't do the funny CCW-to-LW-for-same-origin trick until the wrap callback.

Funnily enough, I just touched this code. I refactored it a bit and added comments in bug 753277, which might help to understand it a bit better. More importantly, I fixed a few bugs, which might have been related to the issues you're seeing (bug 754044 bug 754311). Those bugs both just landed on inbound, and should be on central quite soon. Can you still reproduce the issue?

Anyway, this workaround isn't the way we want to go about things, and we should get to the bottom of this one way or another. This code is all fresh in my head, so I can probably be of good help here.
Attachment #623880 - Flags: feedback?(bobbyholley+bmo) → feedback-
Bill,

Could you apply a security rating to the risk/exposure on this (see https://wiki.mozilla.org/Security_Severity_Ratings/Proposals)?
Whiteboard: [js:p1:fx15][sg:critical]
How far back does this problem go? Does it affect the ESR-10 branch?
I ended up dealing with two problems in this bug. One of them goes back to ESR. The other one is more recent--a regression from compartment-per-global (or maybe one of the precursor patches).
Attached patch patch v2Splinter Review
This patch folds together the previous two. This is pretty much what we talked about, Bobby.
Attachment #623782 - Attachment is obsolete: true
Attachment #623880 - Attachment is obsolete: true
Attachment #625681 - Flags: review?(bobbyholley+bmo)
Comment on attachment 625681 [details] [diff] [review]
patch v2


>     } else if (WrapperMap::Ptr p = map.lookup(origv)) {
>         // There might already be a wrapper for the original object in
>         // the new compartment. If there is, we use its identity and swap
>         // in the contents of |target|.
>         newIdentity = &p->value.toObject();
>         map.remove(p);
>+        NukeCrossCompartmentWrapper(newIdentity);
>         if (!newIdentity->swap(cx, target))
>             return NULL;

It would be much clearer IMO to nuke |target| after the swap, rather than nuking |newIdentity| before the swap. Also, it would be good to have a comment along the lines of '|target| now posses the old guts that are going away, but we want to make sure they're never traced by the stack scanner. Neuter it'.

Any reason not to do it that way?

>@@ -1635,16 +1637,20 @@ RemapWrappers(JSContext *cx, JSObject *o
> 
>     for (Value *begin = toTransplant.begin(), *end = toTransplant.end(); begin != end; ++begin) {
>         JSObject *wobj = &begin->toObject();
>         JSCompartment *wcompartment = wobj->compartment();
>         WrapperMap &pmap = wcompartment->crossCompartmentWrappers;
>         JS_ASSERT(pmap.lookup(origv));
>         pmap.remove(origv);
> 
>+        // This wrapper will soon be dead. Overwrite it so it doesn't contain

Nit: I think "Neuter" is more clear than "Overwrite" here.

>+        // any stale values for the GC to find.
>+        NukeCrossCompartmentWrapper(wobj);
>+
>         // First, we wrap it in the new compartment. This will return
>         // a new wrapper.
>         AutoCompartment ac(cx, wobj);
>         JSObject *tobj = target;
>         if (!ac.enter() || !wcompartment->wrap(cx, &tobj))
>             return false;
> 
>         // Now, because we need to maintain object identity, we do a

Similarly, can we Nuke |tobj| post-swap rather than nuking |wobj| pre-swap? Nuking something and then using it extensively is really confusing IMO.



>@@ -1671,31 +1677,33 @@ RemapWrappers(JSContext *cx, JSObject *o
> JS_FRIEND_API(JSObject *)
> js_TransplantObjectWithWrapper(JSContext *cx,
>                                JSObject *origobj,
>                                JSObject *origwrapper,
>                                JSObject *targetobj,
>                                JSObject *targetwrapper)
> {
>     AssertNoGC(cx);
>+    JS_ASSERT(!IsCrossCompartmentWrapper(origwrapper));

Might as well assert the same about targetWrapper here, since otherwise the implication is that it might be a CCW. In fact, might as well assert it for all four inputs.

>     // There might already be a wrapper for the original object in the new
>     // compartment.
>     if (WrapperMap::Ptr p = map.lookup(origv)) {
>         // There is. Make the existing cross-compartment wrapper a same-
>         // compartment wrapper.
>         newWrapper = &p->value.toObject();
>+        NukeCrossCompartmentWrapper(newWrapper);
>         map.remove(p);
>         if (!newWrapper->swap(cx, targetwrapper))
>             return NULL;

Same issue here - I'd prefer to nuke |targetwrapper| post-swap.


>+        } else {
>+            JS_ASSERT(v.isString());
>+            Value v = e.front().key;
>+            MarkValueRoot(trc, &v, "cross-compartment wrapper");
>+            JS_ASSERT(v == e.front().key);
>+        }

A comment explaining why we do this differently for strings is in order, here.



>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>--- a/js/src/jsproxy.cpp
>+++ b/js/src/jsproxy.cpp
>@@ -1433,16 +1433,46 @@ static JSBool
> proxy_DeleteSpecial(JSContext *cx, JSObject *obj, SpecialId sid, Value *rval, JSBool strict)
> {
>     return proxy_DeleteGeneric(cx, obj, SPECIALID_TO_JSID(sid), rval, strict);
> }
> 
> static void
> proxy_TraceObject(JSTracer *trc, JSObject *obj)
> {
>+#ifdef DEBUG
>+    Value priv = GetProxyPrivate(obj);
>+    if (priv.isObject()) {
>+        Cell *cell = (Cell *)priv.toGCThing();

Proxy privates can be all sorts of things (XPCOM objects, for example). I grok that this is handled by the isObject check, but it would be clearer IMO to check obj->isWrapper().

>+        if (cell->compartment() != obj->compartment()) {
>+            /*
>+             * Assert that this proxy is tracked in the wrapper map. Normally,
>+             * the wrapped object will be the key in the wrapper map. However,
>+             * sometimes when wrapping Location objects, the wrapped object is
>+             * not the key. In that case, we must iterate through the map to
>+             * look for an entry.
>+             */

Why not just js::UnwrapObject the referent before asserting? That would handle both of these cases in a much cleaner fashion.

Can we separate the Nuking stuff and the wrapping changes into two separate patches? I don't see any obvious way they depend on each other, and it would be very useful for regression finding, archaeology, and backporting. No need to flag for re-review.
Attachment #625681 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2d73c14150
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee26f29f4b6

Bobby, the idea of using UnwrapObject in the proxy trace assert is good. However, it exposed additional problems. I'd like to deal with them in a separate bug, though. I checked in the current patches, but without any asserts at all in proxy_TraceObject.
Target Milestone: --- → mozilla15
Actually, I was able to modify the assertion to make it pass, I think. Assuming it's green on try, I'll just land it in this bug.
Whiteboard: [js:p1:fx15][sg:critical] → [js:p1:fx15][sg:critical][leave-open]
Here's the checkin for the assertion:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da890c10b4ac

Using UnwrapObject by itself is not enough. I found the following scenario:

Compartment C1:

    Proxy P1 (a CrossOriginWrapper) points to X
    Proxy P2 (a xpc::XrayWrapper<js::CrossCompartmentWrapper,xpc::XPCWrappedNativeXrayTraits>) points to Y

Compartment C2:

    Proxy X (a WaiveXrayWrapperWrapper) points to Y
    Proxy Y (an nsOuterWindowProxy) points to a Window

In this case, the wrapper map for C1 contains [X->P1, Y->P2]. So if we always call UnwrapObject when doing the assertion, we'll fail when tracing through P1: we'll unwrap X to Y and then try to assert that [Y->P1] is in the wrapper map, which it isn't.
Whiteboard: [js:p1:fx15][sg:critical][leave-open] → [js:p1:fx15][sg:critical]
(In reply to Bill McCloskey (:billm) from comment #16)
> In this case, the wrapper map for C1 contains [X->P1, Y->P2]. So if we
> always call UnwrapObject when doing the assertion, we'll fail when tracing
> through P1: we'll unwrap X to Y and then try to assert that [Y->P1] is in
> the wrapper map, which it isn't.

Ah yes, right. The correct way to check for this is to look for WAIVE_XRAY_WRAPPER_FLAG in the call to js::Unwrap. We really need a better way to specify which wrappers we want to unwrap and which ones we don't...
Is there any way to specifically verify this fix?
(In reply to Al Billings [:abillings] from comment #20)
> Is there any way to specifically verify this fix?

I don't know enough about wrappers to write a test case. So probably not.
(In reply to Bill McCloskey (:billm) from comment #10)
> I ended up dealing with two problems in this bug. One of them goes back to
> ESR. The other one is more recent--a regression from compartment-per-global
> (or maybe one of the precursor patches).

Is the older problem a security vulnerability on its own? If so how severe, and how easy would it be to backport to Beta (Fx14) and ESR?
I rebased this for beta and esr.

[Approval Request Comment]
Regression caused by (bug #): Compartments (FF4)
User impact if declined: Small likelihood of crashes/exploitation.
Testing completed (on m-c, etc.): On m-c, aurora.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None
Attachment #636905 - Flags: review+
Attachment #636905 - Flags: approval-mozilla-release?
Attachment #636905 - Flags: approval-mozilla-beta?
Comment on attachment 636905 [details] [diff] [review]
patch for beta/esr10

I think you meant to flag for esr10, not release, so approving for beta and esr10.
Attachment #636905 - Flags: approval-mozilla-release?
Attachment #636905 - Flags: approval-mozilla-esr10+
Attachment #636905 - Flags: approval-mozilla-beta?
Attachment #636905 - Flags: approval-mozilla-beta+
Whiteboard: [js:p1:fx15][sg:critical] → [js:p1:fx15][sg:critical][needs-branches]
Whiteboard: [js:p1:fx15][sg:critical][needs-branches] → [js:p1:fx15][sg:critical][needs-branches][advisory-tracking+]
Is there something QA can do to verify this fix?
Whiteboard: [js:p1:fx15][sg:critical][needs-branches][advisory-tracking+] → [js:p1:fx15][sg:critical][needs-branches][advisory-tracking+][qa?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.