Closed Bug 1640737 (CVE-2020-12417) Opened 8 months ago Closed 8 months ago

Assertion failure: LoadElement instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:1862

Categories

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

ARM64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 78+ fixed
firefox-esr78 78+ fixed
firefox76 --- wontfix
firefox77 - wontfix
firefox78 + fixed

People

(Reporter: deian, Assigned: jandem)

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage][sec-survey][adv-main78+][adv-esr68.10+])

Attachments

(4 files)

Our JITing produced ( run with --ion-eager --ion-offthread-compile=off) two crash files, both triggering the assert here. Unfortunately, the stack trace wasn't very useful, so I'm not sure what the root cause is. (On x86 this doesn't crash.)

function main() {
let v2 = 0;
do {
    const v3 = v2 + 1;
    v2 = v3;
} while (v2 < 7);
for (let v7 = 0; v7 < 2; v7++) {
}
let v10 = 0;
while (v10 < 8) {
    for (let v14 = 0; v14 < 6; v14++) {
    }
    const v16 = [13.37,13.37,13.37];
    const v18 = [1337,1337];
    const v19 = {__proto__:v18,e:1337,valueOf:v16};
    for (let v23 = 0; v23 < 100; v23++) {
        const v25 = [13.37,v19];
        const v26 = v25.flat();
    }
}
}
main();
gc();
function main() {
let v2 = 0;
do {
    const v3 = v2 + 1;
    v2 = v3;
} while (v2 < 7);
for (let v7 = 0; v7 < 2; v7++) {
}
let v10 = 0;
while (v10 < 8) {
    for (let v14 = 0; v14 < 6; v14++) {
    }
    const v16 = [13.37,13.37,13.37];
    const v18 = [1337,1337];
    const v19 = {__proto__:v18,e:1337,valueOf:v16};
    for (let v23 = 0; v23 < 100; v23++) {
        const v25 = [13.37,v19];
        const v26 = v25.flat();
    }
}
}
main();
gc();
Group: core-security → javascript-core-security

The two tests are identical AFAICS?

Yep, sorry! I also double checked the files to make sure I didn't copy incorrectly. Apparently the fuzzer thought they were different cases. Woops.

No problem, thanks for reporting! The not-x86 thing is interesting.

More minimal test that doesn't rely on self-hosted code:

function g(arr) {
    var res = [];
    for (var i = 0; i < arr.length; i++) {
        var el = arr[i];
        res.push(el);
    }
    return res;
}
function f() {
    for (var i = 0; i < 2; i++) {
        var obj = {__proto__: []};
        for (var j = 0; j < 100; j++) {
            g([13.37, obj]);
        }
    }
}
f();

I think I know what's going on here. Really good find.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

This is an ARM64-specific bug. Exploitable, could be causing crashes in the wild.

The culprit is the MacroAssembler::extractTag implementation for TypedOrValueRegister:

  MOZ_MUST_USE Register extractTag(const TypedOrValueRegister& reg,
                                   Register scratch) {
    if (reg.hasValue()) {
      return extractTag(reg.valueReg(), scratch);
    }
    mov(ImmWord(MIRTypeToTag(reg.type())), scratch);
    return scratch;
  }

The problem is the last part: the ARM64 backend assumes ValueTags are sign-extended, as explained by this long comment. The code above doesn't sign-extend the tag. The result is that we load an object tag into the register, then we do branchTestNumber under guardTypeSet, and due to the tag not being sign-extended, we incorrectly let an object pass through the type barrier...

How to fix this: extractTag on a known-type seems kind of silly and suboptimal to begin with. I have a patch to specialize guardTypeSet for TypedOrValueRegister, I think we should land that. Then in a later patch we can remove this extractTag overload - this way it's really hard to figure out what the problem is just from the patch.

Jan, are you asking for tracking for 77 because you want to uplift the fix to mozilla-release and esr68 before we ship? (we ship 77 next Tuesday)

Flags: needinfo?(jdemooij)

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

Jan, are you asking for tracking for 77 because you want to uplift the fix to mozilla-release and esr68 before we ship? (we ship 77 next Tuesday)

I just requested tracking to get this bug on your radar. It would be nice to have this fixed for 77 though.

Flags: needinfo?(jdemooij)

I think this can wait the next 78 cycle as we are building 77RC2 today.

Super interesting. This test case was generated by Fuzzilli, by the way!

Comment on attachment 9152016 [details]
Bug 1640737 part 1 - Add a guardTypeSet specialization for TypedOrValueRegister. r?iain!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Difficult. (It's not clear from the patch that it's ARM64-specific, this patch doesn't touch the buggy code.)
  • 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?: No
  • If not, how different, hard to create, and risky will they be?: Should apply or be easy to backport.
  • How likely is this patch to cause regressions; how much testing does it need?: Not very likely. A green Try run should be sufficient.
Attachment #9152016 - Flags: sec-approval?
Severity: -- → S2
Priority: -- → P1

Comment on attachment 9152016 [details]
Bug 1640737 part 1 - Add a guardTypeSet specialization for TypedOrValueRegister. r?iain!

sec-approval=dveditz

Attachment #9152016 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(jdemooij)
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Comment on attachment 9152016 [details]
Bug 1640737 part 1 - Add a guardTypeSet specialization for TypedOrValueRegister. r?iain!

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Crashes or security bugs on ARM64 platforms.
  • Fix Landed on Version: 78
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Already landed on Nightly. Patch applies to ESR 68.
  • String or UUID changes made by this patch: None
Flags: needinfo?(jdemooij)
Attachment #9152016 - Flags: approval-mozilla-esr68?
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]

Hey folks, can we tag this bug for a bounty? If awarded we'd like to donate the cash to, say NAACP Legal Defense and Educational Fund or ACLU. Thanks!

I'll set the sec-bounty flag. It can take some time for the security team to make the decision on that.

Flags: sec-bounty?
Flags: needinfo?(jdemooij)

Comment on attachment 9152016 [details]
Bug 1640737 part 1 - Add a guardTypeSet specialization for TypedOrValueRegister. r?iain!

Approved for 68.10esr.

Attachment #9152016 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attached patch Patch for ESR68Splinter Review

We just had to include the TypeSet::PrimitiveType function that was added after ESR68. I confirmed this builds and passes jit-tests on ESR68.

Flags: needinfo?(jdemooij)
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Attachment #9154482 - Flags: approval-mozilla-esr68+
Attachment #9152016 - Flags: approval-mozilla-esr68+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage][sec-survey] → [post-critsmash-triage][sec-survey][adv-main78+]
Whiteboard: [post-critsmash-triage][sec-survey][adv-main78+] → [post-critsmash-triage][sec-survey][adv-main78+][adv-esr68.10+]
Flags: in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.