1640737 - (CVE-2020-12417) Assertion failure: LoadElement instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:1862

Status: RESOLVED FIXED

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

Description

[Deian Stefan]

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

Comment 1

[Jan de Mooij [:jandem]]

The two tests are identical AFAICS?

Comment 2

[Deian Stefan]

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.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[Jan de Mooij [:jandem]]

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

Comment 5

[Jan de Mooij [:jandem]]

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.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1640737 part 1 - Add a guardTypeSet specialization for TypedOrValueRegister. r?iain!

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1640737 part 2 - Add a test, remove now unused code. r?iain!

Depends on D77008

Comment 8

[Pascal Chevrel:pascalc]

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)

Comment 9

[Jan de Mooij [:jandem]]

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

Comment 10

[Pascal Chevrel:pascalc]

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

Comment 11

[mlfbrown]

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

Comment 12

[Jan de Mooij [:jandem]]

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.

Comment 13

[Daniel Veditz [:dveditz]]

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

sec-approval=dveditz

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/e816389af5d2c068cbb337b4e87efe568260acd4

I'll request ESR68 approval soon.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/e816389af5d2

Comment 16

[Jan de Mooij [:jandem]]

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

Comment 17

[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 18

[Deian Stefan]

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!

Comment 19

[Jan de Mooij [:jandem]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 68.10esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/410c3a698d84a90f70c831210f6c5fa52d544b55

Comment 22

[Andreea Pavel [:apavel]]

Backed out for build bustages at MacroAssembler.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&resultStatus=testfailed%2Cbusted%2Cexception&selectedTaskRun=AR0piX-oQk2CfiLJYG-eBQ-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305056264&repo=mozilla-esr68&lineNumber=2559

Backout: https://hg.mozilla.org/releases/mozilla-esr68/rev/2790572dd9f56fcb8cd841fb41f93cec041e7d61

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR68

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

Comment 24

[Julien Cristau [:jcristau]]

Looks like comment 22 flipped the wrong flag.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/47e62b30b420424780a69b7fc9e2fa07c486e8b1

Comment 26

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 27

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

part 2 - Add a test, remove now unused code. r=iain
https://hg.mozilla.org/integration/autoland/rev/d818a9726c698c464396d16f43fc73027a78ee4e
https://hg.mozilla.org/mozilla-central/rev/d818a9726c69