Closed Bug 2061775 Opened 3 months ago Closed 3 months ago

Atom-marking bitmap left Gray for black-referenced Symbols marked via WeakMap paths, causing persistent black->gray invariant violations [@ WeakMap::markEntry / MarkingTracerT::markEphemeronEdges]

Categories

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

defect

Tracking

()

RESOLVED FIXED
156 Branch
Tracking Status
firefox-esr115 --- unaffected
firefox-esr140 --- unaffected
firefox-esr153 155+ fixed
firefox154 --- wontfix
firefox155 + fixed
firefox156 + fixed

People

(Reporter: tjr, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main155+r][adv-esr153.2+r])

Attachments

(3 files, 3 external files)

The per-zone atom reference bitmap (zone->referencedAtoms(), maintained by js/src/gc/AtomMarking.cpp) must over-approximate the color of every atoms-zone cell a zone references. For Symbols the bitmap is color-sensitive: GCRuntime::isSymbolReferencedByUncollectedZone (js/src/gc/AtomMarking-inl.h:121-146) reads the black bit to decide, during a GC that collects the atoms zone but not a given zone, whether a WeakMap Symbol key (and hence the entry's value) must be marked black (js/src/gc/WeakMap-inl.h:219-230).

The typed marking path maintains this invariant by upgrading the source zone's bitmap when black-marking a Symbol (MarkingTracerT::markAndTraverseEdge, js/src/gc/Marking.cpp:1276-1281, and onEdge, js/src/gc/Marking.cpp:929-934, the latter only when a tracing source is set). The WeakMap marking paths do not: WeakMap::markEntry marks Symbol keys and values with generic TraceEdge (js/src/gc/WeakMap-inl.h:225 and :256) from markZoneIteratively / enterWeakMarkingMode / markDeferredWeakMapChildren (js/src/gc/WeakMap.cpp:167-172, js/src/gc/Marking.cpp:1514-1519, 2813-2821), none of which sets a tracing source, so onEdge sees tracingZone() == nullptr and skips the fix-up; MarkingTracerT::markEphemeronEdges (js/src/gc/Marking.cpp:784-786) uses a raw markAndTraverse with no source information, and its caller clears the tracing source (js/src/gc/Marking.cpp:852). Because AtomRefRuntime::refineZoneBitmapForCollectedZone (js/src/gc/AtomMarking.cpp:260-270) only bitwise-ANDs the zone bitmap with the final mark bits, a bitmap entry once lowered to Gray can never regain its Black bit through these paths: the under-approximation is persistent across GCs.

Consequence, demonstrated by the testcase on an unmodified ASAN fuzzing build: after one GC in which the Symbol is legitimately gray, a subsequent GC re-blackens it purely through WeakMap paths, leaving zones holding black edges to the Symbol with Gray bitmap entries (PROOF 1). A later GC that collects the atoms zone but not the referencing zone then gets a false answer from isSymbolReferencedByUncollectedZone for black, and the Symbol plus the WeakMap value keyed by it finish the GC Gray while still black-reachable from live script (PROOF 2) — a black->gray marking invariant violation. Gray is the state the browser's cycle collector uses to identify potentially collectable JS, so script-reachable objects are presented to the CC as candidate garbage; this is the standard precursor state for use-after-free. SpiderMonkey itself treats the state as a hard violation: on a debug build the same testcase aborts at GC2 with 'Color of zone reference to symbol is less than symbol key ... (key is black, bitmap is gray); Assertion failure: WeakMapBase::checkMarkingForZone(zone), at js/src/gc/Sweeping.cpp:1262' (MOZ_RELEASE_ASSERT in MaybeCheckWeakMapMarking, backed by the 'References to symbol keys must be recorded in the zone' check at js/src/gc/Verifier.cpp:1236-1249). That checker is compiled out of non-DEBUG builds without the CheckWeakMapMarking zeal mode (js/src/gc/Sweeping.cpp:1250-1268), so shipping builds silently keep the corrupted state.

Build Info

Affected Code

File: js/src/gc/WeakMap-inl.h, line 248-262

  gc::Cell* cellValue = gc::ToMarkable(value);
  if (IsMarked(keyColor)) {
    if (cellValue) {
      CellColor targetColor = std::min(mapColor, keyColor);
      CellColor valueColor = gc::detail::GetEffectiveColor(marker, value.get());
      if (valueColor < targetColor) {
        MOZ_ASSERT(markColor >= targetColor);
        if (markColor == targetColor) {
          TraceEdge(trc, &value, "WeakMap entry value");  // generic path; no source zone -> onEdge skips MaybeUnmarkGraySymbol
          MOZ_ASSERT(cellValue->color() >= targetColor);
          marked = true;
        }
      }
    }
  }

File: js/src/gc/Marking.cpp, line 776-787

  for (auto& edge : edges) {
    if (!edge.target()) {
      continue;
    }

    MarkColor targetColor = std::min(srcColor, MarkColor(edge.color()));
    MOZ_ASSERT(markColor() >= targetColor);
    if (targetColor == markColor()) {
      ApplyGCThingTyped(edge.target(), edge.target()->getTraceKind(),
                        [this](auto t) { this->markAndTraverse(t); });  // raw mark; bypasses onEdge/markAndTraverseEdge Symbol fix-up
    }
  }

File: js/src/gc/Marking.cpp, line 929-934 and 1276-1281

  // onEdge: fix-up only when a tracing source was recorded
  if constexpr (std::is_same_v<T, JS::Symbol>) {
    Zone* zone = tracingZone();
    if (markColor() == MarkColor::Black && zone) {
      MaybeUnmarkGraySymbol(this->runtime(), zone, thing);
    }
  }

  // markAndTraverseEdge (typed path): always fixes the source zone's bitmap
  if constexpr (std::is_same_v<T, JS::Symbol>) {
    if (markColor() == MarkColor::Black) {
      Zone* zone = source->asTenured().zone();
      MaybeUnmarkGraySymbol(this->runtime(), zone, target);
    }
  }

File: js/src/gc/AtomMarking.cpp, line 260-270

void AtomRefRuntime::refineZoneBitmapForCollectedZone(
    Zone* zone, const DenseBitmap& bitmap) {
  MOZ_ASSERT(zone->isCollectingFromAnyThread());
  MOZ_ASSERT(!zone->isAtomsZone());

  // Compute the intersection of the given mark bitmap and the zone's reference
  // set ... (purely subtractive: can never restore a lost Black bit)
  zone->referencedAtoms().bitwiseAndWith(bitmap);
}

File: js/src/gc/AtomMarking-inl.h, line 121-146

inline bool GCRuntime::isSymbolReferencedByUncollectedZone(JS::Symbol* sym,
                                                           MarkColor color) {
  ...
  const DenseBitmap& bitmap = *atomsUsedByUncollectedZones.ref();
  ...
  if (bitmap.getBit(blackBit)) {
    return true;
  }
  return color == MarkColor::Gray && bitmap.getBit(grayOrBlackBit);
}  // answers false-for-black when the bitmap was left Gray, so WeakMap::markEntry
   // (WeakMap-inl.h:219-230) refuses to blacken a black-referenced Symbol key

The WeakMap marking paths black-mark Symbols without the bitmap fix-up that the typed marking path performs, and the refine step can only lower, never raise, the recorded color.

Exploit Chain

  1. Create zone W with a WeakMap wm and key object; create a Symbol sym; store sym as wm's value under key (W's only reference to sym is the weakmap entry). In the main zone, create wm2 keyed by sym with an ordinary object O as value; drop all direct references to sym and O.
  2. Make key reachable only through W's gray roots and run a full GC (GC1): sym legitimately ends Gray, and refineZoneBitmapForCollectedZone lowers both zones' atom-bitmap entries for sym to Gray.
  3. Make key black-reachable again and run a full GC (GC2): the black map x black key entry re-blackens sym, but only through WeakMap::markEntry's generic TraceEdge and markEphemeronEdges' raw markAndTraverse, so neither zone's bitmap Black bit is restored. State: black cell, Gray bitmaps (PROOF 1; debug builds abort here on SpiderMonkey's own MOZ_RELEASE_ASSERT).
  4. Trigger a GC that collects the atoms zone and the main zone but not W (GC3): isSymbolReferencedByUncollectedZone(sym, Black) reads only W's Gray bitmap bit and answers false, so wm2's Symbol key is not blackened; the gray-phase fallback markIncomingGraySymbolEdgesFromUncollectedZones (js/src/gc/Sweeping.cpp:700-729) marks sym and, via the ephemeron edge, O only Gray.
  5. End state: sym and O finish the GC Gray while black-reachable from live script (W.wm.get(key) -> sym; wm2.get(sym) -> O), a black->gray invariant violation. Gray-only marking is what the browser's cycle collector uses to identify collectable JS, so a still-reachable object is presented to the CC as candidate garbage — the precursor state for freeing an attacker-chosen object while script can still reach it (use-after-free).

Steps to Reproduce

  1. Build the ASAN fuzzing shell: build_firefox with /app/mozconfigs/mozconfig.linux.asan.fuzzing (objdir /firefox/obj-firefox-asan)
  2. Run: /firefox/obj-firefox-asan/dist/bin/js --fuzzing-safe testcase.js (js_shell_evaluator does this automatically)
  3. Observe both sentinel lines in stderr: 'GC-INVARIANT-VIOLATION PROOF 1' (black cell vs Gray bitmap after GC2) and 'GC-INVARIANT-VIOLATION PROOF 2' (Symbol and black-reachable weakmap value left Gray by a GC that does not collect W). No crash is expected on this build; the release-assert checker is compiled out.
  4. Corroboration: build the debug shell with /app/mozconfigs/mozconfig.js.linux.debug and run the same testcase; it aborts at GC2 with 'Color of zone reference to symbol is less than symbol key ... (key is black, bitmap is gray)' and 'Assertion failure: WeakMapBase::checkMarkingForZone(zone), at js/src/gc/Sweeping.cpp:1262'.
  5. No prefs, source patches, or external services are required; only fuzzing-safe shell testing functions (getAtomMarkIndex/getAtomMarkColor/getMarks/grayRoot/schedulezone/gc) are used to control GC scheduling and observe mark state.

Security Impact

  • Severity: High
  • Attacker capability: Content-process JavaScript can put the GC's atom-marking bookkeeping into a persistently inconsistent state in which a Symbol and any WeakMap values keyed by it finish a GC marked Gray while still black-reachable from live script (verified end state of the testcase; SpiderMonkey's own release-assert checker classifies the intermediate state as a hard invariant violation). Gray marking is the input the browser's cycle collector uses to decide which JS is collectable, so this is the precursor state for collecting an object that script can still reach — the use-after-free class associated with black->gray violations. The corruption is persistent: the subtractive refineZoneBitmapForCollectedZone (js/src/gc/AtomMarking.cpp:260-270) can never restore the lost Black bit, and every later GC re-blackens the Symbol through the same broken WeakMap paths.
  • Preconditions: None beyond running attacker JavaScript in a content process: Symbols as WeakMap keys and multiple zones are default functionality. The demonstrated sequence needs (a) a GC in which the weakmap key is only gray-reachable, then black-reachable again, and (b) a later GC that collects the atoms zone but not the corrupted zone; the testcase forces these with fuzzing-safe shell functions, while in the browser they correspond to naturally occurring gray roots (cycle-collected wrappers) and routine zone-GC scheduling. Escalating the demonstrated black->gray state to an actual free requires the browser's cycle collector, which is outside the JS shell used here.

ASAN Report

Primary evidence (release semantics, /firefox/obj-firefox-asan/dist/bin/js --fuzzing-safe testcase.js, no crash, stderr):
GC1 (full gc, W's weakmap key only gray-reachable): symbol mark=gray value mark=gray W bitmap=gray main bitmap=gray
GC2 (full gc, W's weakmap key black again): symbol mark=black value mark=black W bitmap=gray main bitmap=gray
GC-INVARIANT-VIOLATION PROOF 1: zone W references the Symbol with a BLACK edge (weakmap value, black map, black key; actual mark color black) but W's atom reference bitmap records GRAY. The bitmap must over-approximate reference color (js/src/gc/AtomMarking.cpp SMDOC).
GC3 (zone gc: atoms + main zone, W uncollected): symbol mark=gray value mark=gray W bitmap=gray main bitmap=gray
GC-INVARIANT-VIOLATION PROOF 2: GC collecting the atoms zone but not W: isSymbolReferencedByUncollectedZone answered false for black, so the Symbol and the weakmap value keyed by it are left GRAY while black-reachable (black->gray violation; gray objects are treated as collectable by the cycle collector).
victim weakmap size=1

Corroboration (debug shell, /firefox/obj-js-debug/dist/bin/js --fuzzing-safe testcase.js — SpiderMonkey's own invariant checker aborts at GC2):
Color of zone reference to symbol is less than symbol key 0x3ca087310020
(key 0x3ca087310020 is black, bitmap is gray)
Assertion failure: WeakMapBase::checkMarkingForZone(zone), at js/src/gc/Sweeping.cpp:1262
    #1 MaybeCheckWeakMapMarking(js::gc::GCRuntime*) js/src/gc/Sweeping.cpp:1262
    #2 js::gc::GCRuntime::endMarkingSweepGroup(JS::GCContext*, JS::SliceBudget&) js/src/gc/Sweeping.cpp:1378
    #5 js::gc::GCRuntime::sweepPhase(JS::SliceBudget&) js/src/gc/Sweeping.cpp:2754
    #6 js::gc::GCRuntime::incrementalSlice(...) js/src/gc/GC.cpp:4487
    #10 JS::NonIncrementalGC(...) js/src/gc/GCAPI.cpp:299
(the abort is the engine's diagnostic release-assert on the corrupted state, not the vulnerability itself; the checker is compiled out of non-DEBUG builds per js/src/gc/Sweeping.cpp:1250-1268)
Attached file crash_stack.txt —
Attached patch fix.patch — — Splinter Review
Attached file testcase.js —

This bug was filed automatically by an AI-powered security testing tool. The analysis, testcase, and any suggested patches were generated by an AI model and have NOT been verified by a human.

Please review all details carefully before triaging. The issue was confirmed by an independent verifier, but for non-ASAN issues, the verifier has been made less strict. Additionally, the root cause analysis and suggested fix may contain errors.

Group: core-security → javascript-core-security

Related to bug 2029769 / bug 2029788 (same atom-marking-bitmap-left-Gray-for-a-black-reachable-Symbol invariant) but a distinct trigger the 2029769 fix does NOT cover -- not a duplicate. That fix upgrades the per-zone atom bitmap only on the typed marking path (MarkingTracerT::onEdge, and only when a tracing source is set). The WeakMap marking paths (WeakMap::markEntry, markEphemeronEdges) mark Symbol keys/values with generic TraceEdge and set no tracing source, so onEdge skips the fix-up; refineZoneBitmapForCollectedZone is subtractive, so the lost Black bit is never restored. SpiderMonkey's own release-assert (WeakMapBase::checkMarkingForZone, js/src/gc/Sweeping.cpp:1262) fires on a debug build, confirming the violation. (No crash on shipping/opt builds -- the checker is compiled out.)

See Also: → 2029769, 2029788
Blocks: sm-security
Severity: -- → S2
Priority: -- → P1
Assignee: nobody → jcoppeard
Depends on: 2042768
Group: javascript-core-security → core-security-release
Status: UNCONFIRMED → RESOLVED
Closed: 3 months ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 156 Branch

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

For more information, please visit BugBot documentation.

Flags: needinfo?(jcoppeard)
Flags: needinfo?(jcoppeard)
Attached file (secure) —
Attachment #9628352 - Flags: approval-mozilla-esr153?

firefox-esr153 Uplift Approval Request

  • User impact if declined/Reason for urgency: Possible crash / security vulnerability.
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: yes
  • Steps to reproduce for manual QE testing: None.
  • Risk associated with taking this patch: low
  • Explanation of risk level: This is a straightforward fix to track the zone when marking in more places, extending the existing system. This has been on central for 5 days without issues.
  • String changes made/needed?: None.
  • Is Android affected?: yes
Flags: qe-verify+

firefox-beta Uplift Approval Request

  • User impact if declined/Reason for urgency: Possible crash / security vulnerability
  • Code covered by automated testing?: yes
  • Fix verified in Nightly?: yes
  • Needs manual QE testing?: no
  • Steps to reproduce for manual QE testing: None
  • Risk associated with taking this patch: low
  • Explanation of risk level: This is a straightforward fix to track the zone when marking in more places, extending the existing system. This has been on central for 5 days without issues.
  • String changes made/needed?: None
  • Is Android affected?: yes
Attachment #9628375 - Flags: approval-mozilla-beta?
Attached file (secure) —
Attachment #9628375 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9628352 - Flags: approval-mozilla-esr153? → approval-mozilla-esr153+

Removing the qe-verify + flag since we consider that manual QA testing is not applicable on this issue, but if there are some STR that can be followed by us in order to reproduce and verify the issue, please add again the flag and the STR. Also this issue is also covered by automated tests.

QA Whiteboard: [sec] [qa-triage-done-c156/b155]
Flags: qe-verify+
Whiteboard: [adv-main155+r][adv-esr153.2+r]
Group: core-security-release
Attachment #9626184 - Attachment description: (secure) → Bug 2061775 - Ensure GC marker knows which zone we're in when marking weakmaps r?sfink
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: