1468579 - Sandboxes are not really protected by Xrays

Status: ASSIGNED

(Core :: XPConnect, defect, P2)

Description

[Oriol Brufau [:Oriol]]

Consider this:
  var s = Cu.Sandbox(null);

The sandbox has some kind of partial Xrays protection:
  Cu.waiveXrays(s) === s; // false
  Cu.isXrayWrapper(s.eval("({})")); // true

But the sandbox itself doesn't really have Xrays:
  Cu.isXrayWrapper(s); // false

For example, if some global property is an unprivileged getter, it will be executed:
  s.eval("Object.defineProperty(this, 'foo', {get(){ return 123 }})");
  s.foo; // 123

Not sure if this is by design, but seems potentially dangerous to me so just in case I'm filing this as security sensitive.

Comment 1

[Daniel Veditz [:dveditz]]

Blake: not sure you want to hear about wrappers, but with bobby on PTO maybe you can opine on whether the above is a problem or as intended. Otherwise can you suggest someone else who would be up to date on this?

Comment 2

[Bobby Holley (:bholley)]

Yeah, this is known:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/js/xpconnect/wrappers/XrayWrapper.cpp#117

The basic idea is that people like to be able to access JS state on sandbox globals, and there's a pretty clear understanding that the sandbox global's state is content-controlled.

The main drawback is that you can't easily depend on standard prototypes on the sandbox object. We could introduce a hybrid of object Xrays plus standard prototypes. Might not be too complicated to do but not a very high priority.

Comment 3

[Oriol Brufau [:Oriol]]

(In reply to Bobby Holley (:bholley) from comment #2)
> there's a pretty clear understanding that the sandbox global's
> state is content-controlled.

Well, I expected it to be protected by Xrays, just like a Window object.
Not doing so may make some common use cases easier, but it can create a security problem if some code assumes the sandbox will behave properly.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Oriol Brufau [:Oriol] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > there's a pretty clear understanding that the sandbox global's
> > state is content-controlled.
> 
> Well, I expected it to be protected by Xrays, just like a Window object.
> Not doing so may make some common use cases easier, but it can create a
> security problem if some code assumes the sandbox will behave properly.

The key distinction is that the Window object has a set of defined and specified behavior that people might rely on, whereas Sandboxes don't (modulo perhaps ES builtins, see comment 2). So for example, there's no standard sandbox.foo method or property that developers might rely on. The expected behavior from a sandbox is entirely dependent on what was evaluated in that sandbox.

We do have a form of Xray wrappers to pure JS objects (and arrays) which try to eliminate surprising behavior. But those are generally useful when people pass around objects as simple dictionaries, where they just want to access some properties and don't expect to run getters/setters/methods. Applying such XrayWrappers to Sandbox globals causes breakage because people do expect to be able to invoke methods in that case

Comment 5

[Daniel Veditz [:dveditz]]

Bobby: what do we do with this bug then? Is there something here you want to change (maybe abuse the "User Story" to summarize) or is this a worksforme/wontfix situation?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Bobby: what do we do with this bug then? Is there something here you want to
> change (maybe abuse the "User Story" to summarize) or is this a
> worksforme/wontfix situation?

This is a low priority (not security-sensitive) bug that I might take a patch for if the complexity was low, but that I wouldn't encourage anyone to work on.

Comment 7

[Rob Wu [:robwu]]

What needs to be done in order to support true Xray vision on sandboxes, at least for the case where the sandbox's prototype is a window object?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Rob Wu [:robwu] from comment #7)
> What needs to be done in order to support true Xray vision on sandboxes, at
> least for the case where the sandbox's prototype is a window object?

Can you motivate this a bit? What problem are you trying to solve?

Comment 9

[Rob Wu [:robwu]]

There are two uses in extension code (and in all cases the sandboxes' prototype is a content window):

1) Some extension API implementations for content scripts needs the standard built-in global instead of a possibly changed global - see bug 1456531

2) The new userScripts extension API (bug 1437864, updated in bug 1498739) involves two types of sandboxes:
   - User scripts are executed in the least privileged sandbox [1].
   - The other sandbox [2] lazily registers the APIs that are lazily available to the user script sandbox.

   When a user script invokes a registered API, the implementation (provided by the extension in the other sandbox) is called with the given parameters and the global of the userScript sandbox.
   Due to this bug, it is not possible for the more privileged sandbox to rely on standard global members of the userScript sandbox, and as a work-around [3] we have to mark some members as read-only. The downside of this work-around is that the it only applies to some (explicitly listed) globals, and that these constructors cannot be replaced / monkey-patched any more.

[1]: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/extensions/ExtensionContent.jsm#633-644
[2]: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/extensions/ExtensionContent.jsm#797-807
[3]: https://phabricator.services.mozilla.com/D11365

Comment 10

[Bobby Holley (:bholley)]

Ok, thanks. So it sounds like you want a few things:
* ES builtins should be reliable on the sandbox global.
* The prototype of the sandbox should reliably reflect the value used in sandboxPrototype.
* As a hopefully-automatic consequence of the above, builtins inherited from windows are reliable on the sandbox global.

I _think_ the implementation of that should be relatively straightforward. The first step would be to add a new XrayType for sandbox globals. ES builtins should be automatically resolved by the code in [1]. So we'd just need to setup up the ::getPrototype trap to understand sandboxPrototype, and return Object.prototype otherwise.

After that, we should do a try push and see how much stuff breaks. I'm guessing the answer might be "a lot", since there are probably a lot of things in the tree (i.e. tests) that assume that they can access properties on unprivileged sandbox globals without waiving Xrays. If it's manageable, we should just fix the consumers. If it's really unmanageable, we could consider adding an opt-in to SandboxOptions to get the new behavior.

All in all, should be fairly straightforward XPConnect work, unless we hit a snag. I don't have time to do it, but could mentor.

[1] https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/xpconnect/wrappers/XrayWrapper.cpp#1587

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #10)
> on unprivileged sandbox globals without waiving Xrays. If it's manageable,
> we should just fix the consumers. If it's really unmanageable, we could
> consider adding an opt-in to SandboxOptions to get the new behavior.

FWIW, I think the bar for "really unmanageable" should be pretty high. It's more obvious to opt out of xrays than to opt back into them and xrays are the default for all (I think) chrome->content interactions elsewhere in Gecko.

Comment 12

[Rob Wu [:robwu]]

I'm working on this, but initially for ES builtins only, and no involved plans for builtins inherit from sandboxPrototype.

I think that it is generally expected that properties/functions can be assigned to a sandbox, and forcing consumers to use sandbox.wrappedJSObject or exportFunction might be a bit too much.

Current WIP:
- property writing is forwarded to the sandbox
- property reading is only forwarded to the sandbox if it is not a built-in
- built-ins can be overwritten in the sandbox, but the public sandbox property is never visibly modified (even the built-in overwrite was done outside of the sandbox)

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&author=rob%40robwu.nl&selectedJob=211388527
There are some oranges. Some are caused by a mismatch in "this" because apparently DOM getters (e.g. document) do not like that this is set to a wrapper around the sandbox instead of a DOM window. Investigating...

Comment 13

[Bobby Holley (:bholley)]

(In reply to Rob Wu [:robwu] from comment #12)
> I'm working on this, but initially for ES builtins only, and no involved
> plans for builtins inherit from sandboxPrototype.
> 
> I think that it is generally expected that properties/functions can be
> assigned to a sandbox, and forcing consumers to use sandbox.wrappedJSObject
> or exportFunction might be a bit too much.
> 
> Current WIP:
> - property writing is forwarded to the sandbox
> - property reading is only forwarded to the sandbox if it is not a built-in
> - built-ins can be overwritten in the sandbox, but the public sandbox
> property is never visibly modified (even the built-in overwrite was done
> outside of the sandbox)

That seems logical, but I'm a bit worried about the complexity of implementing all of that. It seems like it might add a fair amount of footprint to XrayWrapper just for one type.

The semantics you're describing seem pretty similar to what we do for object and array xrays. What would happen if we just lied and treated Sandbox instances vanilla ES Object instances?

Comment 14

[Rob Wu [:robwu]]

(In reply to Bobby Holley (:bholley) from comment #13)
> The semantics you're describing seem pretty similar to what we do for object
> and array xrays.

JS Xrays have additional restrictions, such as disallowing non-value property descriptors and callable values.

> What would happen if we just lied and treated Sandbox
> instances vanilla ES Object instances?

That works, with some other changes.
- JSXrayTraits::createHolder must lie about the type (JSProto_Object instead of JSProto_Null).
- XrayWrapper<Base, Traits>::hasOwn must return true if the sandbox has the property.
- XrayWrapper<Base, Traits>::get should fall back to CCW::get if the property was initially not found.

The last two changes are needed because the sandbox already has special logic to make sure that DOM getters work, and that requires that we end up calling SandboxProxyHandler::get [1] instead of going through prototypes [2] (and the latter happens because XrayWrappers are constructed with hasPrototype=true).

[1] https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/js/xpconnect/src/Sandbox.cpp#803,811
[2] https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/js/src/proxy/Proxy.cpp#367,374,380

I'll attach a patch for review. Be mindful that lots of tests will fail due to the stricter Xray semantics: 
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c940cbdd4752c6011c7aaeb199a3b377cbed3c

Questions:
- Do we indeed want these strict Xray semantics?
- If so, can it be opt-in via a new sandbox option?

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1468579 - Support Xrays for built-ins on sandbox globals

Comment 16

[Rob Wu [:robwu]]

See questions of comment 14.

(In reply to Rob Wu [:robwu] from comment #14)
> JS Xrays have additional restrictions, such as disallowing non-value
> property descriptors and callable values.

And you probably know already, but cross-origin properties are also disallowed. Many tests in js/xpconnect/tests/unit/ break for this reason.

Comment 17

[Bobby Holley (:bholley)]

Thanks for writing that up!

My overriding concern here is to minimize added complexity. That means trying to avoid adding too much new code, and trying to avoid defining an entirely new Xray contract for sandbox globals. Those are the two motivating factors behind my suggestion to piggy-back off the already well-defined and carefully-implemented object Xrays, and the patch you posted seems to do pretty well along both of those axes, relative to the try-run patch in comment 12. So I think this direction is promising, but am not wedded to it if we run into trouble.

I think we shouldn't worry too much about the test failures. Most of them are test-only code, and we can make the new-behavior opt-out (disableSandboxGlobalXrays in SandboxOptions), and sprinkle that across the affected tests. We should avoid involving wantXrays, since that's going away (see bug 1491490).

I think the new semantics are reasonable, given that we already have those semantics for Object/Array Xrays, we've thought a lot about the security properties already, and they're well-documented [1]. Some consumers may well want the old semantics, but that's what the opt-out is for.

So I'd be curious to see if we can get a green try run with the current patch by implementing the opt-out and sprinkling it across a few tests. If so, that's probably the right thing. I'll add a few more comments on the patch.


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision#Xray_semantics_for_Object_and_Array

Comment 18

[Rob Wu [:robwu]]

I've checked what needs to be done to support sandboxes that wrap DOM windows. I think that the implementation could be simplified (and fix some bugs along the way), by removing the two wrappers from Sandbox.cpp, and resolving the sandbox's original proto in dom/bindings/BindingUtils.h.

Longer story:

Sandboxes have a sandboxPrototype, which is supposed to allow sandboxes to pretend that their global is like a DOM window.
Initially, sandboxPrototype could be just a window, but this changed over the course of history:

- bug 726949 [1] - Introduces SandboxProxyHandler, whose getPropertyDescriptor trap replaces getters/setters/function-values with bound functions.
  (to fix the regression caused by bug 622301).

- bug 771429 [2] - Introduces SandboxCallableProxyHandler (a proxy with a "call" trap) to replace bound functions.
  If "this" is set to the sandbox global, then the sandbox proto (e.g. DOM window) is used instead.
  If "this" is undefined, let the callee decide on "this" (if the function is from the DOM window proto, the DOM window becomes "this").

- bug 658909 [3] - If the proxy is a XrayWrapper, compute "this".
  If the proxy is a XrayWrapper (e.g. DOM Window) and "this" is undefined, compute the "this".
  Since the function resides in the sandbox, the "this" becomes the sandbox global.
  Combined with the previous patch, the "this" becomes the sandbox proto (e.g. DOM window).

- bug 1430164 [4] - Stop wrapping DOM constructors.
  
This special handling of "this" is to make sure that inherited methods are invoked on the original sandbox proto (e.g. DOM window). Without this, accessing DOM properties will throw an error, e.g.:

> TypeError: 'document' called on an object that does not implement interface Window

The above validation is performed at the binding level, by mozilla::dom::binding_detail::GenericGetter/GenericSetter/GenericMethod, if "this" does not have the expected class.
The "this" is resolved via UnwrapObjectInternal [4], and uses js::CheckedUnwrap to throw away any wrappers/proxies.


The key points of the above are:
- The wrapper machinery (SandboxCallableProxyHandler) primarily exists to support DOM property descriptors.
- The DOM bindings templates unwrap any wrappers before accessing the target object.


Since the bindings are auto-generated, I wonder whether it is feasible to remove the callable wrapper and remap "this" at the DOM binding level instead. This could be done by climbing the prototype chain once (or using a private slot, whatever) if the target is a Sandbox.
I expect that everything still works when we lie about "this", because both endpoints already support the required capabilities:
- The sandbox already uses wrappers to lie about "this".
- The DOM binding template already unwraps proxies, so unwrapping once more (the sandbox) should not be a problem either.

We must ensure that the DOM window is in the right compartment. Since a DOM window can only be a sandboxPrototype if the sandbox's principal subsumes the DOM window's, an (existing instance of) CrossCompartmentWrapper (or any subclass) should work.


The above proposal does not only make the implementation of this feature easier, it also fixes some issues:


BUG     : re-generated wrappers
STR     : Cu.evalInSandbox('close === close', Cu.Sandbox(window, {sandboxPrototype: window}));
Expected: true.
Actual  : false.
Reason  : This is because functions wrappers are re-generated on each access (variant of bug 1430164).


BUG     : sandbox methods cannot be invoked.
STR     : Cu.Sandbox(window,{sandboxPrototype:window}).focus()
Expected: Should succeed.
Actual  : TypeError: 'focus' called on an object that does not implement interface Window
Reason  :
SandboxProxyHandler::getPropertyDescriptor (used by the "get" trap) wraps the actual DOM getter in a proxy [6].
Sandboxes are accessed over a cross-compartment wrapper [7] [8].
CrossCompartmentWrapper::get wraps the returned object (from the sandbox's compartment to the caller's compartment).
That ends up at Compartment::getNonWrapperObjectForCurrentCompartment (via Compartment::wrap). This method fully unwraps the object before rewrapping [9].
So the SandboxCallableProxyHandler proxy is stripped, and consequently this error arises.

The last bug shows that proxies cannot be used to decorate functionality across compartments (unless some new flags are introduced to control this behavior).

Besides fixing these bugs, the performance of accessing sandbox globals might also improve because there are fewer proxies and no function wrapping. DOM bindings is likely hotter code, so I must be careful there...


I'm going to give the above a try, and see if anything breaks.


[1] https://searchfox.org/mozilla-central/diff/89f614d0599c087f61896225ce869c17cee4211a/js/xpconnect/src/XPCComponents.cpp#3069
[2] https://searchfox.org/mozilla-central/diff/83dc6c87d53281c18b2f2a1742d532a6a65230af/js/xpconnect/src/XPCComponents.cpp#3062
[3] https://searchfox.org/mozilla-central/diff/98277c3a3f52cff1a3ebe54d8d14756ca5c47e97/js/xpconnect/src/XPCComponents.cpp#3050
[4] https://searchfox.org/mozilla-central/diff/4b3ce3fd3cd421d56192697ab9317fb485018845/js/xpconnect/src/Sandbox.cpp#773

[5] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/dom/bindings/BindingUtils.h#233,254-255

[6] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/js/xpconnect/src/Sandbox.cpp#672,683,696,748
[7] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/js/xpconnect/wrappers/XrayWrapper.cpp#122-124
[7] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/js/xpconnect/wrappers/WrapperFactory.cpp#399-401
[9] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/js/src/proxy/CrossCompartmentWrapper.cpp#225,239
[10] https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/js/src/vm/Compartment.cpp#200,226

Comment 19

[Rob Wu [:robwu]]

I'm NOT going to pursue the idea from comment 18 (moving the "this" rebinding to DOM bindings) after all, because the DOM binding layer is not the right place to cross compartments.

The two bugs mentioned in the comment are still valid bugs; and if they are not addressed by the patch that lands in this bug, then I will file follow-up bugs.

Comment 20

[Rob Wu [:robwu]]

> trying to avoid defining an entirely new Xray contract for sandbox globals

This is difficult to attain.
I'm going to AVOID JS object Xray semantics for sandboxes, because the resulting xrayed object would be difficult to comprehend. For DOM objects we have "the state as defined by C++". In case of sandboxes, the above next of properties looks reasonable. Including any JS would make the resulting object dirtier and more difficult to maintain **.

1. Built-in ES globals.
2. wantGlobalProperties globals.
3. .wrappedJSObject = underlying sandbox through CrossCompartmentWrapper but WITHOUT transitive Xrays.
  (sandbox.wrappedJSObject.window.foo should read "foo" in the sandbox's compartment, not in the window's).
4. Anything on the native sandboxPrototype (particularly when it is a DOM window; with property getter/setter/method somehow bound to window).

1 and 4 can reasonably easily be fitted in JS Xrays.
2 is not needed for my use cases (comment 12, because export globals are all inherit from the window). But if we want to enable xrays on sandboxes by default, then we need to keep track of the exported globals (e.g. in CompartmentPrivate).
3 could be special cased in the .wrappedJSObject getter. But if we want it to play nicely with waiveXrays/unwaiveXrays, then WrapperFactory::CreateXrayWaiver only needs to remove the XrayWrapper over a sandbox, and use a transparent CCW, instead of a transitively waiving wrapper).

** In sandboxes, the global scope consists of global variables and properties of "window". In Xrayed JS objects, only own value properties are visible (so excluding prototypes). These two are incompatible. Besides the implementation complexity, including arbitrary properties from the global scope also makes the behavior of a xrayed global less predictable.

Comment 21

[Bobby Holley (:bholley)]

That all sounds straightforward, and pretty much what I had in mind in comment 10. Comment 12 introduced more complex requirements, so if we're fine dropping those, things get simpler.

It seems like the main remaining complexity is (4), right? Presumably we need to replicate the behavior of SandboxProxyHandler or somesuch?

Comment 22

[Rob Wu [:robwu]]

I've temporarily paused the work because I need to rewrite the patches (comment 20), and the userScripts API launch has been deferred. I'll resume the work once a target release is set (bug 1514809).

Comment 23

[Rob Wu [:robwu]]

At this point it is too late to change in 68, so this does not block the launch of the userScripts API (bug 1514809). This work is scheduled for the next release (69).

Comment 24

[sgl4kn]

This work is scheduled for the next release (69)
You see any chance for 70 or is this starting then from nightly onwards?

Comment 25

[Rob Wu [:robwu]]

I haven't worked on this bug yet. I'll update the bug if there is any news.