2024918 - Escape analysis bypass via phi in [@ IsWasmStructEscaped] leaves uninitialized anyref in live Wasm GC struct

Status: RESOLVED FIXED

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

Description

[Bugmon [:jkratzer for issues]]

Summary

Type: Uninitialized heap memory → GC type confusion / wild pointer dereference
File: js/src/jit/ScalarReplacement.cpp, line 3900
Root cause: IsWasmStructEscaped is called recursively when walking through phi nodes (IsWasmStructEscaped(phi, newStruct)), but the WasmStoreFieldRef case compares value() against the original newStruct parameter instead of the current iteration cursor ins. When the cursor is a phi and the phi is the value() operand of a store into an escaping struct, the comparison phi == newStruct is always false — the escape is missed. Scalar replacement then discards the struct's init store while the MWasmNewStructObject(zeroFields=false) allocation — which skips field zeroing at MacroAssembler.cpp:7645 — still reaches the heap with raw nursery garbage in a GC-traced anyref slot.

Affected Code

File: js/src/jit/ScalarReplacement.cpp, line 3900

static bool IsWasmStructEscaped(MDefinition* ins, MInstruction* newStruct) {
  ...
  for (MUseIterator i(ins->usesBegin()); i != ins->usesEnd(); i++) {
    ...
    MDefinition* def = consumer->toDefinition();
    switch (def->op()) {
      ...
      case MDefinition::Opcode::WasmStoreFieldRef: {
        // Escaped if it's stored into another struct.
        if (def->toWasmStoreFieldRef()->value() == newStruct) {  // <-- BUG: should compare to `ins`
          JitSpewDef(JitSpew_Escape, "is escaped by\n", def);
          return true;
        }
        break;
      }
      ...
      case MDefinition::Opcode::Phi: {
        auto* phi = def->toPhi();
        if (!WasmStructPhiOperandsEqualTo(phi, newStruct)) { ... return true; }
        if (IsWasmStructEscaped(phi, newStruct)) {           // <-- recursion: ins becomes `phi`
          ... return true;
        }
        break;
      }

Why it's wrong: At line 3883 the loop iterates ins->usesBegin(). Any consumer found there uses ins, not newStruct. In the top-level call ins == newStruct so the check happens to work. But after the recursion at line 3917, ins is the phi — so a WasmStoreFieldRef whose value() is the phi will satisfy value() != newStruct, and the function treats the store as using the phi as its base (safe) when it's actually the value (escaping).

Connection to MIR-wasm.cpp

MWasmStructState at MIR-wasm.cpp:1019-1045 is the state-tracking infrastructure that scalar replacement uses. MWasmStructState::init() sizes fields_ based on wasmStruct_->toWasmNewStructObject()->structType().fields_.length(). The state machine assumes that after SR completes, struct_->hasUses() is false — enforced only by MOZ_ASSERT (debug-only) at ScalarReplacement.cpp:3648. When the escape bypass occurs, MWasmStructState correctly tracks the field values but the MWasmNewStructObject itself leaks into the graph with no init stores and zeroFields=false, violating the invariant that MWasmStructState relies on.

Exploit Chain

1. Wasm source defines three struct types: $Inner (holds anyref), $Outer (holds ref $Inner), $Container (holds ref $Inner, escapes via global).
2. Ion compiles run after warmup. MIR contains:
   • s1 = MWasmNewStructObject($Outer, zeroFields=false)
   • s2 = MWasmNewStructObject($Inner, zeroFields=false)
   • container = MWasmNewStructObject($Container, ...) — escapes via global, never SR-eligible.
   • StoreA = MWasmStoreFieldRef(base=s1, value=s2) before the loop.
   • Inside the loop: Load = MWasmLoadField(base=s1) → StoreB = MWasmStoreFieldRef(base=container, value=Load) → StoreC = MWasmStoreFieldRef(base=s1, value=s2).
3. ScalarReplacement iterates RPO. s1 is defined first, processed first.
4. Escape check for s1 passes: all its uses are as base() of loads/stores.
5. SR on s1 runs. mergeIntoSuccessorState creates Phi1 at the loop header for s1's field (loop has 2 predecessors). StoreA sets state to s2 pre-loop; StoreC sets state to s2 at backedge → Phi1 = phi(s2, s2). visitWasmLoadField replaces Load with Phi1, so StoreB becomes MWasmStoreFieldRef(base=container, value=Phi1). StoreA and StoreC are discarded.
6. Driver loop reaches s2. Escape check IsWasmStructEscaped(s2, s2):
   • s2's init store: value() = i31ref ≠ s2 → safe (s2 is base).
   • s2 feeds Phi1: WasmStructPhiOperandsEqualTo(Phi1, s2) → true (both inputs are s2). Recurse: IsWasmStructEscaped(Phi1, s2).
   • Phi1 is used by StoreB. Line 3900: StoreB->value() == s2? → Phi1 == s2? → FALSE. Escape missed.
   • Returns not escaped. ← Bug triggered.
7. SR on s2 runs: visitWasmStoreFieldRef discards s2's init store. visitPhi replaces Phi1 with s2 → StoreB is now MWasmStoreFieldRef(base=container, value=s2).
8. assertSuccess is a no-op in release — MOZ_ASSERT(!struct_->hasUses()) would fire in debug since StoreB still uses s2.
9. Codegen: wasmNewStructObject with zeroFields=false does wasmBumpPointerAllocate + shape/supertype stores, skips the zeroing loop at line 7645. s2's field 0 contains raw prior nursery contents.
10. Runtime: s2 (uninitialized) is stored into container, container is in global $g. read() dereferences s2->field[0] as anyref → wild pointer. Or GC traces container → s2 → garbage in WasmStructObject::obj_trace → follows garbage as object edge → SEGV in TenuringTracer::onObjectEdge.

Security Impact

Severity: sec-high / sec-critical

Attacker capability: A malicious webpage can deliver wasm with this struct-chaining pattern to the content-process Ion compiler (Wasm GC ships by default). The nursery is a bump-pointer allocator with no reuse poisoning in release; the attacker can spray it with arbitrary 64-bit values before triggering the bug, placing a chosen 64-bit word into the uninitialized anyref slot. Since the GC will subsequently TraceEdge this value as a JSObject*, this is a fake-object primitive — the attacker can point the GC at attacker-controlled memory, leading to arbitrary read/write and code execution in the content process.

Preconditions: None beyond default Ion/Wasm-GC — triggers under --fuzzing-safe with standard tiering (--fast-warmup only accelerates). No user interaction beyond visiting a page.

Differential: --ion-scalar-replacement=off → correct output final: -1052688063. Default → crash.

Suggested Fix

Compare value() against the current iteration cursor ins:

--- a/js/src/jit/ScalarReplacement.cpp
+++ b/js/src/jit/ScalarReplacement.cpp
@@ -3897,7 +3897,7 @@ static bool IsWasmStructEscaped(MDefinition* ins, MInstruction* newStruct) {
       }
       case MDefinition::Opcode::WasmStoreFieldRef: {
         // Escaped if it's stored into another struct.
-        if (def->toWasmStoreFieldRef()->value() == newStruct) {
+        if (def->toWasmStoreFieldRef()->value() == ins) {
           JitSpewDef(JitSpew_Escape, "is escaped by\n", def);
           return true;
         }

Additionally, upgrade assertSuccess at line 3648 from MOZ_ASSERT to MOZ_RELEASE_ASSERT(!struct_->hasUses()) — an escaping non-zeroed struct is always a memory-safety hazard.

Comment 1

[Bugmon [:jkratzer for issues]]

Attachment: Crash stack trace

Comment 2

[Bugmon [:jkratzer for issues]]

Attachment: Testcase: testcase.js

Comment 4

[Jan de Mooij [:jandem]]

Julien, needinfo'ing you since you did work on scalar replacement before.

Comment 5

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Sorry for the mid-air collision, but I wrote up a quick fix for this. The analysis and suggested fix are correct. The regressing bug is bug 1947614.

I "cleaned up" the hard coded '3' constant by changing:

       if (def->indexOf(*i) == 3) {

to:

        if (def->toWasmStoreFieldRef()->value() == newStruct) {

Which is the vulnerable form.

Comment 6

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Attachment: (secure)

Comment 7

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Attachment: Bug 2024918 - wasm: Add test. r?jseward

Comment 8

[Julien Pages [:jpages]]

The analysis and the fix also look good to me.

Comment 9

[BugBot [:suhaib / :marco]]

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 10

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Comment on attachment 9555747 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy for a human. Requires knowledge of Ion/scalar replacement algorithm. Maybe easier for Claude?
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: Beta, release, ESR. Flags are right.
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: One line change.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, it passes all our tests, and just reverts a bad change to a known good previous.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 9555747 [details]
(secure)

sec-approval+ to land now and request uplifts; please wait until May to land the tests

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1c39b127b06b
https://hg.mozilla.org/integration/autoland/rev/5927bf0cbc3d
Fix scalar replacement. r=jseward,jpages

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/5927bf0cbc3d

Comment 14

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:rhunt, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: fakeObj primitive that can lead to arbitrary read/write.
• 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: Reverts an optimization back to a previous good state.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 17

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Attachment: (secure)

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

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: fakeObj primitive that can lead to arbitrary read/write.
• 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: Reverts an optimization back to a previous good state.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 19

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Attachment: (secure)

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

Comment 20

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/79331863ae46
https://hg.mozilla.org/releases/mozilla-beta/rev/1f77dfc560d1

Comment 21

[BugBot [:suhaib / :marco]]

a month ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2026-05-05] .

rhunt, please refer to the original comment to better understand the reason for the reminder.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bdc273a75d37
https://hg.mozilla.org/integration/autoland/rev/4c542e93014e
wasm: Add test. r=jseward

Comment 23

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/4c542e93014e