Last Comment Bug 754989 - Nuke dead cross-compartment wrappers during brain transplant
: Nuke dead cross-compartment wrappers during brain transplant
Status: RESOLVED FIXED
[js:p1:fx15][sg:critical][needs-branc...
: sec-critical
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-14 13:04 PDT by [PTO to Dec5] Bill McCloskey (:billm)
Modified: 2015-10-16 11:48 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
fixed
14+
fixed


Attachments
patch (11.04 KB, patch)
2012-05-14 13:04 PDT, [PTO to Dec5] Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review
patch for wider issue (15.09 KB, patch)
2012-05-14 17:17 PDT, [PTO to Dec5] Bill McCloskey (:billm)
bobbyholley: feedback-
Details | Diff | Splinter Review
patch v2 (13.21 KB, patch)
2012-05-21 10:28 PDT, [PTO to Dec5] Bill McCloskey (:billm)
bobbyholley: review+
Details | Diff | Splinter Review
patch for beta/esr10 (15.05 KB, patch)
2012-06-26 15:33 PDT, [PTO to Dec5] Bill McCloskey (:billm)
wmccloskey: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description [PTO to Dec5] Bill McCloskey (:billm) 2012-05-14 13:04:35 PDT
Created attachment 623782 [details] [diff] [review]
patch

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.
Comment 1 Luke Wagner [:luke] 2012-05-14 13:24:08 PDT
(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 2 Luke Wagner [:luke] 2012-05-14 13:29:11 PDT
Comment on attachment 623782 [details] [diff] [review]
patch

Oops, nm, W' was right.
Comment 3 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-14 17:17:08 PDT
Created attachment 623880 [details] [diff] [review]
patch for wider issue

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.
Comment 4 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-14 17:20:33 PDT
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.
Comment 5 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-14 17:27:47 PDT
One more thing: this is blocking incremental GC landing right now.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-14 20:55:33 PDT
Glad to see this stuff is useful ;-)
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-05-14 21:36:19 PDT
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.
Comment 8 Al Billings [:abillings] 2012-05-16 10:20:48 PDT
Bill,

Could you apply a security rating to the risk/exposure on this (see https://wiki.mozilla.org/Security_Severity_Ratings/Proposals)?
Comment 9 Daniel Veditz [:dveditz] 2012-05-17 13:19:55 PDT
How far back does this problem go? Does it affect the ESR-10 branch?
Comment 10 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-17 13:25:53 PDT
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).
Comment 11 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-21 10:28:59 PDT
Created attachment 625681 [details] [diff] [review]
patch v2

This patch folds together the previous two. This is pretty much what we talked about, Bobby.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-05-22 03:53:32 PDT
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.
Comment 13 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-22 14:57:15 PDT
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.
Comment 14 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-22 15:46:02 PDT
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.
Comment 16 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-23 10:21:52 PDT
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.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-05-23 10:29:04 PDT
(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...
Comment 18 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-23 10:34:07 PDT
This fixes a stupid mistake Ms2ger spotted:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c5e83e9289
Comment 20 Al Billings [:abillings] 2012-05-24 11:27:46 PDT
Is there any way to specifically verify this fix?
Comment 21 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-25 10:58:45 PDT
(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.
Comment 22 Daniel Veditz [:dveditz] 2012-06-21 17:24:12 PDT
(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?
Comment 23 [PTO to Dec5] Bill McCloskey (:billm) 2012-06-26 15:33:39 PDT
Created attachment 636905 [details] [diff] [review]
patch for beta/esr10

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
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-27 09:53:47 PDT
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.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-12 15:09:51 PDT
Is there something QA can do to verify this fix?

Note You need to log in before you can comment on or make changes to this bug.