New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

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



Sign in to add a comment

ThinStrings are incompatible with TurboFan SeqString types

Project Member Reported by bmeu...@chromium.org, Mar 15

Issue description

As found in b/74469145, we've determined that the SeqString's in TurboFan can actually change to ThinString at any time, so having SeqString doesn't really work.
 
Cc: -tebbi@chromium.org bmeu...@chromium.org
Labels: Security
Owner: tebbi@chromium.org
Cc: hablich@chromium.org mvstan...@chromium.org
Labels: -Type-Bug Type-Bug-Security
Labels: Security_Severity-Medium
This allows for out-of-bounds read in the renderer process.
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 15

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

commit c65f0a78c33452dc19b52934771ecb2c1ce3a0b8
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Thu Mar 15 17:52:12 2018

[turbofan] NumberToString can return non-sequential strings.

TurboFan assumed that the output of NumberToString is always a
sequential string, since that's what we put into the number to
string table. However we might eventually morph these strings
into ThinStrings when we need to internalize them, in which case
the type in TurboFan will be wrong, and we read out of bounds.

Also-By: tebbi@chromium.org
Bug:  chromium:822284 
Change-Id: I5aebe73028b95849fff72bba262c517677112353
Reviewed-on: https://chromium-review.googlesource.com/964523
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51970}
[modify] https://crrev.com/c65f0a78c33452dc19b52934771ecb2c1ce3a0b8/src/compiler/operation-typer.cc
[add] https://crrev.com/c65f0a78c33452dc19b52934771ecb2c1ce3a0b8/test/mjsunit/regress/regress-crbug-822284.js

Labels: Merge-Request-66
Warnings:
Merge requests should specify the affected OS(es) so that requests are routed appropriately
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 9 by sheriffbot@chromium.org, Mar 15

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 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), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

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

Comment 10 by bugdroid1@chromium.org, Mar 16

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

commit 36426ab7388f8bea2ea218b4238075d7e886d0fb
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Fri Mar 16 08:24:23 2018

[turbofan] Remove unsound SeqString types.

A value of type OtherSeqString can change its type to OtherNonSeqString
via inplace internalization (and redirection via a ThinString). This can
lead to out of bounds memory accesses and generally correctness bugs, as
seen with  crbug.com/822284 .

This change might affect performance in some cases, and we'll need to
evaluate whether it's worth spending cycles on adding another mechanism
that leverages the sequential string information in a safe way on a case
by case basis.

Bug:  chromium:822284 
Change-Id: I0de77ec089a774236555f38c365f7548f454edfe
Reviewed-on: https://chromium-review.googlesource.com/966021
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51975}
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/effect-control-linearizer.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/effect-control-linearizer.h
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/node-properties.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/opcodes.h
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/property-access-builder.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/redundancy-elimination.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/simplified-operator.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/simplified-operator.h
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/typed-optimization.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/typed-optimization.h
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/typer.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/types.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/types.h
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/src/compiler/verifier.cc
[modify] https://crrev.com/36426ab7388f8bea2ea218b4238075d7e886d0fb/test/cctest/test-types.cc

Status: Fixed (was: Assigned)
Project Member

Comment 12 by sheriffbot@chromium.org, Mar 16

Labels: Restrict-View-SecurityNotify
Please verify the fix in the latest canary
This seems like a fairly large change. How confident are we that this is safe?
It's a fairly straight-forward removal.
Cc: jkadams@google.com
jkadams@google.com verified the fix on canary.
Labels: -Merge-Review-66 Merge-Approved-66
approving merge for m66. Branch:3359
Project Member

Comment 19 by bugdroid1@chromium.org, Mar 26

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

commit f3ea486e11cfdb25b0917b4f2c25ed965d1e9fe6
Author: Tobias Tebbi <tebbi@chromium.org>
Date: Mon Mar 26 09:28:56 2018

Merged: [turbofan] NumberToString can return non-sequential strings.

Revision: c65f0a78c33452dc19b52934771ecb2c1ce3a0b8

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

Change-Id: If1198ab25ef20a8d952b3c31aad7be1886b25ecf
Reviewed-on: https://chromium-review.googlesource.com/979803
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.6@{#31}
Cr-Branched-From: d500271571b92cb18dcd7b15885b51e8f437d640-refs/heads/6.6.346@{#1}
Cr-Branched-From: 265ef0b635f8761df7c89eb4e8ec9c1a6ebee184-refs/heads/master@{#51624}
[modify] https://crrev.com/f3ea486e11cfdb25b0917b4f2c25ed965d1e9fe6/src/compiler/operation-typer.cc
[add] https://crrev.com/f3ea486e11cfdb25b0917b4f2c25ed965d1e9fe6/test/mjsunit/regress/regress-crbug-822284.js

Project Member

Comment 20 by sheriffbot@chromium.org, Mar 26

Cc: abdulsyed@google.com
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
Project Member

Comment 21 by sheriffbot@chromium.org, Mar 29

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
Labels: -Merge-Approved-66
Project Member

Comment 23 by sheriffbot@chromium.org, Jun 22

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