1973990 - (CVE-2025-8033) crash on pointer to `NullValue` with thenable Object.prototype and generator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Shaheen Fazim]

Attachment: testcase.js

./firefox/objdir-asan/dist/bin/js --fuzzing-safe testcase.js

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1402854==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x562422ded4a6 bp 0x7ffda58ac4f0 sp 0x7ffda58ac400 T0)
==1402854==The signal is caused by a READ memory access.
==1402854==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x562422ded4a6 in JS::Value::toPrivate() const /home/user/firefox/objdir-asan/dist/include/js/Value.h:1005:46
    #1 0x562422ded4a6 in JSFunction::nativeJitInfoOrInterpretedScript() const /home/user/firefox/js/src/vm/JSFunction.h:521:63
    #2 0x562422ded4a6 in JSFunction::baseScript() const /home/user/firefox/js/src/vm/JSFunction.h:554:35
    #3 0x562422ded4a6 in JSFunction::nonLazyScript() const /home/user/firefox/js/src/vm/JSFunction.h:542:35
    #4 0x562422ded4a6 in js::InterpreterStack::resumeGeneratorCallFrame(JSContext*, js::InterpreterRegs&, JS::Handle<JSFunction*>, JS::Handle<JSObject*>) /home/user/firefox/js/src/vm/Stack-inl.h:306:35
    #5 0x562422ded4a6 in js::InterpreterActivation::resumeGeneratorFrame(JS::Handle<JSFunction*>, JS::Handle<JSObject*>) /home/user/firefox/js/src/vm/Stack-inl.h:849:14
    #6 0x562422dcba4b in js::AbstractGeneratorObject::resume(JSContext*, js::InterpreterActivation&, JS::Handle<js::AbstractGeneratorObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /home/user/firefox/js/src/vm/GeneratorObject.cpp:275:19
    #7 0x562422b73896 in js::Interpret(JSContext*, js::RunState&) /home/user/firefox/js/src/vm/Interpreter.cpp:4266:14
    #8 0x562422b63dc1 in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) /home/user/firefox/js/src/vm/Interpreter.cpp:395:10
    #9 0x562422b63dc1 in js::RunScript(JSContext*, js::RunState&) /home/user/firefox/js/src/vm/Interpreter.cpp:471:13
    #10 0x562422b650be in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:629:13
    #11 0x562422b6688a in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:664:10
    #12 0x562422b6688a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:696:8
    #13 0x5624230d0533 in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/user/firefox/js/src/vm/SelfHosting.cpp:1405:10
    #14 0x562422cc4292 in AsyncGeneratorResume(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, js::CompletionKind, JS::Handle<JS::Value>) /home/user/firefox/js/src/vm/AsyncIteration.cpp:967:8
    #15 0x562422fc50cc in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /home/user/firefox/js/src/builtin/Promise.cpp:2329:12
    #16 0x562422b64faf in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /home/user/firefox/js/src/vm/Interpreter.cpp:501:13
    #17 0x562422b64faf in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:597:12
    #18 0x562422b6688a in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:664:10
    #19 0x562422b6688a in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /home/user/firefox/js/src/vm/Interpreter.cpp:696:8
    #20 0x562422d236e1 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/user/firefox/js/src/vm/CallAndConstruct.cpp:119:10
    #21 0x562422e492e4 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JSObject*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/user/firefox/objdir-asan/dist/include/js/CallAndConstruct.h:110:10
    #22 0x562422e492e4 in js::InternalJobQueue::runJobs(JSContext*) /home/user/firefox/js/src/vm/JSContext.cpp:877:14
    #23 0x562422e4872e in js::RunJobs(JSContext*) /home/user/firefox/js/src/vm/JSContext.cpp:813:17
    #24 0x562422a715f8 in RunShellJobs(JSContext*) /home/user/firefox/js/src/shell/js.cpp:1411:5
    #25 0x562422a4d724 in Shell(JSContext*, js::cli::OptionParser*) /home/user/firefox/js/src/shell/js.cpp:12077:5
    #26 0x562422a3a830 in main /home/user/firefox/js/src/shell/js.cpp:12470:12
    #27 0x7fbda48d61c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #28 0x7fbda48d628a in __libc_start_main csu/../csu/libc-start.c:360:3
    #29 0x562422946db8 in _start (/home/user/firefox/objdir-asan/dist/bin/js+0x1cacdb8) (BuildId: 99eefa853e35784ab6cb3d580e28274c)

==1402854==Register values:
rax = 0x0000800000000005  rbx = 0x00007ffda58ac400  rcx = 0x00007ffda58ad0c0  rdx = 0x00007fbda2b1ff00
rdi = 0x0004000000000028  rsi = 0x00000ff7b4563fe0  rbp = 0x00007ffda58ac4f0  rsp = 0x00007ffda58ac400
 r8 = 0x00007ffda58ac590   r9 = 0x0000000000000f01  r10 = 0x00000fffb4b15a46  r11 = 0x00000fffb4b158b9
r12 = 0x000051e000000c80  r13 = 0x000052d000000400  r14 = 0x00007fbda2b1ff00  r15 = 0x00000fffb4b158b2
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/user/firefox/objdir-asan/dist/include/js/Value.h:1005:46 in JS::Value::toPrivate() const
==1402854==ABORTING

mozconfig:

ac_add_options --enable-project=js
ac_add_options --enable-address-sanitizer
ac_add_options --disable-jemalloc
ac_add_options --enable-optimize
ac_add_options --enable-debug-symbols
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/objdir-asan

Comment 1

[Andrew McCreight [:mccr8]]

It looks like this test case turns Object.prototype into a thenable so I'm sure something fun is happening.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Debug build hits an assert before the private unwrap failure:

#1  0x00005fc8ed200b2b in AsyncGeneratorDrainQueue (cx=0x782bcd63be00, generator=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:550
#2  0x00005fc8ed205764 in AsyncGeneratorYield (cx=0x782bcd63be00, generator=..., value=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:327
#3  0x00005fc8ed204fd7 in AsyncGeneratorResume (cx=0x782bcd63be00, generator=..., completionKind=js::CompletionKind::Normal, argument=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:982
#4  0x00005fc8ed201d22 in AsyncGeneratorAwaitedFulfilled (cx=0x782bcd63be00, generator=..., value=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:347
#5  0x00005fc8ed201b23 in js::AsyncGeneratorPromiseReactionJob (cx=0x782bcd63be00, handler=js::PromiseHandler::AsyncGeneratorAwaitedFulfilled, generator=..., argument=...)
    at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:1160
#6  0x00005fc8ed4c79de in PromiseReactionJob (cx=0x782bcd63be00, argc=0, vp=0x7ffc38ef2810) at /home/matthew/unified-git/js/src/builtin/Promise.cpp:2329
#7  0x00005fc8ed0e0295 in CallJSNative (cx=0x782bcd63be00, native=0x5fc8ed4c74f0 <PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:501
#8  0x00005fc8ed0b8375 in js::InternalCallOrConstruct (cx=0x782bcd63be00, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:597
#9  0x00005fc8ed0b8a48 in InternalCall (cx=0x782bcd63be00, args=..., reason=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:664
#10 0x00005fc8ed0b8bbe in js::Call (cx=0x782bcd63be00, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:696
#11 0x00005fc8ed26c981 in JS::Call (cx=0x782bcd63be00, thisv=..., fval=..., args=..., rval=...) at /home/matthew/unified-git/js/src/vm/CallAndConstruct.cpp:119
#12 0x00005fc8ed37be8c in JS::Call (cx=0x782bcd63be00, thisv=..., funObj=..., args=..., rval=...) at /home/matthew/unified-git/obj-debug-shell-x86_64-pc-linux-gnu/dist/include/js/CallAndConstruct.h:110
#13 0x00005fc8ed37bb60 in js::InternalJobQueue::runJobs (this=0x782bcd60d080, cx=0x782bcd63be00) at /home/matthew/unified-git/js/src/vm/JSContext.cpp:877
#14 0x00005fc8ed37b7b7 in js::RunJobs (cx=0x782bcd63be00) at /home/matthew/unified-git/js/src/vm/JSContext.cpp:813
#15 0x00005fc8ecf72553 in RunShellJobs (cx=0x782bcd63be00) at /home/matthew/unified-git/js/src/shell/js.cpp:1414
#16 0x00005fc8ecf61eeb in Shell (cx=0x782bcd63be00, op=0x7ffc38ef2c60) at /home/matthew/unified-git/js/src/shell/js.cpp:12080
#17 0x00005fc8ecf5d1d5 in main (argc=2, argv=0x7ffc38ef2ed8) at /home/matthew/unified-git/js/src/shell/js.cpp:12473

Which, fun, is an assert added by Arai in Bug 1901411, the specification CVE

Pernosco session for original test case is here.

Test case reduced nicely down to

async function* gen() {
  yield 1;
}

let g;

Object.defineProperty(Object.prototype, "then", {
  get() {
    g.return();
    return undefined;
  },
});

g = gen();
g.next();

Opt build on the reduced test case crashes like this for me:

#0  js::InterpreterStack::resumeGeneratorCallFrame (this=0x7cfe6a22c000, cx=0x7cfe6a235100, regs=..., callee=..., envChain=...) at /home/matthew/unified-git/js/src/vm/Stack-inl.h:306
#1  js::InterpreterActivation::resumeGeneratorFrame (this=this@entry=0x7ffef3cf3c90, callee=callee@entry=..., envChain=envChain@entry=...) at /home/matthew/unified-git/js/src/vm/Stack-inl.h:849
#2  0x00005727e375dd88 in js::AbstractGeneratorObject::resume (cx=<optimized out>, activation=..., genObj=..., arg=..., resumeKind=...) at /home/matthew/unified-git/js/src/vm/GeneratorObject.cpp:275
#3  0x00005727e3686ffa in js::Interpret (cx=cx@entry=0x7cfe6a235100, state=...) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:4260
#4  0x00005727e3681cfb in MaybeEnterInterpreterTrampoline (cx=0x7cfe6a235100, state=...) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:395
#5  js::RunScript (cx=cx@entry=0x7cfe6a235100, state=...) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:471
#6  0x00005727e3682396 in js::InternalCallOrConstruct (cx=0x7cfe6a235100, args=..., construct=construct@entry=js::NO_CONSTRUCT, reason=reason@entry=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:629
#7  0x00005727e3682916 in InternalCall (args=..., reason=js::CallReason::Call, cx=<optimized out>) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:664
#8  js::Call (cx=cx@entry=0x7cfe6a235100, fval=..., thisv=thisv@entry=..., args=..., rval=rval@entry=..., reason=reason@entry=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:696
#9  0x00005727e38615cf in js::CallSelfHostedFunction (cx=cx@entry=0x7cfe6a235100, name=..., thisv=thisv@entry=..., args=..., rval=rval@entry=...) at /home/matthew/unified-git/js/src/vm/SelfHosting.cpp:1406
#10 0x00005727e3701235 in AsyncGeneratorResume (cx=0x7cfe6a235100, generator=..., completionKind=<optimized out>, argument=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:967
#11 0x00005727e37005e9 in js::AsyncGeneratorPromiseReactionJob (cx=0x7ffef3cf3c90, cx@entry=0x7cfe6a235100, handler=<optimized out>, generator=..., generator@entry=..., argument=..., argument@entry=...) at /home/matthew/unified-git/js/src/vm/AsyncIteration.cpp:288
#12 0x00005727e38095b3 in PromiseReactionJob (cx=cx@entry=0x7cfe6a235100, argc=<optimized out>, vp=<optimized out>) at /home/matthew/unified-git/js/src/builtin/Promise.cpp:2329
#13 0x00005727e3682113 in CallJSNative (cx=0x7cfe6a235100, native=0x5727e3809150 <PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:501
#14 js::InternalCallOrConstruct (cx=0x7cfe6a235100, args=..., construct=construct@entry=js::NO_CONSTRUCT, reason=reason@entry=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:597
#15 0x00005727e3682916 in InternalCall (args=..., reason=js::CallReason::Call, cx=<optimized out>) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:664
#16 js::Call (cx=cx@entry=0x7cfe6a235100, fval=fval@entry=..., thisv=..., args=..., rval=rval@entry=..., reason=reason@entry=js::CallReason::Call) at /home/matthew/unified-git/js/src/vm/Interpreter.cpp:696
#17 0x00005727e37245c7 in JS::Call (cx=cx@entry=0x7cfe6a235100, thisv=..., fval=fval@entry=..., args=..., rval=rval@entry=...) at /home/matthew/unified-git/js/src/vm/CallAndConstruct.cpp:119
#18 0x00005727e37869b6 in JS::Call (cx=0x7cfe6a235100, thisv=..., funObj=..., args=..., rval=...) at /home/matthew/unified-git/obj-opt-shell-nodebug-x86_64-pc-linux-gnu/dist/include/js/CallAndConstruct.h:110
#19 js::InternalJobQueue::runJobs (this=0x7cfe6a222a60, cx=0x7cfe6a235100) at /home/matthew/unified-git/js/src/vm/JSContext.cpp:877
#20 0x00005727e37865e9 in js::RunJobs (cx=cx@entry=0x7cfe6a235100) at /home/matthew/unified-git/js/src/vm/JSContext.cpp:813
#21 0x00005727e36194dd in RunShellJobs (cx=cx@entry=0x7cfe6a235100) at /home/matthew/unified-git/js/src/shell/js.cpp:1414
#22 0x00005727e360cc4a in Shell (cx=0x7cfe6a235100, op=op@entry=0x7ffef3cf48b8) at /home/matthew/unified-git/js/src/shell/js.cpp:12080
#23 0x00005727e36046ea in main (argc=<optimized out>, argv=<optimized out>) at /home/matthew/unified-git/js/src/shell/js.cpp:12473

At this point callee is 0x4000000000000 which we dereference. If my rr tracing in the opt build is correct, this value is from the allocation of a SharedShape, and so I'd likely call this a wild-pointer deref, and will mark this as sec-hig.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

pernosco trace of above reduced test case, debug build

Comment 4

[Andrew McCreight [:mccr8]]

Thank you for the quick analysis.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Unfortunately, the current spec and our implementation have diverged quite a bit, which makes it hard for me to reason about what the current spec expectations are here.

I can say this test case causes v8, engine262 and xs to hang, and graaljs to crash;

The then call is happening at what is in the new spec labelled as 27.6.3.5.7.d

1. Assert: generator.[[AsyncGeneratorQueue]] is not empty.
   1. Let next be the first element of generator.[[AsyncGeneratorQueue]].
   1. Remove the first element from generator.[[AsyncGeneratorQueue]].
   1. Let promiseCapability be next.[[Capability]].
   1. Let value be completion.[[Value]].
   1. If completion is a throw completion, then
   1. Perform ! Call(promiseCapability.[[Reject]], undefined, « value »).
   1. Else,
   1. Assert: completion is a normal completion.
   1. If realm is present, then
   1. Let oldRealm be the running execution context's Realm.
   1. Set the running execution context's Realm to realm.
   1. Let iteratorResult be CreateIteratorResultObject(value, done).
   1. Set the running execution context's Realm to oldRealm.
   1. Else,
   1. Let iteratorResult be CreateIteratorResultObject(value, done).
   1. Perform ! <emu-meta effects="user-code">Call</emu-meta>(promiseCapability.[[Resolve]], undefined, « iteratorResult »).
   1. Return unused.

Our implementation looks like this:

// ES2022 draft rev 193211a3d889a61e74ef7da1475dfa356e029f29
//
// AsyncGeneratorYield ( value )
// https://tc39.es/ecma262/#sec-asyncgeneratoryield
//
// Stesp 10-13.
[[nodiscard]] static bool AsyncGeneratorYield(
    JSContext* cx, Handle<AsyncGeneratorObject*> generator, HandleValue value) {
  // Step 13.a.
  generator->setSuspendedYield();

  // Step 10. Perform
  //          ! AsyncGeneratorCompleteStep(generator, completion, false,
  //                                       previousRealm).
  if (!AsyncGeneratorCompleteStepNormal(cx, generator, value, false)) {
    return false;
  }

  // Steps 11-13.
  return AsyncGeneratorDrainQueue(cx, generator);
}

The problem is that CompleteStepNormal can trigger user code, which invalidates some of the assumptions behind AsynGeneratorDrainQueue, and we assert as soon as we call it.

At this point I no longer have enough of this in my head to proceed without assistance, so I'm going to ni? arai -- the fact that the state machine is out of sync with the spec makes reasoning about this extra hard for me.

Comment 6

[Iain Ireland [:iain]]

Note that 0x4000000000000 has a high bit set (outside the 48-bit address space). If I'm doing my math right, it's what you expect as the result of NullValue.toObject() (in a non-debug build). If we reliably crash by dereferencing this value, it's probably not exploitable.

That's consistent with the ASAN output in the initial report: rdi = 0x0004000000000028 is precisely what we'd expect if we unboxed a null value where we expected a JSFunction, then tried to load the script from that 'function'. (NativeJitInfoOrInterpretedScriptSlot is the third reserved slot. Offset 0x0 is the shape, 0x8 is the slots pointer, 0x10 is the elements pointer, and then three reserved slots gets us to an offset of 0x28 from the base pointer.)

Edit: I see a number of places where we store NullValue in AsyncGeneratorObject slots. Without digging into the details of the bug, it seems plausible to me that calling a method in the wrong state would deterministically return this specific bogus value.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

It's plausible I goofed the optimized build reversing; had to do a bunch of pointer math to get the slot. If so, that would be nice :)

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Converted test case to HTML and tested in browser; it does seem like that's the same crash address.

https://crash-stats.mozilla.org/report/index/19d69735-a9c1-4245-b957-6ebd60250625

So that's nice.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Yep. I re-did my analysis on the opt build and I made a mistake with the pointer offset :)

In AbstractGeneratorObject::setClosed we wipe out the callee slot; so I think Iain's analysis is correct for this test case. In the opt case I see the generator object getting closed (which is what wipes out the callee slot) in js::HandleClosingGeneratorReturn.

However, in the asserting case we don't get there... I'm not sure if there's branch points where something else could be forced to go wrong with more manipulation in the then handler. I'm going to defer on future analysis to Arai to see if there's an exploitable gap.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

opt crash pernosco trace if anyone wants it

Comment 11

[Tooru Fujisawa [:arai]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #5)

Unfortunately, the current spec and our implementation have diverged quite a bit, which makes it hard for me to reason about what the current spec expectations are here.

The divergence comes from 2 separate reasons:

• (a) We've updated our implementation for https://github.com/tc39/ecma262/pull/2413 , which is before the spec issue fix, and then partially reverted it, as a short-term fix for the spec issue, thus, our implementation is a mix of the following:
  • before https://github.com/tc39/ecma262/pull/2413 (reverted part. affected by the spec issue)
  • after https://github.com/tc39/ecma262/pull/2413 (not-reverted part. not-affected by the spec issue)
  • slight modification to connect them
• (b) We have an implementation-defined [[AsyncGeneratorState]] value State_AwaitingYieldReturn, which was a part of "executing" (now it might be "draining-queue" maybe? I'll check details), in order to implement Await operation done in AsyncGeneratorDrainQueue

Then, debug built hits the assertion !generator->isAwaitingYieldReturn() in AsyncGeneratorDrainQueue,
AsyncGeneratorDrainQueue is the part which is partially reverted, and also the assertion is for (b), which means both of them are related to this issue.

For (a), I'll see if re-applying the update simplifies the situation.

For (b), the divergence is still necessary, and the fix would be something specific to our implementation.

Anyway, my current understanding is that,
we should have special code paths for State_AwaitingYieldReturn in some places,
given that the spec assumes the Await operation finishes before continuting to the next step, but that's not true for our C++ implementation, and the spec steps should be split into two separate function in our impl.

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

Ah. So, I'll let you reclassify this as necessary; currently based on the test case it's no longer sec-high, but there is a gap still where I'm not sure what can go wrong.

Comment 13

[Tooru Fujisawa [:arai]]

Here's my current plan:

1. re-apply the update, to reduce the difference, but without introducing any behavior difference, and make a situation where there's only one divergence comes from the implementation-defined states
2. add special code path for implementation-defined states, and verify the fix for this bug
3. add the special code path to the current files (without the changes from 1), to see if we can create a smaller patch for uplift

my current assumption is that:

• we'll need to add early return for AwaitingReturn and AwaitingYieldReturn in some cases, so that steps after Await isn't performed immediately (done in 2, and then ported to 3)
• ensure the corresponding continuation for those Awaits follows the spec steps (utilizing the result of 1)

Comment 14

[Tooru Fujisawa [:arai]]

Actually, the problem was the state transition around suspended-yield.

When yield is performed inside an async generator, our implementation sets the [[AsyncGeneratorState]] from "exexuting" to "suspended-yield" before performing AsyncGeneratorCompleteStep, and then drains the queue.

This is actually reordered from the current spec, as the comment says (Step 13.a vs Step 10), but the order follows the old spec (https://tc39.es/ecma262/2021/#sec-asyncgeneratoryield)

https://searchfox.org/mozilla-central/rev/058836008f131ae5591d04952a1500c9f94bedbc/js/src/vm/AsyncIteration.cpp#314-328

[[nodiscard]] static bool AsyncGeneratorYield(
    JSContext* cx, Handle<AsyncGeneratorObject*> generator, HandleValue value) {
  // Step 13.a.
  generator->setSuspendedYield();

  // Step 10. Perform
  //          ! AsyncGeneratorCompleteStep(generator, completion, false,
  //                                       previousRealm).
  if (!AsyncGeneratorCompleteStepNormal(cx, generator, value, false)) {
    return false;
  }

  // Steps 11-13.
  return AsyncGeneratorDrainQueue(cx, generator);
}

Given that AsyncGeneratorCompleteStep has side-effect when resolving a promise, the reorder between setSuspendedYield and AsyncGeneratorCompleteStepNormal is problematic here.

With the state set to "suspended-yield" before AsyncGeneratorCompleteStep, it allows another state transition inside AsyncGeneratorCompleteStep, for example, calling %AsyncGeneratorPrototype%.return can transition from "suspended-yield" to "AwaitingYieldReturn", that breaks the assumption.

If we keep the "executing" state when performing AsyncGeneratorCompleteStep, it cannot transition to other state, and the problem won't happen in AsyncGeneratorDrainQueue.

I confirmed the following:

• moving setSuspendedYield after AsyncGeneratorCompleteStepNormal fixes the crash, and the testcase results in infinite loop
• updating our code to the latest spec cannot be done unless moving the setSuspendedYield call. and otherwise the update is mostly no-functional-change (except for the state is defined differently between them)
• other side-effect cases are all unrelated to "AwaitingYieldReturn" state, given that all of them are done with "Completed" state, which cannot transition to "AwaitingYieldReturn" state

I'll prepare a patch for this issue, and then post another patch stack that updates the implementation to the latest spec, in a separate bug, after this bug gets fixed and uplift finishes.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1973990 - Do not reorder the state transition for async generator yield. r?mgaudet!

Comment 16

[Matthew Gaudet (he/him) [:mgaudet]]

Thank you for the diagnosis! My only question remaining is your read on exploitability; currently I don't think this is actually a wild pointer deref, but I'd be open to a more confident diagnosis.

Comment 17

[Tooru Fujisawa [:arai]]

Yeah, the issue here is that we're trying to resume a closed generator, because we transition to AwaitingYieldReturn twice and enqueued a job that resumes the generator twice.
The first one resumes and closes the generator, and the second one resumes the closed generator.

When we close the generator, we clear the CALLEE_SLOT with NullValue

https://searchfox.org/mozilla-central/rev/445973bddca7645f6d8068e9347b4d98b1aeb59e/js/src/vm/GeneratorObject.cpp#452-453

void AbstractGeneratorObject::setClosed(JSContext* cx) {
  setFixedSlot(CALLEE_SLOT, NullValue());

and resuming the generator reads the CALLEE_SLOT and converts it into JSFunction*

https://searchfox.org/mozilla-central/rev/445973bddca7645f6d8068e9347b4d98b1aeb59e/js/src/vm/GeneratorObject.cpp#267-273

bool AbstractGeneratorObject::resume(JSContext* cx,
                                     InterpreterActivation& activation,
                                     Handle<AbstractGeneratorObject*> genObj,
                                     HandleValue arg, HandleValue resumeKind) {
  MOZ_ASSERT(genObj->isSuspended());

  RootedFunction callee(cx, &genObj->callee());

https://searchfox.org/mozilla-central/rev/445973bddca7645f6d8068e9347b4d98b1aeb59e/js/src/vm/GeneratorObject.h#76-77

JSFunction& callee() const {
  return getFixedSlot(CALLEE_SLOT).toObject().as<JSFunction>();

Thus, this is always nullptr-deref, and not exploitable.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f639eb380aad
https://hg.mozilla.org/integration/autoland/rev/bedff7cb27c4
Do not reorder the state transition for async generator yield. r=mgaudet

Comment 19

[Tooru Fujisawa [:arai]]

Given now that it's sec-low (or maybe other), I'll land the patch without asking sec-approval.
The same patch is applicable to all supported branches. I'll ask for uplift shortly

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: (secure)

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

Comment 21

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Nullptr-deref crash when visiting a crafted website
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This prevents unexpected state transition, and such transition won't be used in wild except for malicious intent
• String changes made/needed: none
• Is Android affected?: yes

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1973990 - Do not reorder the state transition for async generator yield. r?mgaudet!

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

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: (secure)

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

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: (secure)

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

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: (secure)

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

Comment 26

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: Nullptr-deref crash when visiting a crafted website
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This prevents unexpected state transition, and such transition won't be used in wild except for malicious intent
• String changes made/needed: none
• Is Android affected?: yes

Comment 27

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined: Nullptr-deref crash when visiting a crafted website
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This prevents unexpected state transition, and such transition won't be used in wild except for malicious intent
• String changes made/needed: none
• Is Android affected?: yes

Comment 28

[Phabricator Automation]

firefox-esr128 Uplift Approval Request

• User impact if declined: Nullptr-deref crash when visiting a crafted website
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This prevents unexpected state transition, and such transition won't be used in wild except for malicious intent
• String changes made/needed: none
• Is Android affected?: yes

Comment 29

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Nullptr-deref crash when visiting a crafted website
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: low
• Explanation of risk level: This prevents unexpected state transition, and such transition won't be used in wild except for malicious intent
• String changes made/needed: none
• Is Android affected?: yes

Comment 30

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

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

Comment 31

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/653a6919ce67
https://hg.mozilla.org/releases/mozilla-beta/rev/388cea8d4e51

Comment 32

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/326832e45625

Comment 33

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/7bd1c6b763a2

Comment 34

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/de7a6da77e99

Comment 35

[Simon Friedberger [:simonf]]

Attachment: advisory.txt