CVE-2020-26950: 1675905 - (CVE-2020-26950) Write side effects in MCallGetProperty opcode not accounted for

Closed Bug 1675905 (CVE-2020-26950) Opened 1 year ago Closed 1 year ago

The root cause is in the |MIR.h| file and the opcode |MCallGetProperty|:

AliasSet getAliasSet() const override {
    if (!idempotent_) {
      return AliasSet::Store(AliasSet::Any);
    }
    return AliasSet::Load(AliasSet::ObjectFields | AliasSet::FixedSlot |
                          AliasSet::DynamicSlot);
  }

if |idempotent_| is true, compiler will think this opcode does NOT have write side effect. But this is wrong.

In the function |createThisScripted|, it will emit a |MCallGetProperty| which |idempotent_| is true:

 else {
    MCallGetProperty* callGetProp =
        MCallGetProperty::New(alloc(), newTarget, names().prototype);
    callGetProp->setIdempotent();
    getProto = callGetProp;
  }

It use this opcode to get callee.prototype, and this operatioin may call function |func_reslove| and write the |prototype| to slots, so it may be grow the slots buffer and update callee’s slots buffer address. This will lead to UaF problem in JIT code as JIT code may be use the old buffer address after the grow.

https://twitter.com/TianfuCup/status/1324900642393976832

Got this zip from them; awaiting the password.

Password is tfc2020@cic@tfc2020

Component: Security → JavaScript Engine: JIT

Summary: Tianfu Cup 2020 Exploit → Write side effects in MCallGetProperty opcode not accounted for

Group: core-security → javascript-core-security

The zip file doesn’t work trivially with all typical unzippers. Attaching PoC directly.

I wrote a PoC based on theirs. Repros on m-c tip, debug build:

$ obj-shell-dbg/dist/bin/js --no-warp --no-threads poc.js
poc.js:23:17 Error: Assertion failed: got -437918235, expected 2

That’s a poison value.

This one triggers a crash in debug and opt builds.

Crashes content process in 82.0.2 on Mac.

IIUC, this might not affect 83+ due to Warp being enabled, but I’ll leave that for someone on the JS team to confirm and set.

We should fix 83, because a warp/ion experiment is supposed to happen when release 83 ships.

(In reply to Ted Campbell [:tcampbell] from comment #10)

We should fix 83, because a warp/ion experiment is supposed to happen when release 83 ships.

Thanks for confirming. Setting flags accordingly.

Assignee: nobody → tcampbell

Attachment #9186450 - Attachment description: Bug 1675905 - Simplify IonBuilder::createThisScripted → Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain!

Status: NEW → ASSIGNED

Attachment #9186450 - Attachment description: Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain! → Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!

Attachment #9186450 - Attachment description: Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem! → Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain!

Comment on attachment 9186450 [details]
Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch suggests that MCallGetProperty is bad in this context, but doesn’t directly point out the fun_resolve reallocation that is also needed to exploit. This aspect was novel to us and deriving from patch would require experience with jit exploitation.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: Patch applies onto FF78 through 84
• How likely is this patch to cause regressions; how much testing does it need?: We are removing a very rare case that existed solely as a perf trick. Correctness risk of this patch is low, and primary risk is a performance cliff in rare cases. We’ve added a perf mitigation in this patch that avoids Ion in rare cases and sticks with the more predictable BaselineJIT.

Comment on attachment 9186450 [details]
Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: External report of sec-crit. TianFu Cup 2020.
• User impact if declined: Remote-code-execution in Content process.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Correctness risk is low since we are removing a rare edge case added as a hypothetical performance fix.
  Perfomance risk is mitigated by an addition in this patch to rely on BaselineJIT in the very rare case instead of IonMonkey doing unnecessary compiles. The only place I’ve run into this rare case is heavily obfuscated JavaScript that is not performance critical.
• String or UUID changes made by this patch: None

Comment on attachment 9186450 [details]
Bug 1675905 - Simplify IonBuilder::createThisScripted. r?jandem!,iain!

Beta/Release Uplift Approval Request

• User impact if declined: External sec-crit. TianFu Cup 2020.
  Remote-code-execution in Content process.

• Is this code covered by automated tests?: Yes

• Has the fix been verified in Nightly?: No

• Needs manual test from QE?: Yes

• If yes, steps to reproduce: See chemspill QA Plan.
  A crash-test HTML file is on bug. It may require 1-line tweaks for different versions.

• List of other uplifts needed: None

• Risk to taking this patch: Low

• Why is the change risky/not risky? (and alternatives if risky): Correctness risk is low since we are removing a rare edge case added as a hypothetical performance fix.
  Perfomance risk is mitigated by an addition in this patch to rely on BaselineJIT in the very rare case instead of IonMonkey doing unnecessary compiles. The only place I’ve run into this rare case is heavily obfuscated JavaScript that is not performance critical.
  Note: Affected code is off-by-default in 83+, so perf risk is very short lived.

• String changes made/needed: None

• ESR-78: Affected. Patch applies cleanly.

• GeckoView-81: Affected. This previous version is still required for some mobile builds.

• Release-82: Affected.

• Beta-83: Disabled by default. A experiment is planned when this hits release that will re-enable Ion for a small population for limited time.

• Nightly-84: Disabled by default.

• Impacted code will be permanently removed from tree in Nightly-85.

Updated version of browser test with support for pre-82 and 82+ versions. On affected builds, this will crash tab. On fixed builds, this will render "Passed".

Attachment #9186486 - Attachment description: Crashtest for QA (FF72-84) → Crashtest for QA (FF78-84)

Status: ASSIGNED → RESOLVED

Closed: 1 year ago

Resolution: — → FIXED

Target Milestone: — → 84 Branch

Attached is an advisory; if it can be improved please leave suggestions.

Flags: needinfo?(jdemooij)

Alias: tfc-2020 → CVE-2020-26950

Whiteboard: [tfc-2020]

Some useful background from Ted in chat that I don’t see here or in phabricator. Might be good history to preserve:

This issue is exactly the sort of problem that motivated the design of Warp. In two weeks, Warp will be shipped to Release FF83 and we hopefully can put many of this family of security issues behind us. This issue has a lot in common with [the] 0-day at start of the Whistler 2019 and was the final straw that kicked off the Warp project. A huge congrats to @jandem and all the others for getting this designed, built, and shipped in less than a year. We’ve focused on the performance side mostly when discussing Warp, but improving security was one of the biggest motivations behind the scenes.

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #23)

Attached is an advisory; if it can be improved please leave suggestions.

Looks good to me, but the text is truncated at the end.

Flags: needinfo?(jdemooij)

Status: RESOLVED → VERIFIED

Flags: qe-verify+

The bug fix was also verified on mobile on the following builds and devices:

• versions: Nightly 84, Beta 83.0.0-beta.4 & RC 82.1.3, Focus Beta 8.8.4
• devices: Xiaomi Mi Pad 2 (Android 5.1, x86), OnePlus A3 (Android 6.0.1), Nexus 9 (Android 7.1.1), Motorola Moto G6 (Android 8), Google Pixel 3a (Android 11), Huawei Mate 20 Lite (Android 10).

Hello,

Verified the official esr 78.5.0 for good measure. No issues.

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?(tcampbell)

Whiteboard: [tfc-2020] → [tfc-2020][sec-survey]

Flags: needinfo?(tcampbell)

Group: javascript-core-security → core-security-release

Group: core-security-release