Bug 1822712 Comment 2 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

I don't think this is security sensitive, however, we do end up with something peculiar. 

The key here is that v8 is a forwarding proxy with "prototype" behaviour, defined in `BaseProxyHandler` as follows: 

```
  /*
   * Proxy handlers can use mHasPrototype to request the following special
   * treatment from the JS engine:
   *
   *   - When mHasPrototype is true, the engine never calls these methods:
   *     has, set, enumerate, iterate.  Instead, for these operations,
   *     it calls the "own" methods like getOwnPropertyDescriptor, hasOwn,
   *     defineProperty, getOwnEnumerablePropertyKeys, etc.,
   *     and consults the prototype chain if needed.
   *
   *   - When mHasPrototype is true, the engine calls handler->get() only if
   *     handler->hasOwn() says an own property exists on the proxy. If not,
   *     it consults the prototype chain.
   *
   * This is useful because it frees the ProxyHandler from having to implement
   * any behavior having to do with the prototype chain.
   */
```

It turns out that the transparent wrapper created by `wrapWithProto` and `WindowNamedPropertiesHandler` are the only two proxy handlers that use this functionality. 

We can definitely see something going wrong with WindowNamedPropertiesHandler, which is used to proxy around the WindowProxy prototype, by playing in the console of a release build which skips that assert. If you visit `about:blank` and then in the console run the following code 

```
v8 = window.__proto__;
function f9(a10) {
    return v8;
}
class C13 extends f9 {
    #b = 12;
    static {
        const v13 = new this();

        const val = v8.#b; // Get
        assertEq(val, 12);

        v8.#b = 0; // Set
        assertEq(v8.#b, 0);

        assertEq(#b in v8.__proto__, false)

        assertEq(#b in v8, true); // HasOwn.
    }
}
```
you'll find that we end up in a seemingly curious situation: according to the devtools console, this object has gained *two copies* of `#b`: 

```
>> window.__proto__
WindowPrototype { #b: 12, #b: 0, … }
```

Which is definitely wrong. Tracing through the calls in a similar shell test case, it appears that because we don't find the private field when calling the proxy's GetOwnPropertyDescriptor, we end up deciding to define a new property, which has the same name. 

I don't _think_ this is exploitable in any way. It's just gross. I have a patch which I think resolves this, by instead avoiding the Proxy prototype behaviour for private fields.  Will post it when I've validated it.
I don't think this is security sensitive, however, we do end up with something peculiar. 

The key here is that v8 is a forwarding proxy with "prototype" behaviour, defined in `BaseProxyHandler` as follows: 

```
  /*
   * Proxy handlers can use mHasPrototype to request the following special
   * treatment from the JS engine:
   *
   *   - When mHasPrototype is true, the engine never calls these methods:
   *     has, set, enumerate, iterate.  Instead, for these operations,
   *     it calls the "own" methods like getOwnPropertyDescriptor, hasOwn,
   *     defineProperty, getOwnEnumerablePropertyKeys, etc.,
   *     and consults the prototype chain if needed.
   *
   *   - When mHasPrototype is true, the engine calls handler->get() only if
   *     handler->hasOwn() says an own property exists on the proxy. If not,
   *     it consults the prototype chain.
   *
   * This is useful because it frees the ProxyHandler from having to implement
   * any behavior having to do with the prototype chain.
   */
```

It turns out that the transparent wrapper created by `wrapWithProto` and `WindowNamedPropertiesHandler` are the only two proxy handlers that use this functionality. 

<del>We can definitely see something going wrong with WindowNamedPropertiesHandler, which is used to proxy around the WindowProxy prototype, by playing in the console of a release build which skips that assert. If you visit `about:blank` and then in the console run the following code 

```
v8 = window.__proto__;
function f9(a10) {
    return v8;
}
class C13 extends f9 {
    #b = 12;
    static {
        const v13 = new this();

        const val = v8.#b; // Get
        assertEq(val, 12);

        v8.#b = 0; // Set
        assertEq(v8.#b, 0);

        assertEq(#b in v8.__proto__, false)

        assertEq(#b in v8, true); // HasOwn.
    }
}
```
you'll find that we end up in a seemingly curious situation: according to the devtools console, this object has gained *two copies* of `#b`: 

```
>> window.__proto__
WindowPrototype { #b: 12, #b: 0, … }
```


Which is definitely wrong. Tracing through the calls in a similar shell test case, it appears that because we don't find the private field when calling the proxy's GetOwnPropertyDescriptor, we end up deciding to define a new property, which has the same name. 

I don't _think_ this is exploitable in any way. It's just gross. I have a patch which I think resolves this, by instead avoiding the Proxy prototype behaviour for private fields.  Will post it when I've validated it.
</del>

Edited to update: No -- I just tested wrong. It seems like even WindowPrototype is working correctly. I've yet to actually discover something that testably produces an incorrect result. I will nevertheless post my patch which I think is an improvement here.
I don't think this is security sensitive, however, we do end up with something peculiar. 

The key here is that v8 is a forwarding proxy with "prototype" behaviour, defined in `BaseProxyHandler` as follows: 

```
  /*
   * Proxy handlers can use mHasPrototype to request the following special
   * treatment from the JS engine:
   *
   *   - When mHasPrototype is true, the engine never calls these methods:
   *     has, set, enumerate, iterate.  Instead, for these operations,
   *     it calls the "own" methods like getOwnPropertyDescriptor, hasOwn,
   *     defineProperty, getOwnEnumerablePropertyKeys, etc.,
   *     and consults the prototype chain if needed.
   *
   *   - When mHasPrototype is true, the engine calls handler->get() only if
   *     handler->hasOwn() says an own property exists on the proxy. If not,
   *     it consults the prototype chain.
   *
   * This is useful because it frees the ProxyHandler from having to implement
   * any behavior having to do with the prototype chain.
   */
```

It turns out that the transparent wrapper created by `wrapWithProto` and `WindowNamedPropertiesHandler` are the only two proxy handlers that use this functionality. 

~We can definitely see something going wrong with WindowNamedPropertiesHandler, which is used to proxy around the WindowProxy prototype, by playing in the console of a release build which skips that assert. If you visit `about:blank` and then in the console run the following code~

```
v8 = window.__proto__;
function f9(a10) {
    return v8;
}
class C13 extends f9 {
    #b = 12;
    static {
        const v13 = new this();

        const val = v8.#b; // Get
        assertEq(val, 12);

        v8.#b = 0; // Set
        assertEq(v8.#b, 0);

        assertEq(#b in v8.__proto__, false)

        assertEq(#b in v8, true); // HasOwn.
    }
}
```
~you'll find that we end up in a seemingly curious situation: according to the devtools console, this object has gained *two copies* of `#b`:~

```
>> window.__proto__
WindowPrototype { #b: 12, #b: 0, … }
```


~Which is definitely wrong. Tracing through the calls in a similar shell test case, it appears that because we don't find the private field when calling the proxy's GetOwnPropertyDescriptor, we end up deciding to define a new property, which has the same name.~

~I don't _think_ this is exploitable in any way. It's just gross. I have a patch which I think resolves this, by instead avoiding the Proxy prototype behaviour for private fields.  Will post it when I've validated it.~

Edited to update: No -- I just tested wrong. It seems like even WindowPrototype is working correctly. I've yet to actually discover something that testably produces an incorrect result. I will nevertheless post my patch which I think is an improvement here.

Back to Bug 1822712 Comment 2