Closed Bug 1647293 (CVE-2020-15656) Opened 4 years ago Closed 4 years ago

TypeSet containing only MagicOptimizedArguments in improveTypesAtTypeOfCompare

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 --- wontfix
firefox-esr78 79+ fixed
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 + fixed
firefox80 + fixed

People

(Reporter: cffsmith, Assigned: jandem)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main79+][adv-ESR78.1+])

Attachments

(3 files, 1 obsolete file)

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.

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

Group: core-security → javascript-core-security
Flags: needinfo?(jdemooij)
Severity: -- → S3
Priority: -- → P1
Flags: needinfo?(sdetar)

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.

Flags: needinfo?(sdetar)
Flags: needinfo?(jdemooij)
Keywords: sec-moderate

Depends on D82155

Assignee: nobody → jdemooij
Attachment #9161184 - Attachment description: Bug 1647293 - Don't improve TypeSets containing the magic-args type.r ?iain → Bug 1647293 - Don't improve TypeSets containing the magic-args type. r?iain
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Flags: needinfo?(jdemooij)

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
Flags: needinfo?(jdemooij)
Attachment #9161184 - Flags: approval-mozilla-esr78?
Attachment #9161184 - Flags: approval-mozilla-beta?

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.

Attachment #9161184 - Flags: approval-mozilla-esr78?
Attachment #9161184 - Flags: approval-mozilla-esr78+
Attachment #9161184 - Flags: approval-mozilla-beta?
Attachment #9161184 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main79+]
Whiteboard: [post-critsmash-triage][adv-main79+] → [post-critsmash-triage][adv-main79+][adv-ESR78.1+]
Attached file advisory.txt (obsolete) —

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.

Flags: needinfo?(jdemooij)
Alias: CVE-2020-15656

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

Flags: needinfo?(jdemooij) → needinfo?(fbraun)
Attached file advisory.txt

Thanks!

Attachment #9165878 - Attachment is obsolete: true
Flags: needinfo?(fbraun)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.