1822712 - Assertion failure: !id.isPrivateName(), at proxy/Proxy.cpp:191

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 8ed22fcd56968c95a73a6c82b42f732f01a4bdae the attached sample asserts in the js-shell when invoked via obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe crash.js. The issue doesn't appear to be a recent regressor, a version from end of 2021 is affected as well. I'm not sure whether this is s-s, on release builds there is no segfault/exception.

#0  js::Proxy::getOwnPropertyDescriptor (cx=cx@entry=0x7ffff7430100, proxy=proxy@entry=..., id=id@entry=..., desc=desc@entry=...)
    at js/src/proxy/Proxy.cpp:190
#1  0x00005555571c4e36 in js::GetOwnPropertyDescriptor (cx=cx@entry=0x7ffff7430100, obj=obj@entry=..., id=id@entry=...,
    desc=desc@entry=...) at js/src/vm/JSObject.cpp:2023
#2  0x00005555576ebfcc in js::ForwardingProxyHandler::getOwnPropertyDescriptor (this=<optimized out>, cx=0x7ffff7430100,
    proxy=..., id=..., desc=...) at js/src/proxy/Wrapper.cpp:56
#3  0x00005555576bea32 in js::BaseProxyHandler::set (this=0x555558922700 <js::Wrapper::singletonWithPrototype>,
    cx=0x7ffff7430100, proxy=..., id=..., v=..., receiver=..., result=...)
    at js/src/proxy/BaseProxyHandler.cpp:148
#4  0x00005555576d8d70 in js::Proxy::setInternal (cx=0x7ffff7430100, proxy=proxy@entry=..., id=id@entry=..., v=v@entry=...,
    receiver=receiver@entry=..., result=...) at js/src/proxy/Proxy.cpp:561
#5  0x00005555576d89fa in js::Proxy::set (cx=0x7ffff79f8a00 <_IO_stdfile_2_lock>, proxy=..., id=..., v=..., receiver_=...,
    result=...) at js/src/proxy/Proxy.cpp:572
#6  0x0000555556eb6150 in SetObjectElementOperation (cx=0x7ffff7430100, value=..., receiver=..., obj=..., id=...,
    strict=<optimized out>) at js/src/vm/Interpreter.cpp:1854
#7  Interpret (cx=cx@entry=0x7ffff7430100, state=...) at js/src/vm/Interpreter.cpp:3207
#8  0x0000555556eaa015 in js::RunScript (cx=cx@entry=0x7ffff7430100, state=...)
    at js/src/vm/Interpreter.cpp:431
#9  0x0000555556ec7762 in js::ExecuteKernel (cx=cx@entry=0x7ffff7430100, script=script@entry=...,
    envChainArg=envChainArg@entry=..., evalInFrame=evalInFrame@entry=..., result=...)
    at js/src/vm/Interpreter.cpp:818
#10 0x0000555556ec7e11 in js::Execute (cx=cx@entry=0x7ffff7430100, script=script@entry=..., envChain=..., rval=rval@entry=...)
    at js/src/vm/Interpreter.cpp:850
#11 0x0000555557075b26 in ExecuteScript (cx=cx@entry=0x7ffff7430100, envChain=..., script=..., rval=rval@entry=...)
   hax/gecko-fuzzilli/js/src/vm/CompilationAndEvaluation.cpp:472
#12 0x0000555557075e00 in JS_ExecuteScript (cx=cx@entry=0x7ffff7430100, scriptArg=scriptArg@entry=...)
    at js/src/vm/CompilationAndEvaluation.cpp:496
#13 0x0000555556de02b0 in RunFile (cx=0x7ffff7430100, filename=0x7fffffffea5f "crash_2023_03_15_2.js", file=<optimized out>, compileMethod=CompileU
tf8::DontInflate,
    compileOnly=false, fullParse=<optimized out>) at js/src/shell/js.cpp:1098
#14 0x0000555556ddf755 in Process (cx=cx@entry=0x7ffff7430100, filename=0x1 <error: Cannot access memory at address 0x1>, forceTTY=false, kind=kind
@entry=FileScript)
    at js/src/shell/js.cpp:1697
#15 0x0000555556da12ef in ProcessArgs (cx=0x7ffff7430100, op=0x7fffffffe438) at js/src/shell/js.cpp:10577
#16 Shell (cx=0x7ffff7430100, op=op@entry=0x7fffffffe438) at js/src/shell/js.cpp:10801
#17 0x0000555556d9add7 in main (argc=<optimized out>, argv=<optimized out>) at js/src/shell/js.cpp:11233

const o0 = {}; 
const v2 = new Proxy(o0, o0);
const v8 = this.wrapWithProto(v2, {});
function f9(a10) {
    return v8; 
}
class C11 extends f9 {
    #b; 
    static {
        const v13 = new this();
        v8.#b = 0;
    }   
}

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

This bug has my name written all over it. I'll try to take a look today, needinfoing myself to ensure it doesn't get dropped.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1822712 - Don't use hasPrototype behaviour for private names in proxy set method r?jandem

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Un-marking this as s-s: I can't think of a way in which this ends up being exploitable. There are certainly possibilities for semantic issues, but they don't stack into a security issue.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/721df3c37567
Don't use hasPrototype behaviour for private names in proxy set method r=jandem

Comment 6

[Marian-Vasile Laza]

Backed out changeset 721df3c37567 (Bug 1822712) for jsreftest failures on prototype-proxy.js.
Backout link
Push with failures <--> J1 <--> X2
Failure Log J1
Failure Log X2
Also X1

Comment 7

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/951431ac000b

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6b34f285903
Don't use hasPrototype behaviour for private names in proxy set method r=jandem

Comment 9

[Norisz Fay [:noriszfay]]

Backed out for causing xpcshell failures on test_private_field_xrays.js

Backout link

Push with failures

Failure log

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1822712 - Implment ProxyGetOwnPropertyDescriptorFromExpando as its accessible under some circumstances r?jandem

Originally proxy support was implemented such that we expected to never invoke
getOwnPropertyDescriptor for a proxy with a private field expando. However, it
turns out that when the proxy returns true for hasPrototype() this is actually
possible; implementation is relatively simple fortunately.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ddbc3fe3800
Implment ProxyGetOwnPropertyDescriptorFromExpando as its accessible under some circumstances r=jandem

Comment 12

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/3ddbc3fe3800