1392026 - (CVE-2017-7831) ExposedPropertiesOnly::check calls proxy traps "has" and "getOwnPropertyDescriptor" for property "__exposedProps__"

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Oriol Brufau [:Oriol]]

Open the browser console and enter this code:

    var proxy = new Proxy({}, new Proxy({}, {get: (_, trap) => (_, ...args) => {
        throw new Error(`proxy trap ${JSON.stringify(trap)} was called with arguments ${args.map(JSON.stringify).join(", ")}.`);
    }}));
    var sandbox = Cu.Sandbox(null);
    sandbox.inheritsProxy = Object.create(proxy);

    sandbox.eval(`Reflect.apply(inheritsProxy, null, [])`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.defineProperty(inheritsProxy, "a", {})`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.deleteProperty(inheritsProxy, "a", {})`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.get(inheritsProxy, "a")`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.getOwnPropertyDescriptor(inheritsProxy, "a")`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.has(inheritsProxy, "a")`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.ownKeys(inheritsProxy)`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

    sandbox.eval(`Reflect.set(inheritsProxy, "a", 1)`);
    // Error: proxy trap "has" was called with arguments "__exposedProps__".

This should not happen. It does not happen if I use one of these:

    sandbox.inheritsProxy = Object.setPrototypeOf(function(){}, proxy);
    sandbox.inheritsProxy = Object.setPrototypeOf([], proxy);
    sandbox.inheritsProxy = proxy;

Then I only get "Permission denied" errors. So how come the proxy trap runs with Object.create(proxy) ?

The security wrapper is calling traps instead of protecting the proxy!!

This seems like a flag of the security wrapper, so I'm marking this as security sensitive.

Comment 2

[Oriol Brufau [:Oriol]]

It seems this can make an assert fail. Initially I reported it as bug 1391869, but I guess the assert is fine and it's just that the security wrappers are not behaving properly.

1. Run a debugging build
2. Disable activity stream, so that new tabs are privileged
3. Open a new tab
4. Open the web console and enter this code:

  var iframe = document.createElement('iframe');
  iframe.src = "about:blank";
  document.documentElement.appendChild(iframe);
  var inheritsProxy = Object.create(new Proxy({}, {
    has: (_, prop) => {
      throw new Error('proxy trap "has" was called with argument ' + JSON.stringify(prop));
    }
  }));
  iframe.onload = function() {
    gDebuggee = iframe.contentWindow;
    Cu.exportFunction(eval, gDebuggee, {defineAs: "globalEval"})
    gDebuggee.eval("(function stopMe(arg) {debugger;})(globalEval('inheritsProxy'));");
  };

Result: Failed assert. Stack trace:

    xul.dll!js::CallJSNative(JSContext * cx, bool(*)(JSContext *, unsigned int, JS::Value *) native, const JS::CallArgs & args) Line 296	C++
    xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 469	C++
    xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args) Line 515	C++
    xul.dll!js::Call(JSContext * cx, JS::Handle<JS::Value> fval, JS::Handle<JS::Value> thisv, const js::AnyInvokeArgs & args, JS::MutableHandle<JS::Value> rval) Line 533	C++
    xul.dll!js::CallGetter(JSContext * cx, JS::Handle<JS::Value> thisv, JS::Handle<JS::Value> getter, JS::MutableHandle<JS::Value> rval) Line 648	C++
    xul.dll!CallGetter(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<js::Shape *> shape, JS::MutableHandle<JS::Value> vp) Line 2064	C++
    xul.dll!GetExistingProperty<1>(JSContext * cx, JS::Handle<JS::Value> receiver, JS::Handle<js::NativeObject *> obj, JS::Handle<js::Shape *> shape, JS::MutableHandle<JS::Value> vp) Line 2112	C++
    xul.dll!NativeGetPropertyInline<1>(JSContext * cx, JS::Handle<js::NativeObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, IsNameLookup nameLookup, JS::MutableHandle<JS::Value> vp) Line 2343	C++
    xul.dll!js::NativeGetProperty(JSContext * cx, JS::Handle<js::NativeObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 2378	C++
    xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 1537	C++
    xul.dll!js::Wrapper::get(JSContext * cx, JS::Handle<JSObject *> proxy, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 147	C++
    xul.dll!js::CrossCompartmentWrapper::get(JSContext * cx, JS::Handle<JSObject *> wrapper, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 226	C++
    xul.dll!js::Proxy::get(JSContext * cx, JS::Handle<JSObject *> proxy, JS::Handle<JS::Value> receiver_, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 340	C++
    xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 1535	C++
    xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, js::PropertyName * name, JS::MutableHandle<JS::Value> vp) Line 836	C++
    xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JS::Value> v, JS::Handle<js::PropertyName *> name, JS::MutableHandle<JS::Value> vp) Line 4407	C++
    xul.dll!GetPropertyOperation(JSContext * cx, js::InterpreterFrame * fp, JS::Handle<JSScript *> script, unsigned char * pc, JS::MutableHandle<JS::Value> lval, JS::MutableHandle<JS::Value> vp) Line 192	C++
    xul.dll!Interpret(JSContext * cx, js::RunState & state) Line 2784	C++
    xul.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 409	C++
    xul.dll!js::InternalCallOrConstruct(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 487	C++
    xul.dll!InternalCall(JSContext * cx, const js::AnyInvokeArgs & args) Line 515	C++
    xul.dll!js::CallFromStack(JSContext * cx, const JS::CallArgs & args) Line 521	C++
    xul.dll!js::jit::DoCallFallback(JSContext * cx, js::jit::BaselineFrame * frame, js::jit::ICCall_Fallback * stub_, unsigned int argc, JS::Value * vp, JS::MutableHandle<JS::Value> res) Line 2589	C++

Comment 4

[Oriol Brufau [:Oriol]]

The problem is here:
https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/js/xpconnect/wrappers/AccessCheck.cpp#359
https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/js/xpconnect/wrappers/AccessCheck.cpp#388

I think the prototype chain should be iterated manually, using JS_GetOwnPropertyDescriptorById on each object **if the policy allows it**. Alternatively, don't iterate the prototype chain of the unwrapped object at all, but this could break things. Anyways, never access proxy objects, they have an opaque policy!

Comment 5

[Bobby Holley (:bholley)]

We should be able to just remove __exposedProps__ at this point, see bug 1377587.

A more-targeted fix is likely possible. I don't have cycles to dig in though, so will need another xpconnect peer to pick this up.

Comment 6

[Andrew McCreight [:mccr8]]

Can you take a look, Gabor?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Can you take a look, Gabor?

Sorry for the lag, I was on PTO. Digging myself out of my TODO list, but I should get to this one during this week.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> A more-targeted fix is likely possible. I don't have cycles to dig in
> though, so will need another xpconnect peer to pick this up.

I don't see any reason why we should let __exposedProps__ work on proxy objects or objects with a proxy in their proto chain especially since we're moving away from __exposedProps__.

(In reply to Oriol Brufau [:Oriol] from comment #4)
> The problem is here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 7dddbd85047c6dc73ddbe1e423cd643a217845b3/js/xpconnect/wrappers/AccessCheck.
> cpp#359
> https://dxr.mozilla.org/mozilla-central/rev/
> 7dddbd85047c6dc73ddbe1e423cd643a217845b3/js/xpconnect/wrappers/AccessCheck.
> cpp#388
> 
> I think the prototype chain should be iterated manually, using
> JS_GetOwnPropertyDescriptorById on each object **if the policy allows it**.
> Alternatively, don't iterate the prototype chain of the unwrapped object at
> all, but this could break things. Anyways, never access proxy objects, they
> have an opaque policy!

Thanks, this is an awesome bug report! Do you know about any use case where we want __exposedProps to work on proxy objects (or objects with proxy in their proto chain)?

Comment 9

[Bobby Holley (:bholley)]

Per bug 1377587, I don't think we care about __exposedProps__ working at all, period. I'm not sure why that bug stalled, but we could also just finish that instead.

Comment 10

[Oriol Brufau [:Oriol]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Do you know about any use case where we want __exposedProps to work
> on proxy objects (or objects with proxy in their proto chain)?

Nope. But I think that if an object has a policy that allows __exposedProps__, then there is no need to break that in case there is a proxy in the prototype. It's just that the __exposedProps__ lookup should stop iterating the prototype chain when an object with an opaque policy is found.

But as Bobby Holley says, if __exposedProps__ is going to be removed soon, then I guess it doesn't matter.

Comment 11

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb7d6b1d4b33fcc866dbca51e21868a3b0933d8c&selectedJob=128534385

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: __exposedProperties__ should not support scripted proxies. v1

Andrew, do you think we should quickfix this issue or should we wait for the removal of __exposedProperties__ ? (I guess that patch won't be upliftable to 56, but I'm not sure if that's a huge problem...)

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> Andrew, do you think we should quickfix this issue or should we wait for the
> removal of __exposedProperties__ ? (I guess that patch won't be upliftable
> to 56, but I'm not sure if that's a huge problem...)

Thanks for working on this. What are the security implications of this problem? Would this require somebody write some odd code in an extension, or does this affect baseline Firefox? What is the worst that could happen from it? That will determine the security rating, which will determine whether we should fix this on older branches, or just remove __exposedProps__ on trunk and leave it alone otherwise.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8904917 [details] [diff] [review]
__exposedProperties__ should not support scripted proxies. v1

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

I'm not really qualified to review this.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +354,5 @@
>      //
>      // Unfortunately, |cx| can be in either compartment when we call ::check. :-(
>      JSAutoCompartment ac(cx, wrappedObject);
>  
> +    // Proxy's are not allowed in the proto chain.

nit: "Proxies" not "Proxy's".

Comment 15

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> > Andrew, do you think we should quickfix this issue or should we wait for the
> > removal of __exposedProperties__ ? (I guess that patch won't be upliftable
> > to 56, but I'm not sure if that's a huge problem...)
> 
> Thanks for working on this. What are the security implications of this
> problem? Would this require somebody write some odd code in an extension, or
> does this affect baseline Firefox? What is the worst that could happen from
> it? That will determine the security rating, which will determine whether we
> should fix this on older branches, or just remove __exposedProps__ on trunk
> and leave it alone otherwise.

So the whole thing came up from a debugger test as far as I can tell (see bug 1391869), and the annoying part is that you don't even have to define any __exposedProps__ on a privileged object to be able to trigger the proxy trap on it. I would pretty much ignore the problem, since normally some really weird add-on is needed to expose a privileged object to content with a proxy hooked into its proto chain.

But it seems like with the debugger we might run into this problem and during debugging hitting proxy traps unexpectedly can cause a whole lot of trouble. So from security standpoint this seems like a moderate, the actual importance/urgency to fix this bug is depending on how can the debugger ever run into a case where this problem happens. 

Oriol, could you explain us what are the implications of this bug from the debugger perspective?

Comment 16

[Oriol Brufau [:Oriol]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Oriol, could you explain us what are the implications of this bug from the
> debugger perspective?

It means that if the debugger gets an object with ExposedPropertiesOnly policy, then just doing trivial things like getting its class will autoenter the policy. This will iterate the prototype with an __exposedProps__ lookup, possibly running proxy traps.
Moreover, this operation is expected to not throw, but a proxy trap can throw and thus fail some asserts as explained in comment 2. Not nice if you are using a debug build and it crashes.

The debugger should never run scripted proxy traps. I'm attempting to cover all cases, so I noticed this one. But I guess that debugging an object with ExposedPropertiesOnly policy is not much common. Mostly it's when you are debugging a global A which has a reference to an object from a system principal global B, but A does not subsume B. This can be done using the console or add-ons.

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Oriol Brufau [:Oriol] from comment #16)
> The debugger should never run scripted proxy traps. I'm attempting to cover
> all cases, so I noticed this one.

Thanks for the thorough tests :) and for the explanation.

So all in all, from a security perspective I'm not too worried probably it's OK to just wait for the removal of __exposedProps__. Based on Comment 16 it does not seem super important for the debugger either, but it might cause some annoyances for a couple of people in some very edge cases. I'm OK with just won't fix this if __exposedProps__ will make it to 57, alternatively we can land/uplift some hack like my patch to quickfix it, if we want to stay on the safe side. Bobby, any preference?

Comment 18

[Bobby Holley (:bholley)]

Unless we have a more compelling threat scenario, I think waiting for the removal of __exposedProps__ is probably fine, assuming that work continues to progress.

Comment 19

[Andrew McCreight [:mccr8]]

Fixed by bug 1377587.

Thanks for filing this bug. It gave me motivation to actually remove __exposedProps__.