Closed Bug 2045307 Opened 5 months ago Closed 5 months ago

Differential behavior: Ion-inlined WeakMap.prototype.get misses incremental read barrier for atoms-zone values (emitWeapMapBarrierFastPath consults compile-realm zone, not cell zone)

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

Product:
Core
Component:
JavaScript Engine: JIT
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
153 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr140 --- unaffected
firefox151 --- wontfix
firefox152 + fixed
firefox153 + fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [adv-main152+r])

Attachments

(3 files, 3 external files)

poc.js — testcase (run with --fuzzing-safe; compare default vs --no-ion)
3.25 KB, text/plain
Details
probe1.js — exploratory probe used during analysis
618 bytes, text/plain
Details
probe2.js — exploratory probe used during analysis
1.71 KB, text/plain
Details

Summary

The Ion fast path for the inlined WeakMap.prototype.get decides whether the value read barrier is needed based on the compilation realm's zone, while the VM implementation (WeakMap::valueReadBarrier → JS::ExposeValueToActiveJS → ExposeGCThingToActiveJS) bases the decision on the zone of the cell being returned.

WeakMap values that are atomized strings live in the atoms zone, not the script's zone. During an incremental GC whose zone set includes the atoms zone but not the script's zone, the atoms zone has needsMarkingBarrier() == true while the script's zone does not. In that window, Ion-compiled wm.get(k) hands an unmarked (white) atoms-zone cell to the running script without ever calling PerformIncrementalReadBarrier, violating the engine's read-barrier contract:

  • js/public/HeapAPI.h:883 — ExposeGCThingToActiveJS is documented as having to be "kept in sync" with js::jit::WeakMapValueReadBarrier; it uses the cell's zone for the needsMarkingBarrier() check.
  • js/src/gc/AtomMarking.cpp:356 (AtomMarkingRuntime::markAtom) — the read barrier on the atom "is necessary if the atom is being marked because a reference to it was obtained from another zone which is not being collected by the incremental GC."

A missed incremental read barrier is the classic GC use-after-free bug class: the mutator obtains and stores a reference to a cell the collector considers unreachable-so-far. This is the same bug class that bug 2034837 / bug 2042465 closed for symbols; the non-symbol atoms-zone case (atom strings) remains broken because the wrong zone's needsMarkingBarrier flag is consulted.

Root cause

Introduced by commit 783dd8752bd2 (bug 2034837, "Call MarkSymbolForWeakMapReadBarrier in JIT path") together with follow-up 1a134015e8ae (bug 2042465). The series replaced the cell-zone-driven js::jit::ReadBarrier(gc::Cell*) with compile-realm-zone wiring:

  1. js/src/jit/CompileWrappers.cpp:170 — CompileZone::addressOfZone() exposes the compilation realm's zone pointer; js/src/jit/CodeGenerator.cpp:22505 bakes it in as an immediate (masm.movePtr(ImmPtr(mirGen().realm->zone()->addressOfZone()), ...)) and passes it to js::jit::WeakMapValueReadBarrier(cell, mapZone).
  2. js/src/jit/MacroAssembler.cpp:8355 — MacroAssembler::emitWeapMapBarrierFastPath is the inline filter deciding whether to call the out-of-line barrier. For a white cell it only calls the barrier if branchTestNeedsMarkingBarrierAnyZone(...) fires (js/src/jit/MacroAssembler.cpp:8415).
  3. js/src/jit/MacroAssembler-inl.h:788 — despite its name, branchTestNeedsMarkingBarrierAnyZone tests (when compiling Ion code) only the compile realm zone's barrier word, via CompileZone::addressOfNeedsMarkingBarrier() (js/src/jit/CompileWrappers.cpp:143).

The VM path checks the cell's zone instead (js/src/gc/WeakMap.h:528 valueReadBarrier → ExposeGCThingToActiveJS, js/public/HeapAPI.h:883).

Divergent configuration:

  • value cell zone = atoms zone (an atomized string; symbols are special-cased, objects/bigints/non-atom strings are always in the script's zone),
  • incremental GC marking a zone set that includes the atoms zone but not the script's zone,
  • wm.get(k) executed in Ion.

Then needsMarkingBarrier(atoms) == true but needsMarkingBarrier(scriptZone) == false, so the Ion fast path falls through to "no barrier" and returns the white cell. Baseline/interpreter (CacheIRCompiler::emitWeakMapGetObjectResult → WeakMapObject::getObject → barriered WeakMap::lookup) marks the cell black.

Note: the out-of-line helper js::jit::WeakMapValueReadBarrier (js/src/jit/VMFunctions.cpp:3448) is itself correct (it re-reads cell->zone()); the bug is purely that the inline fast path filters out the call using the wrong zone's flag.

Consequence

At sweep start, GCRuntime::updateAtomsBitmap (js/src/gc/Sweeping.cpp:1393) first refines every collected zone's atom marking bitmap from the chunk mark bits and only afterwards ORs in the bitmaps of uncollected zones. Because Ion skipped the read barrier, the atom is still white at refinement time, so a collected zone that legitimately had the atom in its marking bitmap has it wrongly removed. The mutator runs with a reference to a white atoms-zone cell for the whole remainder of the marking phase; the cell's survival hinges solely on the redundant snapshot rescue of uncollected-zone bitmaps. Any state where that redundancy does not hold turns this into a use-after-free of an atoms-zone cell (dangling JSString* reachable from live JS).

Reproduction

Testcase poc.js (attached). It observes the divergence deterministically via the --fuzzing-safe testing functions getAtomMarkIndex/getAtomMarkColor (testing functions are used only to observe mark state and make the zonal incremental GC deterministic — the missed barrier itself needs no testing functions):

$ js --fuzzing-safe poc.js          # Ion enabled (default)
atom color in zone A after GC: white
atom color in this zone after GC: black
poc.js:82:9 Error: Assertion failed: got "white", expected "black"
(exit code 3)

$ js --fuzzing-safe --no-ion poc.js
atom color in zone A after GC: black
atom color in this zone after GC: black
TEST PASSED
(exit code 0)

Reproduces identically on the debug ASan shell (m-c-asan-debug) and the opt ASan fuzzing shell (m-c-fuzzing-asan-opt); also with --ion-eager --ion-offthread-compile=off vs --no-ion. Stable 10/10 runs.

Shell version: product_version = 20260603-5cffec5992d3 (mozilla-central, Linux x86_64 ASan builds).

The differential proves the Ion path skips PerformIncrementalReadBarrier on a weakmap value whose zone has needsMarkingBarrier() set — a missing GC read barrier in JIT code, not merely a bookkeeping difference: with --no-ion the same execution leaves the cell black-marked during marking.

Suggested fix

In MacroAssembler::emitWeapMapBarrierFastPath, the white-cell fallback must consult the cell's zone (load the zone from the cell, as ExposeGCThingToActiveJS does), or conservatively also branch to the out-of-line barrier when the value is a (non-permanent) atoms-zone string — mirroring the existing branchTestSymbol special case — or test a runtime-wide "any zone needs marking barrier" flag instead of the compile-realm zone's flag. The out-of-line WeakMapValueReadBarrier already handles all cases correctly.

Files

  • poc.js — the testcase (run with --fuzzing-safe; compare default vs --no-ion).
  • probe1.js, probe2.js — exploratory probes used during analysis.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Blocks: sm-security
Severity: -- → S2
Priority: -- → P1
Keywords: regression
Regressed by: 2000328

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

status-firefox151: --- → affected
status-firefox152: --- → affected
status-firefox153: --- → affected
status-firefox-esr140: --- → unaffected
Attached file (secure) —
Attached file (secure) —

Comment on attachment 9595417 [details]
(secure)

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Pretty difficult to set up the right state but maybe possible now with AI?
  • 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
  • If not all supported branches, which bug introduced the flaw?: Bug 2000328
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Should apply or easy to backport.
  • How likely is this patch to cause regressions; how much testing does it need?: Pretty unlikely, it's a small change.
  • Is the patch ready to land after security approval is given?: Yes
  • Is Android affected?: Yes
Attachment #9595417 - Flags: sec-approval?
Attachment #9595417 - Flags: sec-approval? → sec-approval+
Attached file (secure) —

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

Attachment #9595724 - Flags: approval-mozilla-beta?

firefox-beta Uplift Approval Request

  • User impact if declined/Reason for urgency: Potential crashes or security bugs.
  • 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: Small and local fix.
  • String changes made/needed?: N/A
  • Is Android affected?: yes
Attachment #9595724 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out from Beta for bustage. Jan, it appears this depends on bug 2004011 which had to be backed out from Beta. Can you please post a reworked patch?
https://treeherder.mozilla.org/logviewer?job_id=571778675&repo=mozilla-beta&task=e0z8fQmBQr-l8zvRlh6NDw.0&lineNumber=9236

status-firefox152: fixed → affected
Flags: needinfo?(jdemooij)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)

Can you please post a reworked patch?

I've updated the patch.

Flags: needinfo?(jdemooij)

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

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
status-firefox153: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 153 Branch
QA Whiteboard: [sec] [uplift] [qa-triage-done-c153/b152]
Whiteboard: [adv-main152+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: