1504660 - a instanceof b triggers proxy traps if a and b are in the same global

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[Rob Wu [:robwu]]

When "LHS instanceof RHS" is used, and LHS is a Proxy, then the proxy traps are triggered if both LHS and RHS are in the same global, even if the LHS and RHS are xray wrappers. I did not expect this, since the caller is the system principal and these objects are XrayWrappers.

STR:
1. Open the global JS console and run:

let sand = Cu.Sandbox(null);
let retval = sand.eval(`new Proxy({}, {getPrototypeOf() { throw new Error("Triggered trap"); }, });`);
console.log(`isXrayWrapper(retval): ${Cu.isXrayWrapper(retval)}`);
console.log(`isXrayWrapper(Object): ${Cu.isXrayWrapper(sand.Object)}`);
retval instanceof sand.Object;

Expected:
isXrayWrapper(retval): true
isXrayWrapper(Object): true
true

Actual:
isXrayWrapper(retval): true
isXrayWrapper(Object): true
Error: Triggered trap

Comment 1

[Rob Wu [:robwu]]

Is this behavior intentional (and should we always add a Cu.isProxy check before instanceof) or is it a bug (so should the behavior be changed, to not trigger these traps)?

Comment 2

[Bobby Holley (:bholley)]

Hm, that's not the behavior I'd expect.

We don't support meaningful JSXrays to proxies [1], so we should end up with opaque Xray traits [2]. Getting the prototype of that should result in a call into the opaque xray traits [3], which should just return sand.Object.prototype [4].

I'm swamped with a few things, so if you have a minute to figure out why that's not happening, I'd appreciate it.

[1] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/xpconnect/wrappers/XrayWrapper.cpp#75
[2] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/xpconnect/wrappers/XrayWrapper.cpp#126
[3] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/xpconnect/wrappers/XrayWrapper.cpp#2349
[4] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/xpconnect/wrappers/XrayWrapper.h#336

Comment 3

[Rob Wu [:robwu]]

I think that CrossCompartmentWrapper::hasInstance [1] is the culprit:
    AutoRealm call(cx, wrappedObject(wrapper));
    if (!cx->compartment()->wrap(cx, v)) {
        return false;
    }
    return Wrapper::hasInstance(cx, wrapper, v, bp);

(where the variables have the meaning in "v instanceof wrapper")

At the start, wrapper's realm is entered.
Then v is supposedly wrapped. Since v and wrapper are in the same compartment, v ends up being unwrapped [2].
Consequently, proxy traps of v are triggered.


Besides triggering the "getPrototypeOf" trap of v,
proxy traps of wrapper may also be triggered:

let sand = Cu.Sandbox(null);
let func = sand.eval(`new Proxy(function(){}, {get(t,p) { throw "triggered trap: " + p.toString(); } })`);
"bar" instanceof func;
// Error: triggered trap: Symbol(Symbol.hasInstance)


[1]: https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/proxy/CrossCompartmentWrapper.cpp#437-446
[2]: https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/vm/Compartment.cpp#226-227,229

Comment 4

[Bobby Holley (:bholley)]

Ok, yeah. Looks like XrayWrapper is missing an override for hasInstance - we should add one. We should probably add an override on SecurityWrapper as well for good measure (though it wouldn't apply in this particular case).

Comment 5

[Rob Wu [:robwu]]

When I stepped through GDB with the original test case, due to the early return [1] the preWrap callback [2][3] wasn't even called. The fallback getOrCreateWrapper [4] is not triggered either for the same reason (=both operands being in the same compartment).
So the missing override in XrayWrapper might be responsible for the one I mentioned at the end of comment 3 (I haven't verified), but the cause of the original bug is likely different.

[1]: https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/vm/Compartment.cpp#226-227,229
[2]: https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/vm/Compartment.cpp#232-238,243
[3]: https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/xpconnect/wrappers/WrapperFactory.cpp#161
[4]: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/js/src/vm/Compartment.cpp#316-317,319-320

Comment 6

[Bobby Holley (:bholley)]

I'm not sure I understand.

The code in CrossCompartmentWrapper::hasInstance enters the realm of the _wrapped_ object (not the wrapper object), rewraps the operand into that compartment, and then forwards the call. This is standard CCW behavior, but not what we want out of XrayWrappers (since XrayWrappers are supposed to operate without ever entering the compartment of the target object). So the fix I'm proposing is to override CrossCompartmentWrapper::hasInstance with XrayWrapper::hasInstance, and replace the implementation with code that does not enter the target compartment.

Does that make sense?

Comment 7

[Rob Wu [:robwu]]

> Does that make sense?

Yes, thanks. My misconception was that hasInstance would be responsible for creating wrappers on the fly. That is of course not the case, as the wrappers are created when the object is taken from the sandbox.

(In reply to Bobby Holley (:bholley) from comment #6)
> So the fix I'm proposing is to override CrossCompartmentWrapper::hasInstance
> with XrayWrapper::hasInstance, and replace the implementation with code that
> does not enter the target compartment.

Not entering the target compartment = staying in the compartment of the caller of instanceof (instead of either operand)?
I tried that before (without adding a XrayWrapper::hasInstance override), by (re)moving AutoRealm from CCW::hasInstance (with and without rewrapping both operands of instanceof). The result was a crash due to an eventual assertion failure (compartment mismatch), originating from [1] (full stack trace: https://pastebin.com/c7dWFnss ).
I could try to rewrap the arguments before calling the @@hasInstance function, but I'm not sure if that is safe or desired. What do you suggest?

And is there any need to add an OpaqueCrossCompartmentWrapper::hasInstance override? I'm not sure what to do with that class since putting an unconditional "return false;" in OpaqueXrayTraits::hasInstance seems sufficiently sensible.


[1] https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/vm/Interpreter.cpp#871

Comment 8

[Bobby Holley (:bholley)]

(In reply to Rob Wu [:robwu] from comment #7)
> > Does that make sense?
> 
> Yes, thanks. My misconception was that hasInstance would be responsible for
> creating wrappers on the fly. That is of course not the case, as the
> wrappers are created when the object is taken from the sandbox.

That's right. Wrappers are created automatically (well, via the wrap() calls sprinkled in the JS engine) whenever an object crosses compartment boundaries. So in the case of |a instanceof b|, we'll end up in CrossCompartmentWrapper::hasInstance i.f.f. |b| is not in the current compartment of the executing script. And so CrossCompartmentWrapper enters the compartment of |b|, and then we pass |a| to the wrapper machinery. That machinery will strip off the old wrapper, and then, if the underlying identity object for |a| is not same-compartment with that of |b|, a new wrapper will be applied.
> 
> (In reply to Bobby Holley (:bholley) from comment #6)
> > So the fix I'm proposing is to override CrossCompartmentWrapper::hasInstance
> > with XrayWrapper::hasInstance, and replace the implementation with code that
> > does not enter the target compartment.
> 
> Not entering the target compartment = staying in the compartment of the
> caller of instanceof (instead of either operand)?

Correct. The basic model of Xrays is that they shield the caller from the JS state of the Xray-wrapped object. The easiest way of making sure that happens is to never enter the compartment of that object, and so that's generally what the XrayWrapper machinery does. There are some situations where it does enter the target compartment, but does so in a controlled fashion to avoid running any script.

> I tried that before (without adding a XrayWrapper::hasInstance override), by
> (re)moving AutoRealm from CCW::hasInstance (with and without rewrapping both
> operands of instanceof). The result was a crash due to an eventual assertion
> failure (compartment mismatch), originating from [1] (full stack trace:
> https://pastebin.com/c7dWFnss ).
> I could try to rewrap the arguments before calling the @@hasInstance
> function, but I'm not sure if that is safe or desired. What do you suggest?

Well, you don't want to touch CCW::hasInstance. It's doing the right thing for the general transparent CCW case, which is what that code implements and what lots of things depend on. You want to fix it for XrayWrapper, which inherits CCW and overrides the virtual methods to substitute different behavior (specifically, the "mostly avoid entering the compartment" behavior described above).

Basically, proxy handlers (which are singletons) specify the behavior that a given proxy object will implement. For our internal proxies, we have a large inheritance hierarchy of these handlers, rooted at BaseProxyHandler. The methods on the derived classes often add additional functionality and then delegate to the superclass (like how CrossCompartmentWrapper:foo methods manage the compartment-switching and wrapping and then delegate to Wrapper::foo). However, in the case of XrayWrapper, we usually re-implement a lot of that functionality to give the "clean view" that Xray callers expect.
 
> And is there any need to add an OpaqueCrossCompartmentWrapper::hasInstance
> override? I'm not sure what to do with that class since putting an
> unconditional "return false;" in OpaqueXrayTraits::hasInstance seems
> sufficiently sensible.

OpaqueCrossCompartmentWrapper is different from OpaqueXrayTraits - the latter is for subtypes of XrayWrappers, and the former is used in situations where XrayWrappers are not available (such as in workers, which don't have access to XPConnect stuff). So I think we want a separate implementation on both OpaqueCrossCompartmentWrapper and XrayWrapper.

I also realized that we don't need to add anything on SecurityWrapper, because Proxy::hasInstance does a proper AutoEnterPolicy in that case [1]. See bug 945826 comment 0.

[1] https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/js/src/proxy/Proxy.cpp#605

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1504660 - Move js::InstanceOfOperator to JS::InstanceofOperator

There is currently no public API to call the 'instanceof' handler
without triggering proxies. The public method, JS_HasInstance, may
skip the default logic if a class has a non-null JSHasInstanceOp.
(i.e. js/src/proxy/Proxy.cpp and js/src/ctypes/CTypes.cpp ).

To serve the need of the next patch (which needs to trigger the
instanceof logic without triggering the proxy), this patch publishes the
js::InstanceofOperator method.
JS::InstanceOfOperator is the new name, and the new capitalization
matches the name of the abstract operation in the ES6 specification.

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1504660 - Implement Xrays for instanceof

Depends on D11591

Comment 11

[Rob Wu [:robwu]]

The patches have landed a few days ago:

https://hg.mozilla.org/mozilla-central/rev/c07309d939dd
https://hg.mozilla.org/mozilla-central/rev/c50c098e40c4

(Why is this bug not auto-closed, like all others for which the patches have been pushed?)

Comment 12

[Boris Zbarsky [:bzbarsky]]

It's a security bug.  The auto-close bot doesn't have security bug access.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

What security rating should this bug have and how far back does the problem go?

Comment 14

[Rob Wu [:robwu]]

This problem has been around for ages; I just tested 57 and it's affected too.

As for the security rating: potentially bad behavior may occur when privileged code unexpectedly triggers (untrusted) JS code when the "instanceof" operator is used on an object+constructor from a different (untrusted) compartment. This untrusted code can then spoof the result of "instanceof" checks and either throw (e.g. comment 0) or return true to lie about its JS type; both are things that XrayWrappers are supposed to prevent.

To see the true impact, we need to audit the codebase and see if there is any use of the "instanceof" operator by privileged callers, involving (two) operands that are in the same untrusted compartment, and see if the privileged caller would behave incorrect if the "instanceof" operator behaves unexpectedly.

In other words, find JavaScript code that matches the following:

- "a instanceof b"
  where "a" and b" are in the same compartment,
  and the caller is in a different compartment (so "a" and/or "b" have XrayWrappers).

- "b" is usually a constructor for a known type.
  (this bug also affects "b" values with untrusted values, but if "b" is untrusted, then we are lost anyway, e.g. bug 1456531 - outside the scope of this bug)

- "a" is an untrusted value, provided by a third party script or page.

- The caller of "a instanceof b" relies on the accuracy of the operator for an important, security-sensitive decision.


I do not expect much code that matches all criteria (especially the last one), so in the absence of an actual audit, I would rate this as sec-low.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Sounds like we're fine letting this ride the trains then. In the future, please try to assess the severity and affected releases prior to landing so we can follow the process for sec approval if necessary.

Comment 16

[Oana Botisan, Desktop QA]

I managed to reproduce the issue on Windows 10 x64 using an older version of Nightly (2018-11-05) and the steps from comment 0.
I retested everything using Firefox 65.0.2, beta 66.0b13 and latest Nightly 67.0a1 on Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13. The issue is not reproducing anymore.

However, I have a question: If I run the text the second time (without restarting the browser), I get this error: " SyntaxError: redeclaration of let sand". I am guessing that this is expected, but I wanted to be sure.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I am guessing that this is expected

Yes, if you are re-evaluating things involving "let" in the same scope. If you wanted to run the test multiple times, you could run it like so:

(() => {
  let sand = Cu.Sandbox(null);
  let retval = sand.eval(`new Proxy({}, {getPrototypeOf() { throw new Error("Triggered trap"); }, });`);
  console.log(`isXrayWrapper(retval): ${Cu.isXrayWrapper(retval)}`);
  console.log(`isXrayWrapper(Object): ${Cu.isXrayWrapper(sand.Object)}`);
  retval instanceof sand.Object;
})();

Comment 18

[Oana Botisan, Desktop QA]

Considering comment 16 and comment 17, I will mark this issue as verified fixed.