2042295 - [JSPI] Assertion failure: this->is<T>(), at js/src/vm/JSObject.h:527 with AddPromiseReactions

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Summary

A WebAssembly.Suspending import that returns a cross-compartment wrapper around a Promise whose .constructor (read through the wrapper) equals the suspending realm's Promise constructor causes js::wasm::AddPromiseReactions to perform an unchecked as<PromiseObject>() cast on the wrapper.

• Debug: Assertion failure: this->is<T>(), at js/src/vm/JSObject.h:527, synchronously while calling the promising wrapper (no job-queue drain involved).
• Opt/Nightly (MOZ_ASSERT compiled out): a ProxyObject (CrossCompartmentWrapper) is static_cast to PromiseObject* and handed to JS::AddPromiseReactions — type confusion.

JSPI is enabled by default on Nightly, so this is reachable from web content.

Root cause

The suspending side computes the awaited promise via js::PromiseResolve (Promise.cpp CommonStaticResolveImpl). When the argument is a CCW that unwraps to a Promise and its constructor property (read through the wrapper) equals the suspending realm's Promise constructor, the function returns the wrapper itself rather than adopting into a fresh same-compartment promise. That wrapper then flows into the promising module's resume handler as promiseRef.

AddPromiseReactions (WasmPI.cpp) casts the incoming ref directly:

Rooted<PromiseObject*> promiseObject(
    cx,
    &AnyRef::fromCompiledCode(promiseRef).toJSObject().as<PromiseObject>());

The sibling GetPromiseResults (WasmPI.cpp:1248-1257) — which reads the same $promise value after resume — already handles this with IsWrapper + UncheckedUnwrap (with a JS_IsDeadWrapper check) before the cast. AddPromiseReactions is missing the equivalent guard.

The other refs (contRef, reactionRef, promisingPromiseRef) are produced inside the module by ref.func / cont.new / create-promise and aren't attacker-influenced; only promiseRef is reachable with a wrapper.

Test case

var gB = newGlobal({newCompartment: true});
gB.AProm = Promise;
var w = gB.evaluate(`var p = Promise.resolve(0); p.constructor = AProm; p;`);

var sf = new WebAssembly.Suspending(() => w);
var ins = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`(module
  (import "" "f" (func $f (result externref)))
  (func (export "g") (result externref) (call $f)))`)), {"": {f: sf}});

WebAssembly.promising(ins.exports.g)();

Crashes on debug:

Assertion failure: this->is<T>(), at js/src/vm/JSObject.h:527

Suggested fix

Mirror what GetPromiseResults already does on the same value — IsWrapper + UncheckedUnwrap with a JS_IsDeadWrapper check before as<PromiseObject>():

--- a/js/src/wasm/WasmPI.cpp
+++ b/js/src/wasm/WasmPI.cpp
@@ AddPromiseReactions (~line 1332)
   MOZ_ASSERT(SASigAddPromiseReactions.failureMode == FailureMode::FailOnNegI32);
   JSContext* cx = instance->cx();
-  Rooted<PromiseObject*> promiseObject(
-      cx,
-      &AnyRef::fromCompiledCode(promiseRef).toJSObject().as<PromiseObject>());
+  JSObject* promiseObj = &AnyRef::fromCompiledCode(promiseRef).toJSObject();
+  if (IsWrapper(promiseObj)) {
+    promiseObj = UncheckedUnwrap(promiseObj);
+    if (JS_IsDeadWrapper(promiseObj)) {
+      JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_DEAD_OBJECT);
+      return -1;
+    }
+  }
+  Rooted<PromiseObject*> promiseObject(cx, &promiseObj->as<PromiseObject>());
   Rooted<ContObject*> contObject(
       cx, &AnyRef::fromCompiledCode(contRef).toJSObject().as<ContObject>());

Equivalently, accept the promise as a plain RootedObject and let JS::AddPromiseReactions unwrap (the shape the bug 2021780 fix used in the pre-cont design).

Comment 2

[Yury Delendik (:yury)]

It is also duplicate of bug 2021780, which was fixed and its test (D287216) is failing to. We just did not commit it due to bug 2021780 being sec-high. Most likely we missed during bug 2023217 refactoring

Comment 3

[Yury Delendik (:yury)]

Attachment: (secure)

Comment 4

[BugBot [:suhaib / :marco]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:yury, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c1450bc54340
https://hg.mozilla.org/integration/autoland/rev/177126498103
Detect dead cross-compartment promise in JSPI. r=rhunt

Comment 6

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

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

Comment 7

[Yury Delendik (:yury)]

I'll block bug that released the feature in nightly, and will request uplift

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: We were attempting to release the JSPI feature in FF152 (see 2015877). It will be nice to uplift sec-high there
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The fix was already present at some point but was removed during refactoring
• String changes made/needed?: n/a
• Is Android affected?: yes

Comment 9

[Yury Delendik (:yury)]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D302908

Comment 10

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/84d32e19e349
https://hg.mozilla.org/releases/mozilla-beta/rev/1de77fa39557