Security: OOB read in TypedArray.from |
|||||||||||||||||||||||||
Issue descriptionThis is copied from the report in issue 835887: OOB read in TypedArray.from -------------------------------------------------------------------------------- https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-typed-array-gen.cc?rcl=8ba3ef5fcd21c5e717a1b7bd0713612a287b9500&l=1705 ES6 #sec-%typedarray%.from TF_BUILTIN(TypedArrayFrom, TypedArrayBuiltinsAssembler) { [...] TNode<JSArray> values = CAST( CallBuiltin(Builtins::kIterableToList, context, source, iterator_fn)); // This is not a spec'd limit, so it doesn't particularly matter when we // throw the range error for typed array length > MaxSmi. TNode<Object> raw_length = LoadJSArrayLength(values); GotoIfNot(TaggedIsSmi(raw_length), &if_length_not_smi); final_length = CAST(raw_length); final_source = values; Goto(&create_typed_array); BIND(&if_length_not_smi); ThrowRangeError(context, MessageTemplate::kInvalidTypedArrayLength, raw_length); } [...] BIND(&create_typed_array); { // 7c/11. Let targetObj be ? TypedArrayCreate(C, «len»). target_obj = CreateByLength(context, receiver, final_length.value(), "%TypedArray%.from"); Branch(mapping.value(), &slow_path, &fast_path); } BIND(&fast_path); { Label done(this); GotoIf(SmiEqual(final_length.value(), SmiConstant(0)), &done); CallRuntime(Runtime::kTypedArrayCopyElements, context, target_obj.value(), final_source.value(), final_length.value()); Goto(&done); BIND(&done); args.PopAndReturn(target_obj.value()); } https://cs.chromium.org/chromium/src/v8/src/elements.cc?rcl=8ba3ef5fcd21c5e717a1b7bd0713612a287b9500&l=3362 static bool TryCopyElementsFastNumber(Context* context, JSArray* source, JSTypedArray* destination, size_t length, uint32_t offset) { if (Kind == BIGINT64_ELEMENTS || Kind == BIGUINT64_ELEMENTS) return false; Isolate* isolate = source->GetIsolate(); DisallowHeapAllocation no_gc; DisallowJavascriptExecution no_js(isolate); ElementsKind kind = source->GetElementsKind(); BackingStore* dest = BackingStore::cast(destination->elements()); [...] if (kind == PACKED_SMI_ELEMENTS) { FixedArray* source_store = FixedArray::cast(source->elements()); for (uint32_t i = 0; i < length; i++) { Object* elem = source_store->get(i); DCHECK(elem->IsSmi()); int int_value = Smi::ToInt(elem); dest->set(offset + i, dest->from(int_value)); } return true; [...] -------------------------------------------------------------------------------- |TypedArrayFrom()| obtains the length of the |source| argument and then calls a user-defined function inside |CreateByLength()|, which could shrink |source|. If |source| is a fast array, this may cause an OOB read later in |TryCopyElementsFastNumber()| because it doesn't ensure |length| is still valid. REPRODUCTION CASE <script> oobArray = []; for (let i = 0; i < 1024 * 1024; ++i) { oobArray[i] = 1.1; } floatArray = new Float64Array(oobArray.length); Float64Array.from.call(function(length) { oobArray.length = 0; return floatArray; }, oobArray); </script>
,
Jun 19 2018
,
Jun 19 2018
,
Jun 19 2018
,
Jun 19 2018
Peter's looking into this, thanks! An optimization in IterableToList seems to be the culprit.
,
Jun 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2 commit 2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2 Author: Peter Marshall <petermarshall@chromium.org> Date: Tue Jun 19 13:01:08 2018 [typedarray] Fix incorrect optimization in IterableToList Bug: chromium:854066 Change-Id: Icabd9bf5e00868822b9debfb9bbb5d3932726465 Reviewed-on: https://chromium-review.googlesource.com/1105774 Commit-Queue: Peter Marshall <petermarshall@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#53840} [modify] https://crrev.com/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2/src/builtins/builtins-call-gen.cc [modify] https://crrev.com/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2/src/builtins/builtins-definitions.h [modify] https://crrev.com/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2/src/builtins/builtins-typed-array-gen.cc [modify] https://crrev.com/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2/src/builtins/builtins-typed-array-gen.h [add] https://crrev.com/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2/test/mjsunit/regress/regress-854066.js
,
Jun 19 2018
Yeah the fundamental problem is that IterableToList didn't return a copy, if the result of the iteration was exactly the same as the original array and the iteration protocol could be skipped. The spec assumes it is working with a copy, so modifications to the original are irrelevant. The above fix regresses some TypedArray microbenchmarks because it falls back to the full iteration protocol. We have some ideas about how to make the copy much faster, given that we can just directly copy the JSArray that would be returned by IterableToList, instead of doing the full iteration. We won't backmerge those though because they are only for performance. Requesting merge now, will wait until we have baking time on canary.
,
Jun 19 2018
,
Jun 19 2018
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 19 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
,
Jun 19 2018
+ awhalley@ for merge review. Pls not change is not yet baked in canary. M67 has been out since 05/29 and we're not planing M67 stable respin unless extremely critical issue arise. Thank you.
,
Jun 20 2018
,
Jun 20 2018
,
Jun 20 2018
Re-opening and removing merge request labels for now, since the original reporter indicates the issue is not fully resolved. Can you take another look? Thanks! ---- https://bugs.chromium.org/p/chromium/issues/detail?id=835887#c58 Unfortunately, the fix for issue 854066 is incomplete. It's possible to skip a call to |IterableToList| if the @@iterator property of the source array is null or undefined. Updated repro: <script> oobArray = []; delete oobArray.__proto__[Symbol.iterator]; for (let i = 0; i < 1024 * 1024; ++i) { oobArray[i] = 1.1; } floatArray = new Float64Array(oobArray.length); Float64Array.from.call(function(length) { oobArray.length = 0; return floatArray; }, oobArray); </script>
,
Jun 21 2018
Interesting.. On the iterator path (see the spec at [0]), we can avoid user modifications to the 'values' object since it is an unexposed copy. But the non-iterable path just takes the user-provided 'source' object itself as 'arrayLike', and many of the following steps can modify it (e.g. Get, ToLength, TypedArrayCreate, and Set within the loop). I see two possible solutions: 1. We KISS and strictly follow the spec by using slow Get/Set operations, which handle OOB reads; or 2. we recheck the actual arrayLike length after each possible modification. Perhaps we could add a bailout condition to TryCopyElementsFastNumber when the given length is OOB? [0] https://tc39.github.io/ecma262/#sec-%typedarray%.from
,
Jun 21 2018
(In any case, TryCopyElementsFastNumber should probably have a range DCHECK or even CHECK).
,
Jun 22 2018
I forgot to put this bug number in the CL, but here is the 2nd fix: https://chromium-review.googlesource.com/c/v8/v8/+/1108203 I'd suggest we backmerge both, possibly as one CL.
,
Jun 22 2018
,
Jun 22 2018
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 22 2018
We've got a 57 stable repsin going out on Monday, which this has missed. Not rejecting the merge, however, since it would be a good one to take in the (extremely unlikely) case of another respin.
,
Jun 25 2018
For reference here are both the changes: https://chromium.googlesource.com/v8/v8/+/bededee46ed289e652793e2ba09cd540196ac81d https://chromium.googlesource.com/v8/v8/+/2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2 Looks like were in canary over the weekend. Just waiting on the 68 merge review then
,
Jun 25 2018
Have the changes been verified in canary? Overall no other issues, and is this a safe merge?
,
Jun 25 2018
The canary looks fine re. these two fixes, nothing concerning there. The two fixes fall back to slow paths that have been in-place for a while already, so are pretty safe IMO.
,
Jun 25 2018
Ok great thanks for getting back. Approving merge to M68. Branch:3440
,
Jun 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/dd7862b6c0555dcbe150abce22070b856602d1ce commit dd7862b6c0555dcbe150abce22070b856602d1ce Author: Peter Marshall <petermarshall@chromium.org> Date: Tue Jun 26 08:53:35 2018 Merged: Squashed multiple commits. Merged: [typedarray] Fix incorrect optimization in IterableToList Revision: 2ebd3ed2fac2689da175cfc5db1d18ed6c5b59e2 Merged: [typedarray] Use slow case more aggressively in CopyElementsHandleImpl Revision: bededee46ed289e652793e2ba09cd540196ac81d TBR=jgruber@chromium.org BUG= chromium:854066 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=jgruber@chromium.org Change-Id: Ib8d28c4e3c03376007aa5d97f8c8d51a57caa157 Reviewed-on: https://chromium-review.googlesource.com/1113936 Reviewed-by: Peter Marshall <petermarshall@chromium.org> Commit-Queue: Peter Marshall <petermarshall@chromium.org> Cr-Commit-Position: refs/branch-heads/6.8@{#29} Cr-Branched-From: 44d7d7d6b1041b57644400a00cb3fee35f6c51b2-refs/heads/6.8.275@{#1} Cr-Branched-From: 5754f66f75136dc17b4c63fec84f31dfdb89186e-refs/heads/master@{#53286} [modify] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/src/builtins/builtins-call-gen.cc [modify] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/src/builtins/builtins-definitions.h [modify] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/src/builtins/builtins-typed-array-gen.cc [modify] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/src/builtins/builtins-typed-array-gen.h [modify] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/src/elements.cc [add] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/test/mjsunit/regress/regress-854066-2.js [add] https://crrev.com/dd7862b6c0555dcbe150abce22070b856602d1ce/test/mjsunit/regress/regress-854066.js
,
Jun 26 2018
,
Jun 29 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. 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
,
Jul 2
,
Jul 10
We're not planning any further M67 releases. Pls target fix for M68.
,
Jul 23
(reward-topanel as part of considering issue 835887)
,
Jul 23
,
Jul 30
,
Sep 28
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 creis@chromium.org
, Jun 19 2018