Closed Bug 1851599 (CVE-2023-5171) Opened 11 months ago Closed 10 months ago

Assertion failure: baselineScript_, at js/src/jit/JitScript.h:479

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
119 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 118+ fixed
firefox117 --- wontfix
firefox118 + fixed
firefox119 + fixed

People

(Reporter: lukas.bernhard, Assigned: iain)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression, reporter-external, sec-high, Whiteboard: [adv-main118+][adv-esr115.3+])

Attachments

(12 files)

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 ?? ()
Group: firefox-core-security → core-security
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Attached file replayFlaky.py
Attached file file_0.js
Attached file file_4.js
Attached file file_5.js
Attached file file_6.js
Attached file file_7.js
Attached file file_9.js
Attached file .mozconfig
Group: core-security → javascript-core-security
Version: Firefox 119 → Trunk

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?

Blocks: sm-jits
Severity: -- → S2
Flags: needinfo?(lukas.bernhard)
Flags: needinfo?(iireland)
Priority: -- → P2

From my side, everything can be published

Flags: needinfo?(lukas.bernhard)

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.

Flags: needinfo?(iireland)
Keywords: sec-high, regression
Priority: P2 → P1
Regressed by: 1819722
Assignee: nobody → iireland

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.

Depends on D187592

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
Attachment #9351868 - Flags: sec-approval?

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

Approved to land and request uplift

Attachment #9351868 - Flags: sec-approval? → sec-approval+
Whiteboard: [reminder-test 2023-11-07]

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
Attachment #9351868 - Flags: approval-mozilla-esr115?
Attachment #9351868 - Flags: approval-mozilla-beta?
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c16080311817
Move stub folding bailout data to JitZone r=jandem
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch

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

Approved for 118.0b8, thanks.

Attachment #9351868 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

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.
Attachment #9353233 - Flags: approval-mozilla-esr115?
Attachment #9351868 - Flags: approval-mozilla-esr115?

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

Approved for 115.3esr

Attachment #9353233 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Whiteboard: [reminder-test 2023-11-07] → [reminder-test 2023-11-07][adv-main118+]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [reminder-test 2023-11-07][adv-main118+] → [reminder-test 2023-11-07][adv-main118+][adv-esr115.3+]

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.

Flags: needinfo?(iireland)
Whiteboard: [reminder-test 2023-11-07][adv-main118+][adv-esr115.3+] → [adv-main118+][adv-esr115.3+]
Flags: needinfo?(iireland)
Group: core-security-release
Alias: CVE-2023-5171
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: