Closed Bug 1791520 (CVE-2022-42928) Opened 5 months ago Closed 4 months ago

Nullptr dereference in IsWriteableAddress when coming from CreateBigIntFromInt64

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr102 106+ fixed
firefox105 --- wontfix
firefox106 + fixed
firefox107 + fixed

People

(Reporter: saelo, Assigned: anba)

References

(Blocks 2 open bugs)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main106+][adv-esr102.4+])

Attachments

(4 files)

The following sample will cause a nullptr dereference in Spidermonkey built from current HEAD:

function main() {
const v19 = async (v20,v21,v22) => {
};
for (let v26 = 0; v26 < 100; v26++) {
    const v27 = v19();
    const v28 = v27.finally();
    for (const v30 in this) {
        const v33 = "function".trimLeft(-992713.7098905144,"function");
        const v34 = v33.startsWith("function");
        const v37 = this.BigInt64Array;
        const v38 = new v37(2);
        const v39 = v38[1];
        continue;
    }
}
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:

Here is the stacktrace from gdb:

Thread 1 "js" received signal SIGSEGV, Segmentation fault.
0x0000555558176292 in IsWriteableAddress (ptr=0x0) at js/src/gc/Nursery.cpp:771
771       *vPtr = *vPtr;
(gdb) bt
#0  0x0000555558176292 in IsWriteableAddress (ptr=0x0) at js/src/gc/Nursery.cpp:771
#1  0x0000555558176175 in js::Nursery::forwardBufferPointer (this=0x7ffff771b138, pSlotsElems=0x7fffffff9aa8) at js/src/gc/Nursery.cpp:800
#2  0x000055555887cc82 in js::jit::UpdateIonJSFrameForMinorGC (rt=0x7ffff7718000, frame=...) at js/src/jit/JitFrames.cpp:1055
#3  0x000055555887ca1a in js::jit::UpdateJitActivationsForMinorGC (rt=0x7ffff7718000) at js/src/jit/JitFrames.cpp:1392
#4  0x0000555558177ad4 in js::Nursery::doCollection (this=0x7ffff771b138, reason=JS::GCReason::OUT_OF_NURSERY) at js/src/gc/Nursery.cpp:1265
#5  0x00005555581772df in js::Nursery::collect (this=0x7ffff771b138, options=JS::GCOptions::Normal, reason=JS::GCReason::OUT_OF_NURSERY) at js/src/gc/Nursery.cpp:1114
#6  0x00005555581020d5 in js::gc::GCRuntime::collectNursery (this=0x7ffff7718768, options=JS::GCOptions::Normal, reason=JS::GCReason::OUT_OF_NURSERY, phase=js::gcstats::PhaseKind::MINOR_GC) at js/src/gc/GC.cpp:4268
#7  0x00005555581056e3 in js::gc::GCRuntime::minorGC (this=0x7ffff7718768, reason=JS::GCReason::OUT_OF_NURSERY, phase=js::gcstats::PhaseKind::MINOR_GC) at js/src/gc/GC.cpp:4240
#8  0x000055555811151a in js::gc::GCRuntime::tryNewNurseryBigInt<(js::AllowGC)1> (this=0x7ffff7718768, cx=0x7ffff772a100, thingSize=16, kind=js::gc::AllocKind::BIGINT) at js/src/gc/Allocator.cpp:263
#9  0x0000555558111241 in js::gc::detail::AllocateBigInt<(js::AllowGC)1> (cx=0x7ffff772a100, heap=js::gc::DefaultHeap) at js/src/gc/Allocator.cpp:302
#10 0x000055555766ca10 in js::gc::CellAllocator::NewCell<JS::BigInt, (js::AllowGC)1, js::gc::InitialHeap&> (cx=0x7ffff772a100, args=@0x7fffffff99a7: js::gc::DefaultHeap) at js/src/gc/Allocator.h:120
#11 0x000055555766431d in JSContext::newCell<JS::BigInt, (js::AllowGC)1, js::gc::InitialHeap&> (this=0x7ffff772a100, args=@0x7fffffff99a7: js::gc::DefaultHeap) at js/src/vm/JSContext.h:267
#12 0x000055555764fa7e in JS::BigInt::createUninitialized (cx=0x7ffff772a100, digitLength=0, isNegative=false, heap=js::gc::DefaultHeap) at js/src/vm/BigIntType.cpp:146
#13 0x00005555576500f5 in JS::BigInt::zero (cx=0x7ffff772a100, heap=js::gc::DefaultHeap) at js/src/vm/BigIntType.cpp:212
#14 0x0000555557655c56 in JS::BigInt::createFromUint64 (cx=0x7ffff772a100, n=0) at js/src/vm/BigIntType.cpp:1755
#15 0x0000555557655cbb in JS::BigInt::createFromInt64 (cx=0x7ffff772a100, n=0) at js/src/vm/BigIntType.cpp:1780
#16 0x0000555558478d2d in js::jit::CreateBigIntFromInt64 (cx=0x7ffff772a100, i64=0) at js/src/jit/VMFunctions.cpp:2141

I don't believe this issue has security impact, but am marking it as a security issue as a precaution.

Group: core-security → javascript-core-security

This looks similar to bug 1160884, but I don't know how bad this is nowadays. Regressions from bug 1536699 and bug 1065894 (for DataView).

Assignee: nobody → andrebargull
Status: NEW → ASSIGNED

Based on the comment int he patch, I am marking this bug as sec-high & csectype-uaf despite seeing a nullptr crash in the crash report.
It sounds like the garbage collection of the nursery allocated BigInt could yield a reused nursery allocation.

Fell free to adjust the security rating.

Severity: -- → S3
Priority: -- → P1

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:anba, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Flags: needinfo?(andrebargull)

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #5)

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.

The severity was explicitly set to S3 in comment #3, therefore I'll leave it as is for now.

Flags: needinfo?(andrebargull)

Comment on attachment 9295432 [details]
Bug 1791520: Add some keep alive annotations. r=jandem!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: The patch changes the NeedsKeepAlive function which was added bug 1160884. So an attacker could infer that a similar problem as in bug 1160884 was found. Additionally the patch only handles MIR instructions which return BigInt values, which further narrows down the set of possibly faulty instructions. OTOH the bug is GC-dependent, so in an exploit the (GC) nursery needs to be full at the correct time to ensure that the allocation fails and we then trigger a minor garbage collection. Getting the GC into the correct state can be a bit tricky and it may prevent that any trivial exploit attempts can easily trigger this bug.
  • 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 supported
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?:
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause any regressions. We only insert an additional instruction (MKeepAliveObject) to keep an object alive in a register. This instruction is also used in many other situations, so we don't expect to see any issues adding it here, too.

Theoretically there could be performance regressions, because an additional value is kept alive in a register, but most platforms have plenty of free registers, so this shouldn't matter in practice. And on top of that, this only affects MIR instructions which return BigInt, which we don't expect to see in performance critical code.

No additional testing should be needed. This code is covered by existing tests.

  • Is Android affected?: Yes
Attachment #9295432 - Flags: sec-approval?

Comment on attachment 9295432 [details]
Bug 1791520: Add some keep alive annotations. r=jandem!

Approved to land and request uplift

Attachment #9295432 - Flags: sec-approval? → sec-approval+
Whiteboard: [reminder-test 2022-11-03]

The bug is marked as tracked for firefox106 (beta) and tracked for firefox107 (nightly). However, the bug still has low severity.

:sdetar, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Flags: needinfo?(sdetar)

:anba can this be landed now?

Flags: needinfo?(sdetar) → needinfo?(andrebargull)

Comment on attachment 9295432 [details]
Bug 1791520: Add some keep alive annotations. r=jandem!

Beta/Release Uplift Approval Request

  • User impact if declined: Possible use-after-free
  • 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 only insert an additional instruction (MKeepAliveObject) to keep an object alive in a register. This instruction is also used in many other situations, so we don't expect to see any issues adding it here, too.
  • String changes made/needed:
  • 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:
  • Fix Landed on Version: 107
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We only insert an additional instruction (MKeepAliveObject) to keep an object alive in a register. This instruction is also used in many other situations, so we don't expect to see any issues adding it here, too.
Attachment #9295432 - Flags: approval-mozilla-esr102?
Attachment #9295432 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Attachment #9295432 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9295432 [details]
Bug 1791520: Add some keep alive annotations. r=jandem!

Approved for ESR 102.4.0

Attachment #9295432 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: qe-verify-
Whiteboard: [reminder-test 2022-11-03] → [reminder-test 2022-11-03][post-critsmash-triage]
Whiteboard: [reminder-test 2022-11-03][post-critsmash-triage] → [reminder-test 2022-11-03][post-critsmash-triage][adv-main106+]
Whiteboard: [reminder-test 2022-11-03][post-critsmash-triage][adv-main106+] → [reminder-test 2022-11-03][post-critsmash-triage][adv-main106+][adv-esr102.4+]
Attached file advisory.txt

Open to better descriptions, but the comments haven't seemed very conclusive about what is going on and what happens.

If you're going to issue an advisory, could you credit "Samuel Groß and Carl Smith of Google V8 Security" for this report? Thanks!

Yes - will do in the final one (i.e. not updating the attachment.)

Alias: CVE-2022-42928
Flags: needinfo?(andrebargull)
Group: core-security-release
Pushed by andre.bargull@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b7c6834859a4
Add GC unsafe region instructions for JIT code. r=jandem

3 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2022-11-03] .

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

Flags: needinfo?(andrebargull)
Whiteboard: [reminder-test 2022-11-03][post-critsmash-triage][adv-main106+][adv-esr102.4+] → [post-critsmash-triage][adv-main106+][adv-esr102.4+]

Clearing NI: The test landed yesterday.

Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.