1166847 - The wrapper for the debuggee global should not be a security wrapper

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Eddy Bruel [:ejpbruel]]

The worker debugger API allows you to create a debugger global in a worker. The debugger global implements several API functions needed by the debugger. Most importantly, it provides a way for the debugger to get access to the debuggee global (see here).

The main (and so far only) reason we need access to the debuggee global is so that we can pass it to addDebuggee in the SpiderMonkey debugger API (see here). Note that this function calls unwrapDebuggeeArgument, which ends up calling CheckedUnwrap on the global passed in (see here).

Since the debugger has access to the debuggee global, it can inadvertently end up calling content code, which is a potential attack vector. To avoid this, we wrap the debugge global in a security wrapper called DebuggeeGlobalSecurityWrapper (defined here).

DebuggeeGlobalSecurityWrapper is highly specific. It is a cross compartment security wrapper that only allows an edge from the debugger to the debuggee global, and to nothing else. Any attempt to unwrap the wrapper, including from the debugger compartment, will fail.

The problem with this solution is that it turns out to be too secure. Any JS code running in the debugger compartment is unable to access the debuggee global (as intended), but neither is the SpiderMonkey debugger API. When it tries to call CheckedUnwrap on the debuggee global, it fails.

What we really want is for everything to be unable to access the debuggee global, except the debugger API.

I discussed this with bholley, and he suggested we do the following:

If we consider the debugger scope to be "chrome" code, then it must always subsume the principal of the content code, which means that we should have mHasSecurityPolicy=false. But we have other reasons why we want to avoid having it interact with the content code, and so we'd create a proxy handler that manually overrides all the traps to have opaque behavior. This would basically be re-implementing OpaqueXrayTraits (see XrayWrapper.h).

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor DebuggeeGlobalSecurityWrapper into DebuggeeGlobalWrapper

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8608259 [details] [diff] [review]
Refactor DebuggeeGlobalSecurityWrapper into DebuggeeGlobalWrapper

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

This looks good - I think we should hoist it into the JS engine (similar to DeadObjectProxy) to improve code reuse and reduce the number of large proxy implementations scattered throughout gecko (which is a pain for SM hackers when they want to rename things).

So please hoist most/all of the meat into js/src/proxy, and flag evilepies for review, who's currently refactoring a bunch of this stuff. :-)

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Implement OpaqueCrossCompartmentWrapper

Comment 4

[Tom S [:evilpie]]

It's a bit sad that we get yet an other totally empty Proxy implementation besides SecurityWrapper and DeadObjectProxy. Why don't the operations throw instead of silently doing nothing? I am understanding this right that the only point of this is to allow the Debugger to unwrap it?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #4)
> It's a bit sad that we get yet an other totally empty Proxy implementation
> besides SecurityWrapper and DeadObjectProxy.

These proxy implementations exist, it's just a question of whether they live in the JS engine (where it's easier for SM hackers to maintain them) or in Gecko.

> Why don't the operations throw
> instead of silently doing nothing? I am understanding this right that the
> only point of this is to allow the Debugger to unwrap it?

They could throw too, though that would make this less analogous to OpaqueXrayTraits, and less likely to share code down the line. But how would throwing reduce the amount of code we have to write? The point here is that we _can't_ make them SecurityWrappers, because then they're not unwrappable.

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8609418 [details] [diff] [review]
Implement OpaqueCrossCompartmentWrapper

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

::: dom/workers/RuntimeService.cpp
@@ +942,5 @@
>      JS_SetRuntimePrivate(rt, new WorkerThreadRuntimePrivate(aWorkerPrivate));
>  
>      js::SetPreserveWrapperCallback(rt, PreserveWrapper);
>      JS_InitDestroyPrincipalsCallback(rt, DestroyWorkerPrincipals);
> +    JS_SetWrapObjectCallbacks(rt, &WrapObjectCallbacks);

Not based on trunk I assume.

::: js/src/jswrapper.h
@@ +204,5 @@
> +    virtual bool hasOwn(JSContext* cx, HandleObject wrapper, HandleId id,
> +                        bool* bp) const override;
> +    virtual bool getOwnEnumerablePropertyKeys(JSContext* cx, HandleObject wrapper,
> +                                              AutoIdVector& props) const override;
> +    virtual const char* className(JSContext* cx, HandleObject wrapper) const override;

What is the reason for not overriding everything in CrossCompartmentWrapper? Like fun_toString here.

::: js/src/proxy/OpaqueCrossCompartmentWrapper.cpp
@@ +13,5 @@
> +                                                        HandleObject wrapper,
> +                                                        HandleId id,
> +                                                        MutableHandle<PropertyDescriptor> desc) const
> +{
> +  return true;

add desc.object().set(nullptr);

@@ +23,5 @@
> +                                              HandleId id,
> +                                              Handle<JSPropertyDescriptor> desc,
> +                                              ObjectOpResult& result) const
> +{
> +  return result.succeed();

4 char indent everything.

@@ +29,5 @@
> +
> +bool
> +OpaqueCrossCompartmentWrapper::ownPropertyKeys(JSContext* cx,
> +                                               HandleObject wrapper,
> +                                               JS::AutoIdVector& props) const

drop the JS::

@@ +43,5 @@
> +}
> +
> +bool
> +OpaqueCrossCompartmentWrapper::enumerate(JSContext* cx, HandleObject wrapper,
> +                                         MutableHandleObject objp) const

This and other definitely fit in one line. (99 chars in SpiderMonkey)

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Implement OpaqueCrossCompartmentWrapper

(In reply to Tom Schuster [:evilpie] from comment #6)
New patch with comments addressed.

> Not based on trunk I assume.

This patch is based on fx-team

> What is the reason for not overriding everything in CrossCompartmentWrapper?
> Like fun_toString here.

I don't know. I didn't write that code.

Note that I had to make a small change to RuntimeService.cpp:Wrap, because it turned out during testing that the debugger *can* in fact have edges to other objects than the global in the debuggee (via unsafeDereference).

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8610596 [details] [diff] [review]
Implement OpaqueCrossCompartmentWrapper

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

::: js/src/jswrapper.h
@@ +206,5 @@
> +    virtual bool getOwnEnumerablePropertyKeys(JSContext* cx, HandleObject wrapper,
> +                                              AutoIdVector& props) const override;
> +    virtual const char* className(JSContext* cx, HandleObject wrapper) const override;
> +    virtual bool defaultValue(JSContext* cx, HandleObject obj, JSType hint, MutableHandleValue vp) const;
> +

Please override objectClassIs { return false; }

and fun_toString {

    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_INCOMPATIBLE_PROTO,
                         js_Function_str, js_toString_str, "object");

}

Comment 9

[ejpbruel]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b740e88fc0

Comment 10

[ejpbruel]

Try push showed build errors due to missing headers. Strangely, I do not get this error on OSX, only on Android and B2g builds, so I can't actually tell which header is missing without installing Linux. I suspect it is "jsobj.h" or "jsobjinlines.h" though, so here is a new try push with that header added:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=badd3118e8e1

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e7aebc8275ce

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e7aebc8275ce

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8610596 [details] [diff] [review]
Implement OpaqueCrossCompartmentWrapper

>+++ b/js/src/jswrapper.h
>+class JS_FRIEND_API(OpaqueCrossCompartmentWrapper) : public CrossCompartmentWrapper
>+{
[...]
>+    virtual bool defaultValue(JSContext* cx, HandleObject obj, JSType hint, MutableHandleValue vp) const;


This method, along with the other two suggested in comment 8, were missing the "override" annotation. This causes clang 3.6 &
newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that annotation to these lines, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31275c9a8ae7

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31275c9a8ae7