Closed Bug 2029788 Opened 7 months ago Closed 7 months ago

WeakMap value under-marking with Symbol key referenced by uncollected zone leads to UAF

Categories

(Core :: JavaScript: GC, defect)

defect

Tracking

()

RESOLVED DUPLICATE of bug 2028416

People

(Reporter: bugmon, Unassigned)

References

Details

(5 keywords)

Attachments

(3 files)

WeakMap::markEntry contains a stale-local-variable bug in the Symbol-key handling path. When a Symbol key is initially White but is referenced (gray) by an uncollected zone, the code marks the Symbol via TraceEdge but fails to update the local keyColor variable or set the marked flag. The subsequent value-marking check at if (IsMarked(keyColor)) therefore reads the stale White color and skips marking the WeakMap value entirely.

The code comment claims this is safe because the value 'would have been marked later anyway' — but this assumption fails when incrementalWeakMapMarkingEnabled is false and weak marking mode has been aborted. In that configuration, populateWeakKeysTable is false (no ephemeron edge is added as a backstop) and marked stays false (so markZoneIteratively returns false and the GC fixed-point loop terminates). No mechanism remains to mark the value.

The result violates the core WeakMap marking invariant: the entry survives sweeping (key is gray, map is black) but the value is White and gets finalized. In debug/zeal builds this is caught by MOZ_RELEASE_ASSERT(WeakMapBase::checkMarkingForZone(zone)). In production builds the check is compiled out, leaving the WeakMap holding a dangling pointer to freed memory. Subsequent WeakMap.get(symbol) returns a JS::Value pointing to a swept GC cell — a use-after-free that, with heap grooming to reallocate the slot, yields type confusion and arbitrary read/write. The symbols-as-weakmap-keys feature defaults to enabled in Firefox.

Build Info

Affected Code

File: js/src/gc/WeakMap-inl.h, line 215-230

  bool marked = false;
  CellColor markColor = AsCellColor(marker->markColor());
  CellColor keyColor = gc::detail::GetEffectiveColor(marker, key.get());

  bool keyIsSymbol = gc::detail::IsSymbol(key.get());
  MOZ_ASSERT(keyIsSymbol == (keyCell->getTraceKind() == JS::TraceKind::Symbol));
  if (keyIsSymbol && keyColor < markColor) {
    // For symbols, also check whether it it is referenced by an uncollected
    // zone, and if so mark it now. There's no need to set |marked| as this
    // would have been marked later anyway.       <-- WRONG: not true in iterative-fallback path
    auto* sym = static_cast<JS::Symbol*>(keyCell);
    gc::GCRuntime* gc = &marker->runtime()->gc;
    if (gc->isSymbolReferencedByUncollectedZone(sym, marker->markColor())) {
      TraceEdge(trc, &key, "WeakMap symbol key");   // Marks symbol Gray
      // BUG: keyColor NOT updated -> stays White
      // BUG: marked NOT set -> markZoneIteratively won't iterate again
    }
  }

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

  gc::Cell* cellValue = gc::ToMarkable(value);
  if (IsMarked(keyColor)) {        // <-- IsMarked(White) == false, block skipped!
    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");  // <-- NEVER REACHED
          MOZ_ASSERT(cellValue->color() >= targetColor);
          marked = true;
        }
      }
    }
  }

File: js/src/gc/WeakMap.cpp, line 165-175

bool WeakMapBase::markZoneIteratively(JS::Zone* zone, GCMarker* marker) {
  MOZ_ASSERT(zone->isGCMarking());

  bool markedAny = false;
  ForAllWeakMapsInZone(zone, [&](WeakMapBase* map) {
    if (map->isMarked() && map->markEntries(marker)) {
      markedAny = true;   // <-- stays false because markEntry returns false (the bug)
    }
  });
  return markedAny;       // <-- false -> GC fixed-point loop exits without marking value
}

File: js/src/gc/WeakMap.cpp, line 141-155

#if defined(JS_GC_ZEAL) || defined(DEBUG)
bool WeakMapBase::checkMarkingForZone(JS::Zone* zone) {
  // This is called at the end of marking.
  MOZ_ASSERT(zone->isGCMarking());

  bool ok = true;
  ForAllWeakMapsInZone(zone, [&ok](WeakMapBase* map) {
    if (map->isMarked() && !map->checkMarking()) {
      ok = false;   // <-- detects: map=black, key=gray, value=white
    }
  });

  return ok;
}
#endif

File: js/src/gc/Sweeping.cpp, line 1245-1262

static inline void MaybeCheckWeakMapMarking(GCRuntime* gc) {
#if defined(JS_GC_ZEAL) || defined(DEBUG)   // <-- compiled OUT in release builds!

  bool shouldCheck;
#  if defined(DEBUG)
  shouldCheck = true;
#  else
  shouldCheck = gc->hasZealMode(ZealMode::CheckWeakMapMarking);
#  endif

  if (shouldCheck) {
    for (SweepGroupZonesIter zone(gc); !zone.done(); zone.next()) {
      MOZ_RELEASE_ASSERT(WeakMapBase::checkMarkingForZone(zone));
    }
  }
#endif
}

The root cause is in markEntry: after TraceEdge marks the Symbol key, the cached keyColor local is not refreshed and marked is not set. The value-marking guard then short-circuits on the stale White color. Without the ephemeron-table backstop (disabled when populateWeakKeysTable=false) and without the marked flag triggering another fixed-point iteration in markZoneIteratively, no other code path will mark the value. The fix at HEAD adds keyColor = markColor; marked = true; after TraceEdge.

Exploit Chain

  1. Attacker creates a unique Symbol and arranges for it to be referenced gray-only by a zone that will not be collected in the next GC (e.g., held only via a weak gray edge such as a cross-compartment wrapper preserved by the cycle collector). The first GC refines the atom marking bitmap, leaving only the gray bit set for this Symbol in the uncollected zone.
  2. Attacker creates a WeakMap in a separate compartment/zone and inserts an entry: wm.set(symbol, victimObject). The victimObject has no other strong references.
  3. Attacker drops all black references to the Symbol so its only liveness is the gray reference from the uncollected zone.
  4. A zone GC runs that collects the atoms zone and the WeakMap's zone, but excludes the zone holding the gray reference. The atomsUsedByUncollectedZones bitmap records the symbol as gray-referenced (only grayOrBlackBit set, not blackBit).
  5. During gray weak marking, GC enters the iterative WeakMap marking fallback (weak marking mode aborted — this can occur organically via abortLinearWeakMarking on OOM in addEphemeronEdgesForEntry).
  6. markZoneIteratively (WeakMap.cpp:165) calls markEntries which calls markEntry: keyColor reads White, isSymbolReferencedByUncollectedZone(sym, Gray) returns true, TraceEdge marks the symbol Gray. keyColor stays White (the bug). IsMarked(White)=false, value marking skipped. populateWeakKeysTable=false so no ephemeron edge added. marked=false, so markZoneIteratively returns false, no further iteration.
  7. GC sweep phase: Symbol survives (gray), WeakMap survives (black), entry survives (key is marked) — but victimObject is White and gets finalized. Its memory is returned to the GC arena freelist.
  8. Attacker sprays GC allocations of the same size class to reclaim the freed slot with attacker-controlled bytes (e.g., a fake object header pointing to a forged Shape with a malicious getter).
  9. Attacker calls wm.get(symbol). The WeakMap returns a JS::Value containing the dangling pointer. The engine treats the reclaimed bytes as a live JSObject -> type confusion.
  10. Property access on the fake object triggers attacker-controlled vtable/shape dispatch -> arbitrary read/write primitive -> renderer code execution.

Steps to Reproduce

  1. Build SpiderMonkey debug shell: ./mach build (debug configuration with --enable-debug)
  2. Save the testcase as weakmap-symbol-undermarking.js
  3. Run: obj-js-debug/dist/bin/js --fuzzing-safe --enable-symbols-as-weakmap-keys weakmap-symbol-undermarking.js
  4. Observe assertion failure: 'WeakMap value is less marked than map and key (map ... is black, key ... is gray, value ... is white)' followed by 'Assertion failure: WeakMapBase::checkMarkingForZone(zone), at js/src/gc/Sweeping.cpp:1257'
  5. Note: enqueueMark/abort-weak-marking-mode are debug-only test queue commands; they simulate the weak-marking-abort condition that can occur organically in production via OOM in ephemeron table allocation (abortLinearWeakMarking is called from WeakMap-inl.h:290 when addEphemeronEdgesForEntry fails).

Security Impact

  • Severity: High
  • Attacker capability: Use-after-free of an arbitrary GC-allocated JSObject reachable via WeakMap.get(). The freed slot can be reclaimed with attacker-controlled GC allocations of the same size class, yielding type confusion when the engine interprets the reclaimed bytes as a live JSObject. This provides a path to arbitrary read/write in the content process and renderer RCE. The symbols-as-weakmap-keys feature defaults to enabled (javascript.options.experimental.symbols_as_weakmap_keys = true).
  • Preconditions: Requires the Symbol key to be referenced gray-only by an uncollected zone during a zone GC that includes the atoms zone. Requires weak marking mode to abort (occurs organically on OOM in ephemeron-table insertion via abortLinearWeakMarking) combined with incrementalWeakMapMarkingEnabled being false — the latter defaults to true in production, which adds an ephemeron-edge backstop, but the underlying stale-keyColor logic error remains and the comment's assumption is unsound. The exact trigger demonstrated uses debug-only enqueueMark to deterministically force the abort path; achieving the same in production requires winning a timing/OOM race.

ASAN Report

WeakMap value is less marked than map and key
(map 0x2d891880d0d0 is black, key 0x2e36e008b030 is gray, value 0x2e36e00ab038 is white)
Key:
JS Symbol(2e36e008b030)
Value:

{
  "address": "(JSObject*)0x2e36e00ab038",
  "nonCCWGlobal": "<global @ (JSObject*)0x2e36e008e030>",
  "clasp": "<Object @ (JSClass*)0x59ddba615ec0>",
  "shape": "<(js::Shape*)0x2e36e00a6aa0, objectFlags=[HasEnumerable]>",
  "shape.base": {
    "address": "(js::BaseShape*)0x2e36e008c0d0",
    "realm": "(JS::Realm*)0x59ddf4ed8380",
    "proto": "(JSObject*)0x2e36e0091040"
  },
  "elementsHeader": "<(js::ObjectElements*)0x59ddb6a923b0, flags=[], init=0, capacity=0, length=0>",
  "properties": {
    "x": "1 (map=(js::CompactPropMap*)0x2e36e0093b88, index=0, enumerable, configurable, writable, slot=0)"
  }
}
[1341] Assertion failure: WeakMapBase::checkMarkingForZone(zone), at /firefox/js/src/gc/Sweeping.cpp:1257
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==1341==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x59ddb8990487 bp 0x7ffe04aa7bb0 sp 0x7ffe04aa7b40 T1341)
==1341==The signal is caused by a WRITE memory access.
==1341==Hint: address points to the zero page.
    #0 0x59ddb8990487 in MOZ_CrashSequence(void*, long) /firefox/obj-js-debug/dist/include/mozilla/Assertions.h:235:3
    #1 0x59ddb8990487 in MaybeCheckWeakMapMarking(js::gc::GCRuntime*) /firefox/js/src/gc/Sweeping.cpp:1257:7
    #2 0x59ddb8990487 in js::gc::GCRuntime::endMarkingSweepGroup(JS::GCContext*, JS::SliceBudget&) /firefox/js/src/gc/Sweeping.cpp:1373:3
    #3 0x59ddb89d93cb in sweepaction::SweepActionSequence::run(js::gc::SweepAction::Args&) /firefox/js/src/gc/Sweeping.cpp:2503:23
    #4 0x59ddb89c4b78 in sweepaction::SweepActionForEach<js::gc::SweepGroupsIter, JSRuntime*>::run(js::gc::SweepAction::Args&) /firefox/js/src/gc/Sweeping.cpp:2538:19
    #5 0x59ddb899c406 in js::gc::GCRuntime::sweepPhase(JS::SliceBudget&) /firefox/js/src/gc/Sweeping.cpp:2740:55
    #6 0x59ddb88d8dca in js::gc::GCRuntime::incrementalSlice(JS::SliceBudget&, JS::GCReason, bool) /firefox/js/src/gc/GC.cpp:4382:11
    #7 0x59ddb88dc711 in js::gc::GCRuntime::gcCycle(bool, JS::SliceBudget const&, JS::GCReason) /firefox/js/src/gc/GC.cpp:4946:3
    #8 0x59ddb88de05a in js::gc::GCRuntime::collect(bool, JS::SliceBudget const&, JS::GCReason) /firefox/js/src/gc/GC.cpp:5136:9
    #9 0x59ddb88bc266 in js::gc::GCRuntime::finishGC(JS::GCReason) /firefox/js/src/gc/GC.cpp:5257:3
    #10 0x59ddb8441f32 in FinishGC(JSContext*, unsigned int, JS::Value*) /firefox/js/src/builtin/TestingFunctions.cpp:3111:12
    #11 0x59ddb9596094 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /firefox/js/src/vm/Interpreter.cpp:490:13
    #12 0x59ddb9571445 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /firefox/js/src/vm/Interpreter.cpp:586:12
    #13 0x59ddb95811ec in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) /firefox/js/src/vm/Interpreter.cpp:658:10
    #14 0x59ddb95811ec in js::Interpret(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:3272:16
    #15 0x59ddb9570aba in js::RunScript(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:460:13
    #16 0x59ddb9573920 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /firefox/js/src/vm/Interpreter.cpp:850:10
    #17 0x59ddb9573d7e in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) /firefox/js/src/vm/Interpreter.cpp:880:10
    #18 0x59ddb982d0b1 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /firefox/js/src/vm/CompilationAndEvaluation.cpp:548:10
    #19 0x59ddb982d293 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /firefox/js/src/vm/CompilationAndEvaluation.cpp:572:10
    #20 0x59ddb81758ad in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) /firefox/js/src/shell/js.cpp:1388:10
    #21 0x59ddb8174d99 in Process(JSContext*, char const*, bool, FileKind) /firefox/js/src/shell/js.cpp
    #22 0x59ddb81308bc in ProcessArgs(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12130:10
    #23 0x59ddb81308bc in Shell(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12383:12
    #24 0x59ddb8127780 in main /firefox/js/src/shell/js.cpp:12789:12

SUMMARY: UndefinedBehaviorSanitizer: SEGV /firefox/obj-js-debug/dist/include/mozilla/Assertions.h:235:3 in MOZ_CrashSequence(void*, long)
==1341==ABORTING
Group: core-security → javascript-core-security
Attached patch fix.patch — — Splinter Review
Attached file crash_stack.txt —

Is this a duplicate of bug 2028416? Note that Clauditor thinks this one is a sec-high; should we uplift the fix?

Flags: needinfo?(jcoppeard)

Yes. I'll upgrade bug 2028416 and take the testcase.

Status: UNCONFIRMED → RESOLVED
Closed: 7 months ago
Duplicate of bug: 2028416
Flags: needinfo?(jcoppeard)
Resolution: --- → DUPLICATE
See Also: → 2061775
Group: javascript-core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: