1647293 - (CVE-2020-15656) TypeSet containing only MagicOptimizedArguments in improveTypesAtTypeOfCompare

Status: RESOLVED FIXED

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

Description

[Carl Smith]

When running the following script

function foo() {
  let v3 = 0;
  while (v3 != 10000) {
    const v6 = typeof v3 === "number";
    if (v6) {
    } else {
        const v7 = v3 || 8;
    }
    const v8 = v3 + 1;
    v3 = arguments;
    v3 = v8;
  }
}

function main() {
    foo();
}
main();
gc();

Spidermonkey fails on the following assertion:

Assertion failure: !IsMagicType(ins->type()) in js/src/jit/IonBuilder.cpp:2625

jit::AnalyzeArgumentsUsage correctly determines that the arguments object is not needed and can be optimized out, which means that the TypeSet for v3 is the union of types int and MagicOptimizedArguments. It seems that the compiler tries to optimize the typeof compare and propagate the type information to the branches. If the typeof compare is true, v3 has to be a number in the true branch, which means that v3 has to be of type MagicOptimizedArguments in the false branch.
It does not seem to be a security issue as the false branch is always dead code but if the bug is observable in a branch taken at runtime, it might be a security issue like shown in https://bugs.chromium.org/p/project-zero/issues/detail?id=1794, which is why I am reporting this as a security issue. I tried to bisect the bug but failed to do so as I had troubles building spidermonkey versions pre 2017. It does trigger on builds from early 2017 to the latest commit as of now (3d39d3b7dd1b2be30692d4541ea681614e34c786) on the github mirror.

Comment 1

[Andrew McCreight [:mccr8]]

Jan, could you please take an initial look at this? Thanks.

Comment 2

[Jan de Mooij [:jandem]]

Thanks for the excellent bug report! And sorry for the delay.

I think we shouldn't bother optimizing any TypeSets that contain the magic-args type as it just leads to complexity/confusion down the line, as shown here. Also note that this code is (too) complicated and we're working on replacing it in the next <= 6 months with WarpBuilder, it has a massively simpler design, so I'll post the minimal fix for this bug.

I'll mark this sec-moderate since it's not exploitable as far as we know, but we should uplift this to 79/ESR78 since the fix should be safe.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1647293 - Don't improve TypeSets containing the magic-args type. r?iain

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1647293 - Add test. r?iain!

Depends on D82155

Comment 5

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

https://hg.mozilla.org/integration/autoland/rev/fa3efba6247a4edf893956e0049a6283a3163a51
https://hg.mozilla.org/mozilla-central/rev/fa3efba6247a

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 9161184 [details]
Bug 1647293 - Don't improve TypeSets containing the magic-args type. r?iain

Beta/Release Uplift Approval Request

• User impact if declined: This is disabling an optimization in a certain edge case to prevent trouble (assertion failures, maybe crashes).
• 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): It has been on Nightly for a few days and just disables a particular optimization path.
• String changes made/needed: N/A

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161184 [details]
Bug 1647293 - Don't improve TypeSets containing the magic-args type. r?iain

Approved for 79.0b7 and 78.1esr.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ec3c84d6b93
https://hg.mozilla.org/releases/mozilla-esr78/rev/02f8f64c38eb

Comment 9

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Jan, I have a hard time coming up with a decent description. Let me know if this is factually wrong and I'm happy to incorporate your suggestions.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Frederik Braun [:freddy] from comment #9)

Jan, I have a hard time coming up with a decent description. Let me know if this is factually wrong and I'm happy to incorporate your suggestions.

For the first sentence, something like this: "JIT optimizations involving the Javascript <code>arguments</code> object could confuse later optimizations. "

Comment 11

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d93f6f078b1
Add test. r=iain

Comment 13

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/1d93f6f078b1