1939970 - Failure to wrap JS exception as wasm with wasm disabled in realm

Status: NEW

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Nils Bars]

Steps to reproduce:

Version: 4f2372638478a8b66ba467867d764ad6cdb16a5d
Args: js --fuzzing-safe --wasm-compiler=none <test-case>
Test case:

a = `
function b() {
    return newGlobal(b)
}
b.principal = WeakMap
c = b()
with (c) {
    function d() {
        e
    }
    l = wasmTextToBinary(\`(module (import "m" "f" (func $f)) (func (export "test") call $f))\`)
    g = WebAssembly.Module
    h = new g(l)
    i = WebAssembly.Instance
    j = {"f": d}
    k = {"m": j}
    new i(h, k).exports.test()
}
`
evalInWorker(a)

Actual results:

    #0 0x55b27fa983d6 in GetOrWrapWasmException(js::jit::JitActivation*, JSContext*) js/src/wasm/WasmBuiltins.cpp:681:3
    #1 0x55b27fa983d6 in js::wasm::HandleExceptionWasm(JSContext*, js::JitFrameIter&, js::jit::ResumeFromException*) js/src/wasm/WasmBuiltins.cpp:816:40
    #2 0x55b27f80b204 in js::jit::HandleException(js::jit::ResumeFromException*) js/src/jit/JitFrames.cpp:754:7
    #3 0x55b27fa9ad58 in WasmHandleThrow(js::jit::ResumeFromException*) js/src/wasm/WasmBuiltins.cpp:914:3
    #4 0x3ba28a46c7c4  (<unknown module>)

Comment 1

[Yury Delendik (:yury)]

The program trips on the assert that checks if we are left with OOM exception at the end of the Wasm exception handler. The WasmExceptionObject::wrapJSValue function call returned nullptr because the WebAssembly was not enabled, and that how we ended up at this assert.

The newGlobal during evalInWorker does not respect --wasm-compiler=none option, and creates the WebAssembly namespace anyway, e.g. evalInWorker("newGlobal({principal:WeakMap}).WebAssembly.i") will not fail. The WebAssembly module is compiled and executed inside the worker. That's why the exception ending up in the exception handler. If the assert will be removed, the failure/error will be properly handle anyway -- it is not a security issue in this particular place.

Though, there can be other problems related to evalInWorker("g = newGlobal({principal:WeakMap})"), e.g. new global has to respect options provided in the command line. Jan, can there be other security implication from creating a global without respect of preferences/settings?

Comment 2

[Yury Delendik (:yury)]

We have special case for system principal to always enable Wasm https://searchfox.org/mozilla-central/source/js/src/wasm/WasmFeatures.cpp#281 . Bug 1576254 enables Wasm for chrome content. Shall we review this policy? Or try to address possible cases with disabled/enabled wasm globals?

Comment 3

[Jan de Mooij [:jandem]]

A reduced test case below.

This probably isn't a security bug - setting wasmExn to nullptr in HandleExceptionWasm just means we don't look for a Wasm exception handler. Also, unlike the browser, the JS shell doesn't use principals consistently and isSystemOrAddonPrincipal always returns true. I suspect this case can't happen in the browser to begin with.

My two cents:

• The behavior of HandleExceptionWasm and GetOrWrapWasmException isn't clearly defined when multiple realms are involved. IMO that's the root cause and bug 1915254 is a similar bug.
• I agree with Yury that it's a little surprising that system-or-addon principals override the main is-wasm-enabled pref. For the JS shell we could change --wasm-compiler=none to disable the wasm-for-trusted-principals pref also.

// --wasm-compiler=none
evalInWorker(`
  function throwErr() { throw new Error("hi"); }
  var g = newGlobal({principal:{}});
  var wasm = wasmTextToBinary(\`(module (import "m" "f" (func $f)) (func (export "test") call $f))\`);
  var h = new g.WebAssembly.Module(wasm);
  new g.WebAssembly.Instance(h, {"m": {"f": throwErr}}).exports.test();
`);

Comment 4

[Ryan Hunt [:rhunt]]

I agree this isn't a security bug, and this is the same root cause of bug 1915254. I have a patch to fix this.

Comment 5

[Ryan Hunt [:rhunt]]

This turned out to be harder to solve than I hoped. My patches only solved simple cases, but not the general case.