Closed Bug 1720031 (CVE-2021-29984) Opened 3 years ago Closed 3 years ago

Assertion failure: !IsInsideNursery(thing) at AtomMarking.cpp:206

Categories

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

x86_64
Unspecified
defect

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox-esr78 91+ fixed
firefox90 --- wontfix
firefox91 --- fixed
firefox92 --- fixed

People

(Reporter: lukas.bernhard, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [sec-survey][adv-main91+][adv-esr78.13+])

Attachments

(3 files)

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();

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

Group: firefox-core-security → javascript-core-security
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript: GC
Ever confirmed: true
Flags: sec-bounty?
Product: Firefox → Core
Hardware: Unspecified → x86_64

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

Flags: needinfo?(jcoppeard)

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

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

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

(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 <jdemooij@mozilla.com>
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?

Flags: needinfo?(jcoppeard) → needinfo?(jdemooij)

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.

Component: JavaScript: GC → JavaScript Engine: JIT
Keywords: sec-high
Severity: -- → S3
Priority: -- → P1
Assignee: nobody → jdemooij

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.

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

Flags: needinfo?(jdemooij)

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

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

Approved to land and uplift

Attachment #9232633 - Flags: sec-approval? → sec-approval+

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
Attachment #9232633 - Flags: approval-mozilla-esr91?
Attachment #9232633 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

Jan, does it need to land on esr78? Thanks

Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
Attachment #9232633 - Flags: approval-mozilla-esr78?

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

Attachment #9232633 - Flags: approval-mozilla-esr91?
Attachment #9232633 - Flags: approval-mozilla-esr78?
Attachment #9232633 - Flags: approval-mozilla-esr78+
Attachment #9232633 - Flags: approval-mozilla-beta?
Attachment #9232633 - Flags: approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+

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.

Flags: needinfo?(jdemooij)
Whiteboard: [sec-survey]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Flags: needinfo?(jdemooij)
Whiteboard: [sec-survey] → [sec-survey][adv-main90+]
Whiteboard: [sec-survey][adv-main90+] → [sec-survey][adv-main91+]
Attached file advisory.txt
Alias: CVE-2021-29984
Whiteboard: [sec-survey][adv-main91+] → [sec-survey][adv-main91+][adv-esr78.13+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: