1853180 - Assertion failure: !cx->runtime()->jitRuntime()->disallowArbitraryCode(), at vm/Interpreter.cpp:416

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 36b5aac98935ee96edeb8ede5de56f935ba8f4ed the js-shell asserts on the following sample when invoked as obj-x86/dist/bin/js --fuzzing-safe --fast-warmup crash.js
Given how fast the fuzzers are finding the crash this is probably a very recent regression, maybe from bug 1850744 (guessing because the bug reproducers look rather similar).
The crash is a bit flaky and didn't reproduce in an non-optimized build; I'll upload my mozconfig and a recording.

function main() {
for (let i3 = 0, i4 = 10; i3 < i4; i4--) {
}
const v11 = [this,this,this,this];
Reflect.setPrototypeOf(Reflect, v11);
function f14() {
    const o18 = {
        get f() {
            for (let i = 0; i < 10; i++) {
                v11[this];
            }
            return Proxy;
        },
        __proto__: Reflect,
    };
    return o18;
}
const v19 = f14();
const t18 = v19.f;
const v21 = new t18(v19, v19);
const t20 = v21.f;
const v23 = new t20(v21, v19);
v23.get(v23, v23);
}
for (let i30 = 0; i30 < 100; i30++) {
    main()
}
gc();

#0  js::RunScript (cx=cx@entry=0x21152572e100, state=...)
    at js/src/vm/Interpreter.cpp:415
#1  0x0000557122d26f68 in js::InternalCallOrConstruct (cx=0x21152572e100, args=..., 
    construct=construct@entry=js::NO_CONSTRUCT, reason=<optimized out>)
    at js/src/vm/Interpreter.cpp:612
#2  0x0000557122d28ed6 in InternalCall (cx=0x461f4e21fa20 <_IO_stdfile_2_lock>, cx@entry=0x21152572e100, 
    args=..., reason=627694000, reason@entry=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:647
#3  0x0000557122d290c3 in js::Call (cx=0x21152572e100, fval=fval@entry=..., thisv=thisv@entry=..., args=..., 
    rval=..., reason=reason@entry=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:679
#4  0x0000557122e4f5ec in js::Call (cx=0x461f4e21fa20 <_IO_stdfile_2_lock>, fval=..., thisObj=<optimized out>, 
    arg0=..., arg1=..., rval=...) at js/src/vm/Interpreter.h:141
#5  0x00005571235cbbd4 in js::ScriptedProxyHandler::getOwnPropertyDescriptor (this=<optimized out>, 
    cx=0x461f4e21fa20 <_IO_stdfile_2_lock>, proxy=..., id=..., desc=...)
    at js/src/proxy/ScriptedProxyHandler.cpp:545
#6  0x00005571235bfc24 in js::Proxy::getOwnPropertyDescriptor (cx=0x21152572e100, proxy=..., id=..., desc=...)
    at js/src/proxy/Proxy.cpp:221
#7  0x000055712307931c in js::GetOwnPropertyDescriptor (cx=cx@entry=0x21152572e100, obj=obj@entry=..., 
    id=id@entry=..., desc=desc@entry=...) at js/src/vm/JSObject.cpp:2032
#8  0x00005571235d2455 in js::ScriptedProxyHandler::checkGetTrapResult (cx=0x21152572e100, target=..., id=..., 
    trapResult=...) at js/src/proxy/ScriptedProxyHandler.cpp:1191
#9  0x0000557123c1cdb0 in js::jit::CheckProxyGetByValueResult (cx=0x21152572e100, obj=..., idVal=..., 
    value=..., result=...) at js/src/jit/VMFunctions.cpp:1649
#10 0x00005eae5593b443 in ?? ()
#11 0x0000000000000000 in ?? ()

Comment 1

[lukas.bernhard]

Attachment: .mozconfig

Comment 2

[lukas.bernhard]

https://pernos.co/debug/vsR-8z6z47-NV8ivS54ZiQ/index.html

Comment 3

[Iain Ireland [:iain]]

Based on the pernosco recording, it looks like the problem is that we give MCheckScriptedProxyGetResult a custom alias set saying that it can only write to the ExceptionState, but inside ScriptedProxyHandler::checkGetTrapResult, we call GetOwnPropertyDescriptor, which can call arbitrary code if the target is a proxy. We catch this using this code.

We don't attach this stub unless the target of the proxy is a native object, but at first glance it looks like we don't actually guard for that while executing the stub. I think we might be able to fix this by adding a GuardIsNativeObject.

I think this problem was already present in bug 1824051. I can't think of a reason off the top of my head that bug 1850744 would have made it any worse.

I'm going to start by trying to write a testcase.

Comment 4

[Iain Ireland [:iain]]

This testcase reproduces the crash:

// |jit-test| --fast-warmup

function foo(o) {
  return o.x;
}

with ({}) {}

var handler = {
  get: (target, prop) => { return 1; },
  getOwnPropertyDescriptor: (target, prop) => { return Object.getOwnPropertyDescriptor(target, prop); }
}

var o = {};
Object.defineProperty(o, 'x', { value: 1, configurable: false, writable: false });

var proxy = new Proxy(o, handler);
for (var i = 0; i < 50; i++) {
  foo(proxy);
}

var proxy_proxy = new Proxy(proxy, handler);
foo(proxy_proxy);

And I can confirm that adding a GuardIsNativeObject fixes it.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1853180: Don't support proxy targets in ScriptedProxy stub r=jandem!

Comment 6

[Jan de Mooij [:jandem]]

Setting the "Regressed by" field to help with release tracking.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1824051

Comment 8

[Iain Ireland [:iain]]

This kind of bug often ends up being exploitable, but in this case I think we get lucky. We only generate this node immediately after a call, so alias analysis will already assume that everything has been clobbered, even though MCheckScriptedProxyGetResult has the wrong alias set. That significantly reduces the scope for something to go wrong. We would have to move some sort of vulnerable instruction in between the call and the check; I can't think of a way to make that happen.

I think sec-moderate is appropriate here.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29fa1526c9bf
Don't support proxy targets in ScriptedProxy stub r=jandem

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/29fa1526c9bf

Comment 11

[Donal Meehan [:dmeehan]]

:iain is this safe to uplift to beta for Fx118? RC week is next week

Comment 12

[Iain Ireland [:iain]]

Comment on attachment 9353296 [details]
Bug 1853180: Don't support proxy targets in ScriptedProxy stub r=jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Potential miscompilation (incorrect alias set) in a case where I don't think it can be exploited, but it's probably best to uplift just to be sure.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a simple guard.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9353296 [details]
Bug 1853180: Don't support proxy targets in ScriptedProxy stub r=jandem!

Approved for landing on mozilla-beta before the merge, will be in the 118.0 release candidate, thanks.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/287241ce9ed0

Comment 15

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons