1851599 - (CVE-2023-5171) Assertion failure: baselineScript_, at js/src/jit/JitScript.h:479

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 3279ccb472284368de49190d73acb13926a757ec I observed an assertion violation in the js-shell. Unfortunately, reproduction this assertion is a bit convoluted.
The 6 input files are provided via the reprl/fuzzing interface, hence the attached python script instead of a simple js file.
Furthermore, reproduction works in 10% of trials only. The script's SPIDERMONKEY must be adapted to point to the js-shell.
Precautiously marking as s-s.

I uploaded a pernosco session here but the source seems to missing, not sure what I did wrong there.
https://pernos.co/debug/5z_QVrXs3ZkRL1NaiXTQfg/index.html

#0  js::jit::AttachBaselineCacheIRStub (cx=cx@entry=0x7ffff662e100, writer=..., kind=<optimized out>,
    outerScript=0x382c863a1330, icScript=icScript@entry=0x7ffff549e400, stub=stub@entry=0x7ffff549ea70, 
    name=0x555555a30c8a "GetProp.TypedElement")
    at js/src/jit/BaselineCacheIRCompiler.cpp:2602
#1  0x0000555557cdd497 in js::jit::TryAttachStub<js::jit::GetPropIRGenerator, js::jit::CacheKind, JS::Handle<JS::Value>&, JS::Handle<JS::Value>&> (name=0x5555559d70d6 "GetElem", cx=cx@entry=0x7ffff662e100, frame=<optimized out>, 
    stub=stub@entry=0x7ffff549ea70, args=..., args=..., args=...)
    at js/src/jit/BaselineIC.cpp:497
#2  0x0000555557cdca6b in js::jit::DoGetElemFallback (cx=0x7ffff662e100, frame=0x7fffffffbea0, 
    stub=0x7ffff549ea70, lhs=..., rhs=..., res=...) at js/src/jit/BaselineIC.cpp:708
#3  0x000035c7531ab627 in ?? ()
#4  0x0000000000000101 in ?? ()
#5  0x00007fffffffbb18 in ?? ()
#6  0x0000000000000000 in ?? ()

Comment 1

[lukas.bernhard]

Attachment: replayFlaky.py

Comment 2

[lukas.bernhard]

Attachment: file_0.js

Comment 3

[lukas.bernhard]

Attachment: file_4.js

Comment 4

[lukas.bernhard]

Attachment: file_5.js

Comment 5

[lukas.bernhard]

Attachment: file_6.js

Comment 6

[lukas.bernhard]

Attachment: file_7.js

Comment 7

[lukas.bernhard]

Attachment: file_9.js

Comment 8

[lukas.bernhard]

Attachment: .mozconfig

Comment 9

[Nicolas B. Pierron [:nbp]]

Iain, you might want to have a look into this issue or forward it to the right person.

Lukas, should we keep some of the attachment private once this bug would be opened?

Comment 10

[lukas.bernhard]

From my side, everything can be published

Comment 11

[Iain Ireland [:iain]]

This is a very good catch. I think this is likely a sec-high UAF, although as far as I can tell it only allows you to write a 2-byte zero.

The pernosco recording is enough to find the culprit, although I haven't gotten a working testcase yet.

The problem is in this code. When we bail out of Ion code because a GuardMultipleShapes failed, but we successfully add a new shape to the folded stub, we want to reset the bailout counter on the IonScript to avoid invalidating. If the guard was monomorphically inlined, though, the folded stub belongs to the inlined child, but the IonScript we want to update belongs to the parent. To make this work we save a parent/child pair in the context. When we update a folded stub belonging to the child, we instead update the parent.

But we're not tracing that field in any way, so if we trigger a GC and finalize the script, then update a folded stub, we execute this code with a previously freed pointer, which bottoms out here. In this particular Pernosco recording, it looks like the freed memory may have been reallocated as a JitCode instance.

Those fields should probably be WeakHeapPtr. While we're at it, we should maybe also move them to the JitZone, since it already has a traceWeak implementation and this information is conceptually kind of similar to inlinedScriptMap_.

Monomorphic inlining landed in 113.

I'm testing a patch and working on a testcase.

Comment 12

[Iain Ireland [:iain]]

I have a testcase that very nearly works:

// |jit-test| --fast-warmup; --no-threads

function foo(phase, o1, o2) {
  switch (phase) {
    case 1:
      return o1.x;
    case 2:
      return o1.x + o2.x;
  }
}

// Set `foo` as last child and `bar` as last parent.
function phase1() {
  eval(`
  function bar(o) {
    foo(1, o);
  }
  with ({}) {}
  for (var j = 0; j < 100; j++) {
    var obj = {x: 1};
    obj["y" + (j % 10)] = 2;
    bar(obj);
  }
  bar({y: 1, x: 2});
  `);
}
phase1();

// Collect `bar`.
gc();

// Recompile `foo` monomorphically.
with ({}) {}
for (var i = 0; i < 100; i++) {
  foo(2, {x: 1}, {x: 1});
}

// Bail out and create a folded stub in `foo`.
// The child matches, so we use `bar` as the owning script.
for (var i = 0; i < 6; i++) {
  var obj = {x: 1};
  obj["y" + i] = 2;
  foo(2, {y: 1, x: 2}, obj);
}

This will call hasIonScript on a freed JSScript. That calls hasJitScript, which checks to see whether the bottom two bits of warmUpData_ are clear. (Otherwise we haven't allocated a JitScript yet, and warmUpData_ holds a warmup count.) However, because we've poisoned the freed JSScript and haven't reallocated anything, warmUpData_ contains the 0x4b poison pattern, which has the low bits set.

A real attacker would massage the GC to reallocate something important over top of the freed JSScript, but I'm not a real attacker, so I will stop here. I've verified in the debugger that my patch (which stores this data using weak pointers in the JitZone) fixes the problem by nulling out lastStubFoldingBailoutParent_ when it is collected.

I really should have caught this problem when reviewing the initial patch.

Comment 13

[Iain Ireland [:iain]]

Attachment: Bug 1851599: Move stub folding bailout data to JitZone r=jandem

Comment 14

[Iain Ireland [:iain]]

Attachment: Bug 1851599: Add testcase r=jandem

Depends on D187592

Comment 15

[Iain Ireland [:iain]]

Comment on attachment 9351868 [details]
Bug 1851599: Move stub folding bailout data to JitZone r=jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: To the best of my understanding, relatively easily. Knowing only what the bug was, I was able to get a testcase working up to the point of having to groom the GC heap to allocate something on top of a UAF pointer. The main constraint is that it only allows writing a two-byte zero.
• 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?: Everything after 113 (not esr112)
• If not all supported branches, which bug introduced the flaw?: Bug 1819722
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I think they should apply cleanly, but I haven't tested.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
• Is Android affected?: Yes

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9351868 [details]
Bug 1851599: Move stub folding bailout data to JitZone r=jandem

Approved to land and request uplift

Comment 17

[Iain Ireland [:iain]]

Comment on attachment 9351868 [details]
Bug 1851599: Move stub folding bailout data to JitZone r=jandem

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high
• User impact if declined: Potentially exploitable UAF.
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is straightforward. It would be more risky to leave this unpatched for long after landing the Nightly fix.

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable UAF.
• 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): The fix is straightforward. It would be more risky to leave this unpatched for long after landing the Nightly fix.
• String changes made/needed:
• Is Android affected?: Yes

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c16080311817
Move stub folding bailout data to JitZone r=jandem

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9351868 [details]
Bug 1851599: Move stub folding bailout data to JitZone r=jandem

Approved for 118.0b8, thanks.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a2a6a7804c50

Comment 22

[Pascal Chevrel:pascalc]

The patch does not graft cleanly to the ESR branch, could you provide an updated patch? Thanks

Comment 23

[Iain Ireland [:iain]]

Attachment: Bug 1851599: Move stub folding bailout data to JitZone (esr) r=jandem!

Comment 24

[Iain Ireland [:iain]]

Comment on attachment 9353233 [details]
Bug 1851599: Move stub folding bailout data to JitZone (esr) r=jandem!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high
• User impact if declined: Potentially exploitable UAF
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is straightforward. It would be more risky to leave this unpatched for long after landing the Nightly fix.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9353233 [details]
Bug 1851599: Move stub folding bailout data to JitZone (esr) r=jandem!

Approved for 115.3esr

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/1aa227e40ab4

Comment 27

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 28

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

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2023-11-07] .

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3302e541a24d
Add testcase r=jandem

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/3302e541a24d