If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clear Xray waivers when passing object references across origins

RESOLVED FIXED in mozilla34

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Depends on: 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
(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"
Depends on: 866823
https://tbpl.mozilla.org/?tree=Try&rev=791ef72798f4
https://tbpl.mozilla.org/?tree=Try&rev=8c3463bb5126
Summary: Strip waivers for compartments that can't use them → Clear Xray waivers when passing object references across origins
Created attachment 8459379 [details] [diff] [review]
Part 1 - Rejigger unsafeWindow definition to not rely on passing waivers across origins. v1
Attachment #8459379 - Flags: review?(gkrizsanits)
Created attachment 8459380 [details] [diff] [review]
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.
Attachment #8459380 - Flags: review?(gkrizsanits)
Created attachment 8459381 [details] [diff] [review]
Part 3 - Only propagate waivers between same-origin compartments. v1
Attachment #8459381 - Flags: review?(gkrizsanits)
Attachment #8459379 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8459381 - Flags: review?(gkrizsanits) → review+
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?
Attachment #8459380 - Flags: review?(gkrizsanits)
(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.
(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. :-)
Created attachment 8460594 [details] [diff] [review]
Part 2 - Pass the old wrapper or value to the prewrap callback instead of its flags. v2
Attachment #8459380 - Attachment is obsolete: true
Attachment #8460594 - Flags: review?(gkrizsanits)
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...
Attachment #8460594 - Flags: review?(gkrizsanits) → review+
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
https://hg.mozilla.org/mozilla-central/rev/77ba979e76af
https://hg.mozilla.org/mozilla-central/rev/cd56605c08f6
https://hg.mozilla.org/mozilla-central/rev/d1fa85777c40
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1043958

Comment 15

3 years ago
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
You need to log in before you can comment on or make changes to this bug.