New issue
Advanced search Search tips
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

Security: V8: A bug in the ObjectDescriptor class

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

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.
 
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?
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).


Project Member

Comment 6 by ClusterFuzz, Jan 25 2018

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5124289511292928.
Cc: -ishell@chromium.org cbruni@chromium.org
Owner: ishell@chromium.org
Reassigning to ishell since cbruni is on short-notice leave.
Project Member

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

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

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

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
The PoC still works on Chrome 65.0.3325.181 (Latest Stable). Would you please check it?
Cc: hablich@chromium.org
Labels: Merge-Request-65
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.
The drop date of the issue is April 16th. Will the deadline grace extension be required?
Cc: awhalley@chromium.org
lokihardt@ - Yes, please.
Project Member

Comment 16 by sheriffbot@chromium.org, May 4

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