1791520 - (CVE-2022-42928) Nullptr dereference in IsWriteableAddress when coming from CreateBigIntFromInt64

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Samuel Groß]

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.

Comment 1

[André Bargull [:anba]]

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).

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1791520: Add some keep alive annotations. r=jandem!

Comment 3

[Nicolas B. Pierron [:nbp]]

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.

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1791520: Add GC unsafe region instructions for JIT code. r=jandem!

Comment 5

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

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.

Comment 6

[André Bargull [:anba]]

(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.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1791520: Add tests. r=jandem!

Comment 8

[André Bargull [:anba]]

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

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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

Approved to land and request uplift

Comment 10

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

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.

Comment 11

[Steven DeTar [:sdetar]]

:anba can this be landed now?

Comment 12

[Iain Ireland [:iain]]

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.

Comment 13

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

Add some keep alive annotations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d9db6d6fe7d255661578fc862408e4f452a65533
https://hg.mozilla.org/mozilla-central/rev/d9db6d6fe7d2

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5cc9509d1de6

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Approved for ESR 102.4.0

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-esr102/rev/52928ba6fd91

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

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

Comment 18

[Samuel Groß]

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

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7c6834859a4
Add GC unsafe region instructions for JIT code. r=jandem

Comment 21

[Pulsebot]

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

Comment 22

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b7c6834859a4
https://hg.mozilla.org/mozilla-central/rev/caa6ecc16202

Comment 23

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

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.

Comment 24

[André Bargull [:anba]]

Clearing NI: The test landed yesterday.