1720031 - (CVE-2021-29984) Assertion failure: !IsInsideNursery(thing) at AtomMarking.cpp:206

Status: RESOLVED FIXED

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

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

The following sample triggers MOZ_ASSERT(!IsInsideNursery(thing)); at this location (in debug builds):
https://searchfox.org/mozilla-central/rev/6550c23862b4ed287843057fd30dbae789bd9b0b/js/src/gc/AtomMarking.cpp#206
Release builds crash with a NULL-deref here:
https://searchfox.org/mozilla-central/rev/6550c23862b4ed287843057fd30dbae789bd9b0b/js/src/util/Text.h#99
Older versions (e.g. git c4573875e2765595093d23b2e73cfa3a976a4ed0 from Mar 2019) crash with:
Hit MOZ_CRASH(*** Zone mismatch 0x7f7c049f4000 vs. 0xfffe2f2f2f2f2f2f at argument 0) at js/src/vm/JSContext-inl.h:48

shell arguments:

obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --baseline-warmup-threshold=10 --ion-warmup-threshold=100 crash.js

function main() {
    const v13 = []; 
    let v15 = "1";
    for (let v16 = "0"; v16 !== 65535; v16 = v16 + v15) {
        if (v16) {
            v15 = "0";
        }   
        const v21 = ["9",v16];
        Reflect.apply(String.fromCharCode,String,v21);
    }   
}
main();

Comment 1

[Christian Holler (:decoder)]

This reproduces just with --no-threads --fuzzing-safe.

Comment 2

[Steven DeTar [:sdetar]]

Jonco: would you be the right person to look at this?

Comment 3

[Andrew McCreight [:mccr8]]

Based on the poison value in the assertion, I assume it is some kind of UAF: "Zone mismatch 0x7f7c049f4000 vs. 0xfffe2f2f2f2f2f2f at argument 0"

Comment 4

[Andrew McCreight [:mccr8]]

Ah, right, that's from older versions. I guess it could still be in newer versions, but we assert in a different way.

Comment 5

[Jon Coppeard (:jonco)]

It's an assertion failure checking a string that was allocated in the nursery by JIT code and then moved by nursery collection. It could be a missing barrier in JIT code somewhere.

Bisection blames bug 1689413 which is not terribly helpful.

Crash backtrace:

#0  0x00005587ff8cf4f6 in js::gc::AtomMarkingRuntime::atomIsMarked<JSAtom> (this=0x7fe5da23c808, zone=0x7fe5d9fea000,
    thing=0x76afbe3ff88) at /home/jon/clone/dev/js/src/gc/AtomMarking.cpp:206
#1  0x00005587fecf3b1f in js::ContextChecks::checkAtom<JSAtom> (this=0x7ffd7c7e43d0, thing=0x76afbe3ff88, argIndex=0)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:108
#2  0x00005587fecf38a2 in js::ContextChecks::check (this=0x7ffd7c7e43d0, str=0x76afbe3ff88, argIndex=0)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:119
#3  0x00005587fecf376c in js::ContextChecks::check (this=0x7ffd7c7e43d0, v=..., argIndex=0)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:133
#4  0x00005587fecf35b7 in js::ContextChecks::check (this=0x7ffd7c7e43d0, args=..., argIndex=0)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:160
#5  0x00005587fecf3474 in JSContext::checkImpl<JS::CallArgs> (this=0x7fe5da219000, args=...)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:213
#6  0x00005587fecf3317 in JSContext::check<JS::CallArgs> (this=0x7fe5da219000, args=...)
    at /home/jon/clone/dev/js/src/vm/JSContext-inl.h:220
#7  0x00005587fecedd2e in CallJSNative (cx=0x7fe5da219000,
    native=0x5587fefec3a0 <js::str_fromCharCode(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call,
    args=...) at /home/jon/clone/dev/js/src/vm/Interpreter.cpp:422
#8  0x00005587fecdbf06 in js::InternalCallOrConstruct (cx=0x7fe5da219000, args=..., construct=js::NO_CONSTRUCT,
    reason=js::CallReason::Call) at /home/jon/clone/dev/js/src/vm/Interpreter.cpp:511
#9  0x00005587fecdc6b6 in InternalCall (cx=0x7fe5da219000, args=..., reason=js::CallReason::Call)
    at /home/jon/clone/dev/js/src/vm/Interpreter.cpp:571
#10 0x00005587fecdc767 in js::Call (cx=0x7fe5da219000, fval=..., thisv=..., args=..., rval=...,
    reason=js::CallReason::Call) at /home/jon/clone/dev/js/src/vm/Interpreter.cpp:588
#11 0x00005587ffba8ab9 in js::jit::InvokeFunction (cx=0x7fe5da219000, obj=..., constructing=false,
    ignoresReturnValue=false, argc=2, argv=0x7ffd7c7e4a60, rval=...)
    at /home/jon/clone/dev/js/src/jit/VMFunctions.cpp

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #5)
Bisection blames this changeset, but it turns out this is due to the change in object size affecting when we trigger a nursery collection:

changeset:   569882:a11a96dce11b
user:        Jan de Mooij <[e-mail]>
date:        Fri Mar 05 19:10:12 2021 +0000
summary:     Bug 1689413 part 16 - Remove JSObject group field. r=tcampbell,jonco

Simplified testcase:

function main() {
  let a = "0";
  let b = "0";
  for (let i = 0; i < 10000; i++) {
    if (b) {
      a = "0";
    }   
    Reflect.apply(String.fromCharCode, String, ["0", b]);
    b = b + a;
  }   
}
main();

The testcase performs a single minor GC and crashes shortly afterwards. The collection happens in js::ConcatStrings and the engine crashes shortly afterwards:

#0  js::Nursery::collect (this=0x7f8b3ef411c8, kind=GC_NORMAL, reason=JS::GCReason::OUT_OF_NURSERY)
    at /home/jon/clone/dev/js/src/gc/Nursery.cpp:1015
#1  0x00005640de1682da in js::gc::GCRuntime::collectNursery (this=0x7f8b3ef3e788, kind=GC_NORMAL,
    reason=JS::GCReason::OUT_OF_NURSERY, phase=js::gcstats::PhaseKind::MINOR_GC)
    at /home/jon/clone/dev/js/src/gc/GC.cpp:7793
#2  0x00005640de16b5cf in js::gc::GCRuntime::minorGC (this=0x7f8b3ef3e788, reason=JS::GCReason::OUT_OF_NURSERY,
    phase=js::gcstats::PhaseKind::MINOR_GC) at /home/jon/clone/dev/js/src/gc/GC.cpp:7759
#3  0x00005640de17fc94 in js::gc::GCRuntime::tryNewNurseryString<(js::AllowGC)1> (this=0x7f8b3ef3e788,
    cx=0x7f8b3ef4c000, thingSize=24, kind=js::gc::AllocKind::STRING) at /home/jon/clone/dev/js/src/gc/Allocator.cpp:177
#4  0x00005640de1803f3 in js::AllocateStringImpl<JSString, (js::AllowGC)1> (cx=0x7f8b3ef4c000, heap=js::gc::DefaultHeap)
    at /home/jon/clone/dev/js/src/gc/Allocator.cpp:217
#5  0x00005640dd92fbaf in js::AllocateString<JSRope, (js::AllowGC)1> (cx=0x7f8b3ef4c000, heap=js::gc::DefaultHeap)
    at /home/jon/clone/dev/js/src/gc/Allocator.h:61
#6  0x00005640dd91f028 in JSRope::new_<(js::AllowGC)1> (cx=0x7f8b3ef4c000, left=..., right=..., length=2016,
    heap=js::gc::DefaultHeap) at /home/jon/clone/dev/js/src/vm/StringType-inl.h:171
#7  0x00005640ddbc0094 in js::ConcatStrings<(js::AllowGC)1> (cx=0x7f8b3ef4c000, left=..., right=...,
    heap=js::gc::DefaultHeap) at /home/jon/clone/dev/js/src/vm/StringType.cpp:948
#8  0x00001db928c53fdd in ?? ()

This doesn't reproduce with --no-ion (and does reproduce with --ion-eager). My guess is that we're not tracing something we should be in the JITs.

Jan does anything stand out to you?

Comment 7

[Jan de Mooij [:jandem]]

This is a really good find.

What happens is that for initializing the array literal we have StoreElement and SetInitializedLength instructions. The instruction reordering pass then moves the Concat between these two instructions. If the concatenation then triggers a GC we don't mark the element because we didn't bump the initialized length yet.

Not sure yet what's the best way to fix this. "Can GC" isn't known at the MIR level. The simplest fix is probably to teach ReorderInstructions to never move anything before a SetInitializedLength instruction.

Comment 8

[Jan de Mooij [:jandem]]

The spot fix for this one is easy but I'm doing some perf measurements to see if we can disable instruction reordering for JS.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #8)

The spot fix for this one is easy but I'm doing some perf measurements to see if we can disable instruction reordering for JS.

I looked into this and some Octane-subtests do seem a bit affected so I'll go with the simpler fix + a new JitOption to help the fuzzers.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1720031 - Check for SetInitializedLength when reordering. r?iain!

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1720031 - Add comment, tests, and a new JitOption. r?iain!

Depends on D120626

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 9232633 [details]
Bug 1720031 - Check for SetInitializedLength when reordering. r?iain!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easy, but it's likely doable with some skill and effort.
• 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?: Yes
• If not, how different, hard to create, and risky will they be?: Trivial to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions, it just disables an optimization in a particular case.

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9232633 [details]
Bug 1720031 - Check for SetInitializedLength when reordering. r?iain!

Approved to land and uplift

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 9232633 [details]
Bug 1720031 - Check for SetInitializedLength when reordering. r?iain!

Beta/Release Uplift Approval Request

• User impact if declined: Security issues, crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): It just disables an optimization in a specific case.
• String changes made/needed: N/A

Comment 15

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

Check for SetInitializedLength when reordering. r=iain
https://hg.mozilla.org/integration/autoland/rev/7528462f1eef91beaf5a97c78c44b3c35b7f5cd1
https://hg.mozilla.org/mozilla-central/rev/7528462f1eef

Comment 16

[Pascal Chevrel:pascalc]

Jan, does it need to land on esr78? Thanks

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Pascal Chevrel:pascalc from comment #16)

Jan, does it need to land on esr78? Thanks

Yes, thanks for noticing! I'm too used to a single supported ESR release...

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9232633 [details]
Bug 1720031 - Check for SetInitializedLength when reordering. r?iain!

Thanks, approved for beta and esr78. This will be inherited in esr91 with the 91 to esr91 merge.

Comment 19

[Pascal Chevrel:pascalc]

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

Comment 20

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-esr78/rev/d05f240578e8

Comment 21

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 22

[Tom Ritter [:tjr]]

Attachment: advisory.txt