945826 - Every proxy trap |foo| must either use AutoEnterPolicy in Proxy::foo or be overridden in SecurityWrapper<Base>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

If you're in the CC list, please read this (comment 0).

I just discovered that Proxy::{,un}watch had been added, with no AutoEnterPolicy and no override of BaseProxyHandler::{,un}watch in SecurityWrapper.

I almost had a heart attack, until I discovered that there was no implementation of DirectProxyHandler::{,un}watch, and that it was only used by DOM proxies. Whew.

So this isn't a security issue as it stands. But I'm worried that people may not understand the security sensitivity of the proxy hierarchy, and so I want to educate all the people who might possibly be reviewing code in js/src about it. If you never work with proxies and your eyes start to glaze over, that's ok, but make sure to flag me if you ever do happen to touch the proxy hierarchy (or review a patch that does).

The basic issue is that wrappers are structured in an inheritance hierarchy, such that all wrappers inherit implementations from DirectProxyHandler unless the implementations are explicitly overridden. In general, if you add a trap with at least one non-primitive argument/rv to DirectProxyHandler, you also need to add a corresponding trap in CrossCompartmentWrapper, otherwise you'll likely pave the way for a compartment mismatch when the trap is triggered on a Cross Compartment Wrapper.

All security-sensitive wrappers in Gecko inherit from SecurityWrapper<Base> (which inherits from |Base|, which is generally either Wrapper or CrossCompartmentWrapper). This does a few things:
* It marks the wrapper as unsafe to unwrap, such that js::CheckedUnwrap will refuse to unwrap it.
* It marks the wrapper as having a security policy. This collaborates with the Proxy::foo entry points, which explicitly query the security policy before proceeding.
* It adds explicit overrides for any traps where Proxy::foo does not query a policy (usually because the operation is very specific, and adding a new policy action for everyone to reason about would be overly cumbersome).

This means that we absolutely need to enforce the following invariants:
* Unless you really know what you're doing, all calls from the VM into a proxy should go through the Proxy::foo entry points. Do not grab the handler and invoke the trap directly.
* Every operation on BaseProxyHandler must either add an AutoEnterPolicy in the corresponding Proxy::foo entry point, or explicitly MOZ_OVERRIDE the trap in SecurityWrapper. doing both is acceptable as well.
* An override in DirectProxyHandler needs a corresponding override in CrossCompartmentWrapper unless you're sure that it doesn't.

It would be nice if we could somehow structure things to avoid these footguns, but it's a pretty hard problem. We've tried a number of different hierarchies over the years, and this seems to be the best one. Waldo has some ideas for enforcing this with static analysis. I don't have the bandwidth to pursue that right now, but if someone did that would be cool.

I'm going to fix up these traps, and add some big comments explaining this. Unfortunately, this stuff is dense enough that it's not guaranteed that people hacking on it will see them.

When in doubt, flag me for review or feedback.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Trap overrides and comments. v1

Comment 2

[Bobby Holley (:bholley)]

build-only try run given the templates here:

https://tbpl.mozilla.org/?tree=Try&rev=aba2e3c98917

Comment 3

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f7a5b3bb3a

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[u279076]

Bobby, does this need QA testing before we release Firefox 28?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> Bobby, does this need QA testing before we release Firefox 28?

Nope.