Security: V8: Type confusion in ElementsAccessorBase::CollectValuesOrEntriesImpl |
||||||||||||||||||||||
Issue descriptionHere's a snippet of the method. https://cs.chromium.org/chromium/src/v8/src/elements.cc?rcl=3cbf26e8a21aa76703d2c3c51adb9c96119500da&l=1051 static Maybe<bool> CollectValuesOrEntriesImpl( Isolate* isolate, Handle<JSObject> object, Handle<FixedArray> values_or_entries, bool get_entries, int* nof_items, PropertyFilter filter) { ... for (int i = 0; i < keys->length(); ++i) { Handle<Object> key(keys->get(i), isolate); Handle<Object> value; uint32_t index; if (!key->ToUint32(&index)) continue; uint32_t entry = Subclass::GetEntryForIndexImpl( isolate, *object, object->elements(), index, filter); if (entry == kMaxUInt32) continue; PropertyDetails details = Subclass::GetDetailsImpl(*object, entry); if (details.kind() == kData) { value = Subclass::GetImpl(isolate, object->elements(), entry); } else { LookupIterator it(isolate, object, index, LookupIterator::OWN); ASSIGN_RETURN_ON_EXCEPTION_VALUE( isolate, value, Object::GetProperty(&it), Nothing<bool>()); <<------- (a) } if (get_entries) { value = MakeEntryPair(isolate, index, value); } values_or_entries->set(count++, *value); } *nof_items = count; return Just(true); } At (a), the elements kind can be changed by getters. This will lead to type confusion in GetEntryForIndexImpl. PoC: let arr = []; arr[1000] = 0x1234; arr.__defineGetter__(256, function () { delete arr[256]; arr.unshift(1.1); arr.length = 0; }); Object.entries(arr).toString(); 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.
,
Jan 3 2018
,
Jan 3 2018
,
Jan 3 2018
,
Jan 3 2018
Detailed report: https://clusterfuzz.com/testcase?key=6278563010707456 Job Type: linux_asan_chrome_mp Crash Type: UNKNOWN READ Crash Address: 0x7eb6b75272e0 Crash State: v8::internal::HashTable<v8::internal::NumberDictionary, v8::internal::NumberDict v8::internal::ElementsAccessorBase<v8::internal::DictionaryElementsAccessor, v8: v8::internal::FastGetOwnValuesOrEntries Sanitizer: address (ASAN) Recommended Security Severity: Medium Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=481254:481399 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6278563010707456 See https://github.com/google/clusterfuzz-tools for more information. The recommended severity is different from what was assigned to the bug. Please double check the accuracy of the assigned severity.
,
Jan 4 2018
,
Jan 8 2018
Reproduces nicely. Camillo, I think you are familiar with this code. Would you be willing to take a look?
,
Jan 8 2018
SGTM
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/be9c5fd982f53c937351eeca55ce668a101792b6 commit be9c5fd982f53c937351eeca55ce668a101792b6 Author: Camillo Bruni <cbruni@chromium.org> Date: Wed Jan 10 13:50:20 2018 [elements] Fix Object.entries/values with changing elements Drive-by-cleanup: - Add InternalElementsAccessor to expose protected instance methods to ElementsAccessor subclasses. - Make some more ElementsAccessor methods protected that take the raw entry as parameter. Bug: chromium:798644 Change-Id: Iffd00f1953461e8dd22c123e62298410fb6e049c Reviewed-on: https://chromium-review.googlesource.com/856816 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Commit-Position: refs/heads/master@{#50480} [modify] https://crrev.com/be9c5fd982f53c937351eeca55ce668a101792b6/src/elements.cc [modify] https://crrev.com/be9c5fd982f53c937351eeca55ce668a101792b6/src/elements.h [modify] https://crrev.com/be9c5fd982f53c937351eeca55ce668a101792b6/test/mjsunit/es8/object-entries.js [add] https://crrev.com/be9c5fd982f53c937351eeca55ce668a101792b6/test/mjsunit/regress/regress-crbug-798644.js
,
Jan 11 2018
ClusterFuzz has detected this issue as fixed in range 528379:528380. Detailed report: https://clusterfuzz.com/testcase?key=6278563010707456 Job Type: linux_asan_chrome_mp Crash Type: UNKNOWN READ Crash Address: 0x7eb6b75272e0 Crash State: v8::internal::HashTable<v8::internal::NumberDictionary, v8::internal::NumberDict v8::internal::ElementsAccessorBase<v8::internal::DictionaryElementsAccessor, v8: v8::internal::FastGetOwnValuesOrEntries Sanitizer: address (ASAN) Recommended Security Severity: Medium Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=481254:481399 Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=528379:528380 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6278563010707456 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Jan 11 2018
ClusterFuzz testcase 6278563010707456 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Jan 11 2018
,
Jan 22 2018
,
Feb 8 2018
,
Feb 9 2018
This bug requires manual review: M65 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), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 9 2018
[Bulk Edit] +awhalley@ (Security TPM) for M65 merge review
,
Feb 9 2018
govind@ - good for 65
,
Feb 9 2018
Approving merge to M65 branch 3325 based on comment #17. Please merge ASAP so we can pick it up for next week Beta release. Thank you.
,
Feb 12 2018
This CL has landed in M65.0.3318.0 already, not sure though if we want to backmerge to M64 if we'd do a respin?
,
Feb 12 2018
Since this has been present in M63, let's target this for M65. M64 is already rolling out. If you disagree, please let me know.
,
Mar 6 2018
,
Mar 6 2018
,
Mar 27 2018
,
Apr 19 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
,
Apr 25 2018
,
Nov 14
|
||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||
Comment 1 by ClusterFuzz
, Jan 3 2018