2027272 - TOCTOU in for-in iterator creation allows stale proto-chain length in [@ js::NativeIterator::NativeIterator], causing null deref or iterator-cache shape-guard bypass

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bugmon [:jkratzer for issues]]

Summary

A Time-Of-Check-Time-Of-Use bug in GetIteratorImpl (js/src/vm/Iteration.cpp). The prototype-chain length (cacheableProtoChainLength) is computed at line 1216 by LookupInIteratorCache, then PropertyEnumerator::snapshot() runs, which calls CheckForInterrupt(cx) at line 697. The interrupt callback can re-enter JS and mutate the object's prototype chain. The stale length is then passed to the NativeIterator constructor which re-walks the (now-changed) chain at lines 918-929 using the stale count.

Two outcomes depending on how the chain is mutated:

• Shortened chain (researcher's PoC): loop derefs nullptr->shape() → crash.
• Lengthened chain (verified during review): iterator is cached with fewer shape guards than chain links, violating the invariant asserted at line 939. The JIT's maybeLoadIteratorFromShape then reads past the end of the protoShapes array.

Affected Code

File: js/src/vm/Iteration.cpp, lines 918-929 (use site)

JSObject* pobj = objBeingIterated;
HashNumber shapesHash = 0;
for (uint32_t i = 0; i < numShapes; i++) {
  MOZ_ASSERT(pobj->is<NativeObject>());
  Shape* shape = pobj->shape();          // <-- null deref when pobj==nullptr
  if (i > 0) {
    new (protoShapesEnd()) GCPtr<Shape*>(shape);
    protoShapeCount_++;
  }
  shapesHash = mozilla::AddToHash(shapesHash, HashIteratorShape(shape));
  pobj = pobj->staticPrototype();        // <-- returns null on 2nd-to-last iter
}

File: js/src/vm/Iteration.cpp, lines 1216-1290 (TOCTOU window)

uint32_t cacheableProtoChainLength = 0;
if (PropertyIteratorObject* iterobj = LookupInIteratorCache(
        cx, obj, &cacheableProtoChainLength, !forObjectKeys)) {  // T1: length captured
  ...
}
...
if (!enumerator.snapshot(cx)) {   // <-- calls CheckForInterrupt at line 697, chain mutated
  return nullptr;
}
...
PropertyIteratorObject* iterobj = CreatePropertyIterator(
    cx, obj, keys, supportsIndices, indicesPtr, cacheableProtoChainLength,  // T2: stale length used
    ownPropertyCount, forObjectKeys);

numShapes in the constructor is derived directly from cacheableProtoChainLength. The loop bound reflects the chain state at T1, but pobj->staticPrototype() reflects the chain state at T2.

Secondary impact — JIT OOB read: js/src/jit/MacroAssembler.cpp, lines 9651-9678. The protoLoop terminates when the live proto chain reaches nullptr (line 9657), not when the stored shape array is exhausted. With a poisoned iterator (fewer stored shapes than chain links), line 9673 loadPtr(Address(expectedProtoShape, 0), temp3) reads past protoShapesEnd(), which line 940 documents as the end of the js_malloc'd allocation. ASAN does not instrument JIT-emitted loads, so this OOB is silent.

Correct Pattern

Re-validate the proto chain length after snapshot() returns, before passing it to CreatePropertyIterator:

if (!enumerator.snapshot(cx)) {
  return nullptr;
}

// snapshot() may have run an interrupt callback that mutated obj's proto chain.
// Re-validate that cacheableProtoChainLength still matches the live chain.
if (cacheableProtoChainLength > 0) {
  uint32_t actualLength = 0;
  JSObject* pobj = obj;
  while (pobj && CanCompareIterableObjectToCache(pobj)) {
    actualLength++;
    pobj = pobj->staticPrototype();
  }
  if (pobj != nullptr || actualLength != cacheableProtoChainLength) {
    cacheableProtoChainLength = 0;  // chain changed; do not cache
  }
}

Alternatively, move CheckForInterrupt out of snapshot() entirely — the comment says it guards against cyclic proto chains, but native chains cannot be cyclic (SetProto rejects cycles), so the check is only needed for the proxy path which already disables caching.

Exploit Chain

1. Create proto with {x: 1}, then make x non-enumerable. proto retains the sticky HasEnumerable ObjectFlag.
2. Create obj = Object.create(proto) with own properties. Chain: obj → proto → null (length 2).
3. Arm an interrupt callback that will execute Object.setPrototypeOf(obj, null) (for null-deref) or Object.setPrototypeOf(proto, ext) (for cache poisoning).
4. Execute for (var k in obj). This calls GetIteratorImpl:
   • LookupInIteratorCache walks obj → proto → null, sets cacheableProtoChainLength = 2.
   • snapshot() enumerates obj's own props. Because proto.hasEnumerableProperty() is true (sticky flag), JSITER_OWNONLY is not set — the loop walks the proto chain.
   • Line 691: GetPrototype advances obj_ to proto.
   • Line 697: CheckForInterrupt fires. Callback mutates the chain.
   • Snapshot continues with the already-fetched proto; x is non-enumerable and skipped at line 204 — crucially before line 233's markIndicesUnsupported(), keeping the cacheable path alive.
5. CreatePropertyIterator called with stale cacheableProtoChainLength = 2.
6. Null-deref variant: Constructor loop iter 0 reads obj->shape(), advances to obj->staticPrototype() = nullptr. Iter 1 executes nullptr->shape() → SEGV.
7. Cache-poison variant: Constructor stores 1 protoShape for a 3+ link chain. MOZ_ASSERT_IF(numShapes > 1, pobj == nullptr) at line 939 is violated (silent in release). Iterator is inserted into both the realm cache (line 1315) and the shape cache (line 1119). Subsequent iterations reuse it without guarding ext's shape — mutating ext is invisible to the cache. The JIT fast path additionally reads 8 bytes past the allocation per unguarded link.

Security Impact

Severity: Low (sec-low candidate).

Attacker capability:

• Denial of service via null-pointer crash (demonstrated).
• Iterator cache poisoning: for-in returns deleted property names and omits newly-added ones on the unguarded portion of the proto chain (demonstrated). This is a spec-correctness violation, not memory corruption — returned keys are valid JSLinearString* and obj[stale_key] safely returns undefined.
• JIT OOB read of ~8 bytes past the NativeIterator allocation in maybeLoadIteratorFromShape. The read value is compared against a Shape pointer; a mismatch (the overwhelming common case) falls through to the slow path. No observable information leak identified. Not independently useful beyond the cache poisoning.

Preconditions:

• Requires the interrupt callback to synchronously execute JS that mutates the target object's proto chain. In the JS shell, setInterruptCallback accepts arbitrary JS directly. In the browser, the operation callback (slow-script dialog, worker termination) does not normally run content JS synchronously; reachability in production Firefox requires further analysis. Debugger-based cross-compartment mutation (as used in this PoC) is a valid path in any embedding with Debugger enabled.

ASAN Report

AddressSanitizer:DEADLYSIGNAL
=================================================================
==301583==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x56ef04d4362a bp 0x7fffbf7abe40 sp 0x7fffbf7abd00 T0)
==301583==The signal is caused by a READ memory access.
==301583==Hint: address points to the zero page.
    #0 0x56ef04d4362a in js::NativeIterator::NativeIterator(JSContext*, JS::Handle<js::PropertyIteratorObject*>, JS::Handle<JSObject*>, JS::Handle<JS::StackGCVector<JS::PropertyKey, js::TempAllocPolicy>>, bool, mozilla::Vector<js::PropertyIndex, 8ul, js::TempAllocPolicy>*, unsigned int, unsigned int, bool, bool*) /firefox/js/src/vm/Iteration.cpp
    #1 0x56ef04d4aa19 in CreatePropertyIterator(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::StackGCVector<JS::PropertyKey, js::TempAllocPolicy>>, bool, mozilla::Vector<js::PropertyIndex, 8ul, js::TempAllocPolicy>*, unsigned int, unsigned int, bool) /firefox/js/src/vm/Iteration.cpp:842:13
    #2 0x56ef04d45190 in GetIteratorImpl(JSContext*, JS::Handle<JSObject*>, bool, bool) /firefox/js/src/vm/Iteration.cpp:1290:37
    #3 0x56ef04d4aca8 in js::GetIterator(JSContext*, JS::Handle<JSObject*>) /firefox/js/src/vm/Iteration.cpp:1324:10
    #4 0x56ef04d4aca8 in js::ValueToIterator(JSContext*, JS::Handle<JS::Value>) /firefox/js/src/vm/Iteration.cpp:1716:10
    #5 0x56ef0492623d in js::Interpret(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:2369:24
    #6 0x56ef04913ea6 in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:384:10
    #7 0x56ef04913ea6 in js::RunScript(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:460:13
    #8 0x56ef04918ae7 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /firefox/js/src/vm/Interpreter.cpp:850:10
    #9 0x56ef04c9bce2 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /firefox/js/src/vm/CompilationAndEvaluation.cpp:572:10
    #10 0x56ef02dd4c07 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) /firefox/js/src/shell/js.cpp:1388:10
    #11 0x56ef02dd42d0 in Process(JSContext*, char const*, bool, FileKind) /firefox/js/src/shell/js.cpp
    #12 0x56ef02d7fed5 in ProcessArgs(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12095:10
    #13 0x56ef02d7fed5 in Shell(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12348:12
    #14 0x56ef02d6d7c8 in main /firefox/js/src/shell/js.cpp:12754:12

==301583==Register values:
rax = 0x00000f2cd9fdc31f  rbx = 0x00007fffbf7abd00  rcx = 0x0000000066035e85  rdx = 0x00007966cfee18f8
rdi = 0x0000000000000000  rsi = 0x00007966cfee18f8  rbp = 0x00007fffbf7abe40  rsp = 0x00007fffbf7abd00
 r8 = 0x00007966cfee18e0   r9 = 0x0000000000000000  r10 = 0x0000000000000000  r11 = 0x00007966cfee18c8
r12 = 0x00000f2cd9fdc31b  r13 = 0x00000f2cd9fdc31a  r14 = 0x0000000000000001  r15 = 0x00007966cfee18d8
SUMMARY: AddressSanitizer: SEGV /firefox/js/src/vm/Iteration.cpp in js::NativeIterator::NativeIterator(...)
==301583==ABORTING

Cache poisoning demonstration (verifier's variant — does not crash, shows invariant violation):

iter1 (poisoned): ["a","STALE_KEY"]
poisonDone: true
iter2 (after ext mutation): ["a","STALE_KEY"]
*** CACHE POISONED: stale key STALE_KEY returned after deletion ***
*** CACHE POISONED: NEW_KEY missing despite being on live proto chain ***

Suggested Fix

After snapshot() returns in GetIteratorImpl, re-walk the proto chain and drop the cache if it no longer matches:

// In GetIteratorImpl, after snapshot() returns (around line 1264):

// snapshot() calls CheckForInterrupt which may run JS that mutates obj's
// proto chain. Re-validate before constructing the iterator with numShapes
// derived from cacheableProtoChainLength.
if (cacheableProtoChainLength > 0) {
  uint32_t liveLength = 0;
  for (JSObject* p = obj; p; p = p->staticPrototype()) {
    if (!CanCompareIterableObjectToCache(p)) {
      liveLength = 0;
      break;
    }
    liveLength++;
  }
  if (liveLength != cacheableProtoChainLength) {
    cacheableProtoChainLength = 0;
  }
}

A more robust alternative: remove CheckForInterrupt from the native-only path in snapshot(). The comment at line 696 says it guards against cyclic chains, but js::SetProto already rejects cycles for native objects — only proxy getPrototypeOf traps can produce unbounded chains, and the proxy path sets cacheableProtoChainLength = 0 anyway.

Comment 1

[Bugmon [:jkratzer for issues]]

Attachment: iter_toctou.js

Comment 2

[Bugmon [:jkratzer for issues]]

Attachment: crash_stack.txt

Comment 4

[Jan de Mooij [:jandem]]

This is another one that uses the shell-specific interrupt callback + debugger loophole. NI myself to fix this.

Comment 5

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20260329205327-086d14f42154.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: 810549a5947f5ae5d8cf1f11c15c841e1a6dc64c (20251231225543)
End: 8e941630524a230f9342db0dad8061dcabcbad12 (20260328200338)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False, searchfox=False, afl=False)

Comment 6

[Jan de Mooij [:jandem]]

Attachment: (secure)

Since bug 1881130 --fuzzing-safe shells can set an interrupt callback string
that gets evaluated in a fresh global. The separate global was meant to keep the
callback from mutating objects used by the interrupted code.

Claude found a loophole where it uses the Debugger API to get around this.
This patch fixes this by disabling the Debugger API for these globals.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/33f54433f0e3
https://hg.mozilla.org/integration/autoland/rev/9f8ef741a9fd
Prevent Debugger access in JS shell interrupt callback globals. r=iain

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/9f8ef741a9fd

Comment 9

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20260407215427-12a1ea4146eb.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.