1679003 - (CVE-2020-16042) Uninitialised memory read with BigInt right-shift

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[André Bargull [:anba]]

Test case:

for (let i = 0, j = 0; i < 1_000_000; ++i) {
  let x = (-0xffffffffffffffff_ffffffffffffffffn >> 0x40n);
  if (x != -0x10000000000000000n) {
    print(x.toString(16));
    if (++j == 10) break;
  }
}

Prints -2f2f2f2f2f2f2f300000000000000000 ten times. 0x2f is the JS_FRESH_NURSERY_PATTERN. The last byte is 0x30 because +1 is added to 0x2f. With JSGC_DISABLE_POISONING=1 uninitialised memory can be observed.

The fix is relatively simple, in the if (!bitsShift) {...} block in https://searchfox.org/mozilla-central/rev/48151c3619b0b5b4b92d5f14eaa839e619605d2d/js/src/vm/BigIntType.cpp#2265, add:

if (!bitsShift) {
  // We must manually initialize the overflow space, if it was allocated.
  result->setDigit(resultLength - 1, 0);
  ...

---

The same BigInt implementation is also used in JSC and V8, so the bug can be reproduced there, too, which makes this a cross-browser issue. That means for example, releasing a regression test could easily be used to discover the same issue in the other browsers.

Comment 1

[André Bargull [:anba]]

V8: https://bugs.chromium.org/p/chromium/issues/detail?id=1151890
JSC: https://bugs.webkit.org/show_bug.cgi?id=219253

Comment 2

[André Bargull [:anba]]

The V8 fix included a regression test using the test case from comment #0: https://github.com/v8/v8/commit/e82a3b4d47a93ab64f07d8c03e3cd17b6b961c3f

Comment 3

[Jason Orendorff [:jorendorff]]

Well, that simplifies things I suppose.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1679003 - Fix uninitialized overflow digit in BigInt right shift. r=jandem.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 9190333 [details]
Bug 1679003 - Fix uninitialized overflow digit in BigInt right shift. r=jandem.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very doable, unfortunately.

NOTE: The bug is in code we share with JSC and V8. V8 has already landed a patch in the open with a check-in comment and test that paint a bulls-eye on the security problem.

Details: This is an info leak. The bug can be exploited to read pointer-sized words from the previous contents of the nursery. Reading one word clobbers some nearby words, but an exploit could do one pass to fill up the nursery with words it wants to read, then another pass just doing right-shifts to read words. You can quickly get a bunch of pointer bits this way, or anything else in the nursery, including strings. The nursery is per-runtime, so different trust domains share it. Even chrome JS.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: Straightforward.
• How likely is this patch to cause regressions; how much testing does it need?: Won't regress. No testing needed beyond the automated test in the patch.

Comment 6

[Andy Wingo [:wingo]]

Well. That's embarrassing! Thank you for the Cc and for the fix.

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Bumping this to sec-critical because Google landed a working testcase with their commit - although the full details of the bug are not public there's not much that's private about it.

Comment 8

[Andrew McCreight [:mccr8]]

Are there any branches that this does not affect? We should set the tracking and affected flags for them.

Comment 9

[Ted Campbell [:tcampbell]]

Looks like mozilla66 introduced it.

Comment 10

[Andrew McCreight [:mccr8]]

Thanks, Ted.

[Tracking Requested - why for this release]: Chrome published a test case that triggers this issue, so we should fix it quickly.

Comment 11

[Daniel Veditz [:dveditz]]

Chrome bug for this is https://bugs.chromium.org/p/chromium/issues/detail?id=1151890

It appears they've merged this to m87 which is the current Release version, so I guess they plan on including this in an update soon.

Comment 12

[Daniel Veditz [:dveditz]]

As I understand it we're thinking that the normal 84 release in two weeks is soon enough to fix this, but that's somewhat at odds with the "critical" rating.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 9190333 [details]
Bug 1679003 - Fix uninitialized overflow digit in BigInt right shift. r=jandem.

sec-approval+ -- normally for a sec-critical bug we'd need to clear a path to uplift approval this close to release (in the 4 week cycles we're always close to release), but since Chrome let the cat out of the bag already there's no point in worrying about that.

Comment 14

[Daniel Veditz [:dveditz]]

Please request beta and ESR78 uplift approval. We'll want to make sure this gets into the next release and RC builds are in about a week.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/00378c0b8d258a9ebfaf734f0176bf4466e30d50

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 9190333 [details]
Bug 1679003 - Fix uninitialized overflow digit in BigInt right shift. r=jandem.

Beta/Release Uplift Approval Request

• User impact if declined: likely info-read vulnerability
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): one-liner already taken by V8
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: likely info-read vulnerability
• Fix Landed on Version: 84
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): one-liner already taken by V8
• String or UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9190333 [details]
Bug 1679003 - Fix uninitialized overflow digit in BigInt right shift. r=jandem.

Approved for 84.0b7 and 78.6esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3147bf02ca67

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00378c0b8d25

Comment 20

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

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/6f1267e4aff2

Comment 22

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 23

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt