"apply" broken between same-origin compartments with asymmetric wantXrays

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: simon.lindholm10, Assigned: bholley)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

The following, run from a Chrome scratchpad:

w = content;
var f = Cu.evalInSandbox('x => typeof x', Cu.Sandbox(w));
w.wrappedJSObject.f = f;
w.eval('[f.apply(null, [alert]), f.call(null, alert), f(alert)]')

returns ["undefined", function", "function"], which is surprising: `f.call(a, b)` and `f.apply(a, [b])` are expected to behave the same.

I guess the issue is that "alert" is a function-valued property (with name "0") on the argument passed to "apply", and thus is Xray-ed away and never reaches f.

(Related to bug 1032457 and bug 1031336. We hit this in Firebug, see https://code.google.com/p/fbug/issues/detail?id=7580.)
Assignee: nobody → bobbyholley
The basic problem in the testcase is that one compartment requests same-origin
Xrays via wantXrays=true (the default for Sandboxes) while the other does not.
The current code only considers the wantXrays flag of the compartment performing
the access, so we end up in a situation where we have same-origin compartments,
but Xray in one direction and Transparent in the other.

This is a problem for crossCompartmentFunction.apply(null, [arg]). If both
globals get transparent wrappers, there's obviously no problem. And if both
globals get XrayWrappers, then the |apply| happens on the XrayWrapper of the
function in the caller's compartment. So the Array is unpacked in the caller's
compartment, and again we have no problem.

But if the caller gets Transparent and the callee gets Xrays, then we end up
invoking |apply| from the callee's side, which then gets an XrayWrapper to the
array. This XrayWrapper may do surprising things, leading to the odd situation
in the testcase.

Same-origin Xrays are kind of broken anyway, but I don't think we'll ever be
able to get rid of them. So the most sensible thing to do is probably to honor
the flag (if set) from either compartment. This patch does that.
Attachment #8450695 - Flags: review?(gkrizsanits)
Summary: "apply" on exported functions should not convert functions to undefined → "apply" broken between same-origin compartments with asymmetric wantXrays
In Jetpack in case of a panel is part of the add-on, so we trust it, we create the related content-script with same origin principal (instead of nsEP) and inject the sandbox (with the name 'addon') to its scope. With stuff like postMessage on it for enabling direct communication etc. I guess now you will have to waive xray before the injection to fix the test. (I _think_ here: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#201)
Comment on attachment 8450695 [details] [diff] [review]
Honor the wantXrays of both sides of the membrane when computing same-origin wrappers. v1

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

I cancel this review for now until you figure something for the SDK issue, since I don't think we can waive xrays from content side... :(
Attachment #8450695 - Flags: review?(gkrizsanits) → review-
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> In Jetpack in case of a panel is part of the add-on, so we trust it, we
> create the related content-script with same origin principal (instead of
> nsEP) and inject the sandbox (with the name 'addon') to its scope.

It seems like we shouldn't be using wantXrays at all in that case, right?

Try push with that change:

https://tbpl.mozilla.org/?tree=Try&rev=ca23b75756d3
Comment on attachment 8452114 [details] [diff] [review]
Don't use wantXrays for requiresAddonGlobal sandboxes. v1

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

I was thinking that we might not need a sandbox at all in this case, but probably this is the simplest and cleanest solution.
Attachment #8452114 - Flags: review?(gkrizsanits) → review+
Attachment #8450695 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/61c1236e6a36
https://hg.mozilla.org/mozilla-central/rev/bb54fa82e9d2
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b54c63d3a4c3219cb9150cc6c3c2838655e9410d
Bug 1034262 - Don't use wantXrays for requiresAddonGlobal sandboxes. r=gabor
You need to log in before you can comment on or make changes to this bug.