1814899 - (CVE-2023-25751) MOZ_ASSERT(ionScript->containsReturnAddress(returnAddr)) at jit/JSJitFrameIter.cpp:71

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

Steps to reproduce:

The attached sample crashes the js-shell on commit a7156afbfa575f12f60b1c8bf099d547c29bcadf when executed via commandline --fuzzing-safe --gc-zeal=10. The crash is flaky, but running it via rr record --chaos for some time should yield a crash. While the crash is in line MOZ_ASSERT(ionScript->containsReturnAddress(returnAddr));, the instruction is mov rax,QWORD PTR [rbx+0x30] instead of a normal assertion failure.

function f0(a1) {
    const v5 = [0,0,0,0,0,0,0,0];
    const v10 = new SharedArrayBuffer(1000, {});
    const v12 = new Int16Array();
    for (const v14 in v5.join(a1)) { } 
}
f0.toString = f0; 
for (let v15 = 0; v15 < 100; v15++) {
    f0(f0);
}
f0();
this.unscopables;

#0  js::jit::JSJitFrameIter::checkInvalidation (this=<optimized out>, ionScriptOut=0x7ffe1b63b5c0)
    at js/src/jit/JSJitFrameIter.cpp:71
#1  0x00005564199cbe93 in js::jit::TraceIonJSFrame (trc=0x7ffe1b63b938, frame=...)
    at js/src/jit/JitFrames.cpp:947
#2  js::jit::TraceJitActivation (trc=0x7ffe1b63b938, activation=<optimized out>)
    at js/src/jit/JitFrames.cpp:1358
#3  js::jit::TraceJitActivations (cx=<optimized out>, trc=0x7ffe1b63b938) at js/src/jit/JitFrames.cpp:1400
#4  0x00005564193b6560 in js::gc::GCRuntime::traceRuntimeCommon (this=0x6d0133323788, trc=0x7ffe1b63b938, 
    traceOrMark=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:303
#5  0x00005564193b6300 in js::gc::GCRuntime::traceRuntimeForMajorGC (this=0x6d0133323788, trc=0x7ffe1b63b938, session=...)
    at js/src/gc/RootMarking.cpp:242
#6  0x000055641931cc57 in js::gc::GCRuntime::updateRuntimePointersToRelocatedCells (this=0x6d0133323788, session=...)
    at js/src/gc/Compacting.cpp:802
#7  js::gc::GCRuntime::compactPhase (this=0x6d0133323788, reason=JS::GCReason::DEBUG_GC, sliceBudget=..., session=...)
    at js/src/gc/Compacting.cpp:103
#8  0x0000556419341b8c in js::gc::GCRuntime::incrementalSlice (this=0x6d0133323788, budget=..., reason=JS::GCReason::DEBUG_GC, 
    budgetWasIncreased=<optimized out>) at js/src/gc/GC.cpp:3696
#9  0x0000556419346109 in js::gc::GCRuntime::gcCycle (this=0x6d0133323788, nonincrementalByAPI=false, budgetArg=..., 
    reason=JS::GCReason::DEBUG_GC) at js/src/gc/GC.cpp:4160
#10 0x0000556419346f63 in js::gc::GCRuntime::collect (this=0x6d0133323788, nonincrementalByAPI=<optimized out>, budget=..., 
    reason=JS::GCReason::DEBUG_GC) at js/src/gc/GC.cpp:4348
#11 0x0000556419311542 in js::gc::GCRuntime::runDebugGC (this=0x6d0133323788) at js/src/gc/GC.cpp:4795
#12 js::gc::GCRuntime::gcIfNeededAtAllocation (this=<optimized out>, cx=<optimized out>)
    at js/src/gc/Allocator.cpp:412
#13 js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0x6d0133323788, cx=0x6d0133336600, 
    kind=js::gc::AllocKind::OBJECT2_BACKGROUND) at js/src/gc/Allocator.cpp:376
#14 0x000055641931095c in js::gc::detail::AllocateObject<(js::AllowGC)1> (cx=0x6d0133336600, kind=<optimized out>, nDynamicSlots=0, 
    heap=js::gc::TenuredHeap, clasp=0x556419f27090 <js::PropertyIteratorObject::class_>, site=0x0)
    at js/src/gc/Allocator.cpp:56
#15 0x00005564188f3319 in js::gc::CellAllocator::NewCell<js::NativeObject, (js::AllowGC)1, js::gc::AllocKind&, unsigned long const&, js::gc::InitialHeap&, JSClass const*&, js::gc
::AllocSite*&> (cx=0x6d0133336600, args=<optimized out>, args=<optimized out>, 
--Type <RET> for more, q to quit, c to continue without paging--
     args=<optimized out>, args=<optimized out>) at js/src/gc/Allocator.h:123
#16 JSContext::newCell<js::NativeObject, (js::AllowGC)1, js::gc::AllocKind&, unsigned long const&, js::gc::InitialHeap&, JSClass const*&, js::gc::AllocSite*&> (this=0x6d013333660
0, args=<optimized out>, args=<optimized out>, args=<optimized out>, args=<optimized out>, 
    args=<optimized out>) at js/src/vm/JSContext.h:266
#17 js::NativeObject::create (cx=0x6d0133336600, kind=js::gc::AllocKind::OBJECT2_BACKGROUND, heap=js::gc::TenuredHeap, shape=..., 
    site=0x0) at js/src/vm/NativeObject-inl.h:446
#18 0x0000556418b84b50 in NewPropertyIteratorObject (cx=0x6d0133336600) at js/src/vm/Iteration.cpp:803
#19 CreatePropertyIterator (cx=0x6d0133336600, objBeingIterated=..., props=..., supportsIndices=false, indices=0x0, numShapes=0)
    at js/src/vm/Iteration.cpp:845
#20 0x0000556418b7f0eb in GetIteratorImpl<false> (obj=..., cx=<optimized out>)
    at js/src/vm/Iteration.cpp:1262
#21 js::GetIterator (cx=0x6d0133336600, obj=...) at js/src/vm/Iteration.cpp:1293
#22 0x0000556418b86112 in js::ValueToIterator (cx=0x6d0133336600, vp=...) at js/src/vm/Iteration.cpp:1619

Comment 1

[Jan de Mooij [:jandem]]

NI Iain because this is in the for-in iterator code.

Comment 2

[Iain Ireland [:iain]]

Thanks for the report.

When generating code for for (const v14 in v5.join(a1)) { }, we emit a call to array_join, followed immediately by a call to ValueToIterator. Because f0.toString has been overwritten to point to f0, the join call enters f0 recursively. If we do a major GC inside ValueToIterator, then we run into problems while invalidating:

[IonInvalidate] Invalidating all frames for GC
[IonInvalidate] BEGIN invalidating activation
[IonInvalidate] #1 exit frame @ 7ffeefb24120
[IonInvalidate] #2 Optimized JS frame @ 7ffeefb24180, [...] (fun: 1abac7685890, [...], pc ***1aa5081073be***) // >1<
[IonInvalidate]    ! Invalidate ionScript 7f50c0227600 (inv count 1) -> patching osipoint 1aa508107674
[IonInvalidate] #3 rectifier frame @ 7ffeefb241b0
[IonInvalidate] END invalidating activation
[IonInvalidate] Invalidating all frames for GC
[IonInvalidate] BEGIN invalidating activation
[IonInvalidate] #1 exit frame @ 7ffeefb251d8
[IonInvalidate] #2 Optimized JS frame @ 7ffeefb25230 [...] (fun: 1abac7685890, [...], pc 1aa5081070d8)
[IonInvalidate]    ! Invalidate ionScript 7f50c0227600 (inv count 2) -> patching osipoint ***1aa5081073b6*** // >2<
[IonInvalidate] #3 baseline stub frame @ 7ffeefb25270
[IonInvalidate] #4 Baseline JS frame @ 7ffeefb252f0, [...] (fun: 0, script: 1abac76900b0, pc 1aa5080c7e58)
[IonInvalidate] END invalidating activation

The issue here is that the first frame we invalidate (>1<) has resume pc 0x1aa5081073be, meaning that we write the delta starting at address 0x1aa5081073ba. But later, when we invalidate a frame for the same script (>2<), the address where we patch the OSI point is 0x1aa5081073b6. We write 5 bytes (0xE8 for the call, and a four byte offset). The most significant byte of the offset clobbers the least significant byte of the delta we wrote for the previous frame. Later, we trace the frame, try to check the invalidation, and crash because the delta does not point to the location of a valid IonScript.

This is obviously not supposed to happen, but I can't find the code that prevents it. We have ensureOsiSpace that seems like it's intended to avoid this, but AFAICT that only prevents OSI patching from being clobbered, not the delta. (It also looks like we never actually generate any nops using that code.)

I don't think this is directly related to my for-in changes. Unless I'm missing something, ValueToIterator just happens to be the easiest way to have a VM call immediately following another call, with no intervening guards.

I think the fix is probably to somehow beef up ensureOsiSpace?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Iain Ireland [:iain] from comment #2)

I think the fix is probably to somehow beef up ensureOsiSpace?

Yes. This is the way!

ensureOsiSpace is used to ensure that the code that we generate for invalidation (i-e. a bailout) does not overlap with the return address of the call of another instruction. Otherwise, we might have random code execution. [overlapping-patches]

When invalidated, we patch the code and store the delta-offset from the return location to the invalidation epilogue. This is useful as if we lose the reference of the current function, then the space might be reclaimed and reused before we return to the invalidated function, and we might once again have random code execution. [exec-after-free]

Ion is limited, It can execute only a single potentially-invalidating call per LIR instruction. If we have more than one, then we risk returning to an invalidated code base where non-code (the delta-offset to the invalidation offset) is written in place of the code. Which can be assimilated to random code execution (except that it is limited to small offsets). [exec-after-patch]

However, we have not forbidden generating multiple potentially-invalidating calls in the same LIR, as we already had use cases for multiple calls on different branches, which is safe as long as we do not re-enter a potentially-invaliding call before reaching the end of the LIR instruction.

(In reply to Iain Ireland [:iain] from comment #2)

This is obviously not supposed to happen, but I can't find the code that prevents it. We have ensureOsiSpace that seems like it's intended to avoid this, but AFAICT that only prevents OSI patching from being clobbered, not the delta. (It also looks like we never actually generate any nops using that code.)

On RISCV this might be another story.

I suspect the problem you are facing is that ensureOsiSpace only inserts nop-slides if the last ensureOsiSpace was not too close, but at the moment it is only called under LOsiPoint, which is the problem.

All potentially invalidating call should be checking that lastOsiPointOffset_ is far away enough (by calling ensureOsiSpace) to insert a nop-slide to safely encode the bailing jump (PatchWrite_NearCall). Which is most likely the problem you are seeing if a potentially-invalidating call instruction is generated directly after an LOsiPoint:

  LOsiPoint
  LPotentiallyInvalidatingCall

Calling ensureOsiSpace() before the call instruction of LPotentiallyInvalidatingCall should do the trick ;)

Comment 4

[Daniel Veditz [:dveditz]]

is this.unscopables limited to the JS Shell? I don't see it in a Firefox document context (not even a privileged one). Is that a necessary part of the problem, or is that just the statement that revealed the problem that had already happened, such that this could be a problem in web content as well?

Calling this sec-high on the worst-case presumption that web content could trigger this. If that's false feel free to lower the severity appropriately.

Comment 5

[Iain Ireland [:iain]]

Unfortunately I don't see anything preventing this from being triggered by web content. The only requirement is being able to trigger a compacting GC at exactly the right time.

Here's a more robust version of the testcase that doesn't depend on exact allocation counts:

function bar(x) {
  with ({}) {}
  switch (x) {
  case 1:
    foo(2);
    break;
  case 2:
    gczeal(14, 1); // Trigger a major GC for the next allocation, which is the PropertyIteratorObject in ValueToIterator
    break;
  }
  return "a sufficiently long string";
}

function foo(x) {
  for (var s in bar(x)) { gczeal(0); }
}

with ({}) {}
for (var i = 0; i < 100; i++) {
  foo(0);
}
foo(1);

In this particular case, the victim is a VM call, and calling ensureOsiSpace just before masm.callJit in callVMInternal fixes the bug. I think we probably want to call ensureOsiSpace before the call whenever we mark a safepoint. I'll put up a patch.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1814899: Ensure more OsiSpace r=jandem

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1814899: Add testcase r=jandem

Depends on D169274

Comment 8

[Iain Ireland [:iain]]

Comment on attachment 9316683 [details]
Bug 1814899: Ensure more OsiSpace r=jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it's reasonable for a smart, sufficiently motivated attacker to deduce the general shape of the bug by looking at where we're adding more NOPs. It might be tricky to deduce the exact sequence of events that I used to trigger it (triggering a major GC in a call to ValueToIterator), but there might be other ways to get at the same bug. Once you can trigger the bug, it seems like there's plenty of room to build an exploit.
• 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 older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I think this patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: We generate a few nops in particularly rare cases. It's fairly unlikely to cause regressions.
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9316683 [details]
Bug 1814899: Ensure more OsiSpace r=jandem

Approved to request uplift and land

Comment 10

[Iain Ireland [:iain]]

Comment on attachment 9316683 [details]
Bug 1814899: Ensure more OsiSpace r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable sec-high bug
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We insert a few extra nops in rare circumstances. At worst there could be a tiny performance regression.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high bug
• User impact if declined: Potential exploitation
• Fix Landed on Version: 112
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We insert a few extra nops in rare circumstances. At worst there could be a tiny performance regression.

Comment 11

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

Ensure more OsiSpace r=jandem
https://hg.mozilla.org/integration/autoland/rev/4378074a655475cf79ddb17cce8ed286f4f22f1f
https://hg.mozilla.org/mozilla-central/rev/4378074a6554

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9316683 [details]
Bug 1814899: Ensure more OsiSpace r=jandem

Approved for 111.0b5

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ceae239742c

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9316683 [details]
Bug 1814899: Ensure more OsiSpace r=jandem

Approved for 102.9esr.

Comment 15

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/207a0ca14a75

Comment 16

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

2 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2023-04-24] .

iain, please refer to the original comment to better understand the reason for the reminder.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e73c493db8f