742444 - Clear Xray waivers when passing object references across origins

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bobby Holley (:bholley)]

Xray waivers are a separate identity (just a transparent wrapper) in their home compartment. When chrome encounters one, it knows to waive its normal Xray vision and use a transparent CrossCompartmentWrapper (to the waiver).

However, chrome code using waivers can sometimes hand one of these things to content. This doesn't do anything insecure, since the code that actually selects the CrossCompartmentWrapper lives under |if (targetIsChrome)|. But it can confuse identity checks.

We should be careful and future-thinking here, since things like jetpack use Xray wrappers for same-compartment access, and probably should also be able to use waivers (though they currently don't). We should revisit this after compartment-per-global settles.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #0)
> We should be careful and future-thinking here, since things like jetpack use
> Xray wrappers for same-compartment access

err, "same-origin access"

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=791ef72798f4

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8c3463bb5126

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Rejigger unsafeWindow definition to not rely on passing waivers across origins. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Pass the old wrapper or value to the prewrap callback instead of its flags. v1

We need this so that we can reason about the origin of the wrapper that
previously had a waiver and decide whether or not to extend it.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Only propagate waivers between same-origin compartments. v1

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8459381 [details] [diff] [review]
Part 3 - Only propagate waivers between same-origin compartments. v1

Review of attachment 8459381 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/tests/unit/test_bug742444.js
@@ +8,5 @@
> +  sb1B.waived = Cu.waiveXrays(obj);
> +  sb2.obj = obj;
> +  sb2.waived = Cu.waiveXrays(obj);
> +  do_check_true(Cu.evalInSandbox('obj === waived', sb1B));
> +  do_check_true(Cu.evalInSandbox('obj === waived', sb2));

You're testing the cross origin case twice, but not the same origin case when the waiver is preserved. Is that intentional?

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +131,5 @@
> +inline bool
> +ShouldWaiveXray(JSContext *cx, JSObject *originalObj)
> +{
> +    unsigned flags;
> +    (void) js::UncheckedUnwrap(originalObj, /* stopAtOuter = */ true, &flags);

I personally prefer a short comment over using (void)... it's confusing to some people what is its intention. It's just a way to say out loud that the return value is being ignored, right?

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8459380 [details] [diff] [review]
Part 2 - Pass the old wrapper or value to the prewrap callback instead of its flags. v1

Review of attachment 8459380 [details] [diff] [review]:
-----------------------------------------------------------------

I have to pass this one. Partially because in theory I should not be allowed to review this part since it's js, but even more because I don't feel comfortable with this optimisation... Could you ask someone else to review the js bits?

::: js/src/jscompartment.cpp
@@ +397,5 @@
> +    //
> +    // Note - we carefully hold a non-rooted reference to the original value
> +    // so that we can pass it to preWrap without incurring a performance cost.
> +    JSObject *objectPassedToWrap = obj;
> +    obj.set(UncheckedUnwrap(obj, /* stopAtOuter = */ true));

Right in the next line we lose the only rooted reference to the original object in this scope. Or is it not how rooting works? I'm sure it can be explained why is that object still rooted somewhere, but I cannot. And I'm not sure that this optimization really worth it. Rooting should be super cheap since it's just adding a pointer to a linked list on the stack, no?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> You're testing the cross origin case twice, but not the same origin case
> when the waiver is preserved. Is that intentional?

It was intentional (one same-origin wantXrays, one cross-origin), since I figured that 'waiver is preserved' case was already implicitly tested all over the place. But I'll add a test for it anyway.

> > +    (void) js::UncheckedUnwrap(originalObj, /* stopAtOuter = */ true, &flags);
> 
> I personally prefer a short comment over using (void)... it's confusing to
> some people what is its intention. It's just a way to say out loud that the
> return value is being ignored, right?

It also tells the compiler (and certain static analysis tools like lint) not to emit a warning about an unused return value.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Right in the next line we lose the only rooted reference to the original
> object in this scope.

The reason this is safe is that there is nothing between this point and the last usage of |objectPassedToWrap| that can GC (and we have static analysis that can ensure that).

> Or is it not how rooting works?

Yes, but it's even more strict than that - everything that's used across a GC needs to be either Rooted or a Handle, because GGC can move the object even if it's rooted elsewhere.

> And I'm
> not sure that this optimization really worth it. Rooting should be super
> cheap since it's just adding a pointer to a linked list on the stack, no?

That's a fair point. Wrapping is generally very hot, but the hottest parts happen before we get to the Object-valued overload of ::wrap.

It probably doesn't matter much, and really the most important consideration is having your review on this patch (since it's really all about wrapping, which is XPConnect stuff). I'll remove the optimization and reflag you. :-)

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 2 - Pass the old wrapper or value to the prewrap callback instead of its flags. v2

Comment 12

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8460594 [details] [diff] [review]
Part 2 - Pass the old wrapper or value to the prewrap callback instead of its flags. v2

Review of attachment 8460594 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I'll sleep a lot better r+ing this one than the previous version :) Also, I'm actually pretty excited to have a reference to the original object in the wrapper factory, something I was always missing from there...

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77ba979e76af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd56605c08f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fa85777c40

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77ba979e76af
https://hg.mozilla.org/mozilla-central/rev/cd56605c08f6
https://hg.mozilla.org/mozilla-central/rev/d1fa85777c40

Comment 15

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0556abfa0bdb409d1d123528a9499ada7a4107a4
Bug 742444 - Rejigger unsafeWindow definition to not rely on passing waivers across origins. r=gabor