New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

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



Sign in to add a comment
link

Issue 880207: Security: incorrect type information on Math.expm1

Reported by sroett...@google.com, Sep 4 Project Member

Issue description

The typer sets the type of Math.expm1 to be Union(PlainNumber, NaN).
This is missing the -0 case: Math.expm1(-0) returns -0.

Here's a quick example that showcases the issue:
```
function foo() {
  return Object.is(Math.expm1(-0), -0);
}

console.log(foo());
%OptimizeFunctionOnNextCall(foo);
console.log(foo());
```
% d8 --allow-natives-syntax expm1-poc.js
true
false

The buggy code is in two locations:
https://cs.chromium.org/chromium/src/v8/src/compiler/typer.cc?rcl=9680338c622d4693f984b49fb24d101acd2d8112&l=1437

https://cs.chromium.org/chromium/src/v8/src/compiler/operation-typer.cc?rcl=9680338c622d4693f984b49fb24d101acd2d8112&l=420


== security impact ==

I'm filing this is a security bug since similar bugs have been exploitable in the past (e.g. crbug/762874). However, I'm not sure if this is the case here. Here's my analysis so far:
The interesting cases I found that can make a distinction between 0 and -0 are division, atan2 and Object.is. The typing code doesn't handle minus zero in the first two cases, which leaves Object.is.
Afaict, the typer runs 3 times:
* in the typer phase
* in the TypeNarrowingReducer (load elimination phase)
* in the simplified lowering phase
After the first two typing runs, the ConstantFoldingReducer will run, so if we get the typer to mark the Object.is result to always be false at this point it will simply be replaced with a false constant.
That leaves the third typing round.
The Object.is call can be represented in two forms at that point. As a ObjectIsMinusZero node if an earlier pass knew that we compare against -0 or as a SameValue node.
The ObjectIsMinusZero case doesn't seem to be interesting since type information are not propagated in the UpdateFeedbackType function.
The feedback type for SameValue is propagated though and will be used for (now buggy) range computations.
But there's one detail that keeps this from being exploitable: since SameValue requires a tagged type, a ChangeFloat64ToTagged node will get inserted. Since the type information say that the input can never be -0, it will not include special minus zero handling which replaces the -0 with the smi 0.
 

Comment 1 by tsepez@chromium.org, Sep 4

Labels: Security_Severity-Low Security_Impact-Stable M-70
Owner: hablich@chromium.org
Status: Assigned (was: Unconfirmed)
Ok, keeping as a low severity security bug out of an abundance of caution, though this seems like a functional issue rather than a security one.

Assigning to v8 triage.

Comment 2 by hablich@chromium.org, Sep 4

Cc: hablich@chromium.org
Components: Blink>JavaScript>Compiler
Labels: Pri-2
Owner: jarin@chromium.org
Jaro, WDYT?

Comment 3 by bmeu...@chromium.org, Sep 4

Cc: neis@chromium.org jarin@chromium.org
Owner: bmeu...@chromium.org
Status: Started (was: Assigned)
The typing rule is wrong, it looks like it was a copy&paste mistake on my side. I agree with the analysis that this is probably not really exploitable.

Comment 4 by bugdroid1@chromium.org, Sep 5

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/56f7dda67fdc9777719f71225494033f03aecc96

commit 56f7dda67fdc9777719f71225494033f03aecc96
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Wed Sep 05 16:07:16 2018

[turbofan] Fix incorrect typing rule for NumberExpm1.

The Math.expm1() function can actually return -0, for example in the
case that -0 is passed to it.

Bug:  chromium:880207 
Change-Id: If3a7a3a1fb6a18075ba0d7816687dfd831ebe293
Reviewed-on: https://chromium-review.googlesource.com/1205072
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55657}
[modify] https://crrev.com/56f7dda67fdc9777719f71225494033f03aecc96/src/compiler/operation-typer.cc
[add] https://crrev.com/56f7dda67fdc9777719f71225494033f03aecc96/test/mjsunit/regress/regress-crbug-880207.js

Comment 5 Deleted

Comment 6 by bmeu...@chromium.org, Sep 21

Status: Fixed (was: ixed)

Comment 7 by sheriffbot@chromium.org, Sep 21

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 8 by sroett...@google.com, Oct 14

I just noticed that the fix only changed the code in the operation typer but typer.cc will need an update as well:
https://cs.chromium.org/chromium/src/v8/src/compiler/typer.cc?rcl=e99cee81c58a61c4ecfee4033af99f78fa371145&l=1441

Comment 9 by awhalley@google.com, Oct 15

Labels: -M-70 M-71

Comment 10 by bugdroid1@chromium.org, Oct 15

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

commit c59c9c46b589deb2a41ba07cf87275921b8b2885
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Mon Oct 15 06:00:03 2018

[turbofan] Fix Math.expm1 builtin typing.

This fixes the typing for the case when the call is not lowered to
the simplified operator.

Bug:  chromium:880207 
Change-Id: Icecf12de77ece0fe9ffec2777874f5f0004a1e97
Reviewed-on: https://chromium-review.googlesource.com/c/1278642
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56621}
[modify] https://crrev.com/c59c9c46b589deb2a41ba07cf87275921b8b2885/src/compiler/typer.cc
[modify] https://crrev.com/c59c9c46b589deb2a41ba07cf87275921b8b2885/test/mjsunit/regress/regress-crbug-880207.js

Comment 11 by sroett...@google.com, Oct 31

This was exploitable after all. I was missing that expm1 can be a regular JSCall node which will return a tagged value instead of a float so there's no conversion happening that would drop the minus zero.
Chrome stable is currently vulnerable to this. Do you know when c59c9c46b589deb2a41ba07cf87275921b8b2885 will make it to stable and is there anything we can do to expedite this?

Comment 12 by hablich@chromium.org, Oct 31

Cc: awhalley@google.com
Labels: M-70
This is only on 72: https://chromiumdash.appspot.com/commit/c59c9c46b589deb2a41ba07cf87275921b8b2885

I suppose this means that the Security_severity needs to be updated.

Andrew/Jaro WDYT?

Comment 13 by awhalley@google.com, Oct 31

Labels: -Security_Severity-Low Merge-Request-71 Security_Severity-High OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Thanks for spotting! Upgrading severity and requesting merge to M71

Comment 14 by sheriffbot@chromium.org, Oct 31

Project Member
Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

Comment 15 by gov...@chromium.org, Oct 31

Labels: -Merge-Review-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #13. Please merge ASAP. Thank you.

Comment 16 by sheriffbot@chromium.org, Nov 1

Project Member
Labels: -Pri-2 Pri-1

Comment 17 by sroett...@google.com, Nov 1

A note on the disclosure of this bug. The issue was found as part of work on Project Zero and is thus subject to a 90 day disclosure deadline.
I'm counting October 31st (Yesterday) as the reporting date since this is when I notified you of the security impact of this bug.

Comment 18 by gov...@chromium.org, Nov 1

Pls merge your change to M71 branch 3578 ASAP so we can pick it up for next beta release. Thank you.

Comment 19 by bugdroid1@chromium.org, Nov 2

Project Member
Labels: merge-merged-7.1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/76df2c50d0e37ab0c42d0d05a637afe999fffc49

commit 76df2c50d0e37ab0c42d0d05a637afe999fffc49
Author: Benedikt Meurer <bmeurer@google.com>
Date: Fri Nov 02 19:16:25 2018

Merged: [turbofan] Fix Math.expm1 builtin typing.

Revision: c59c9c46b589deb2a41ba07cf87275921b8b2885

BUG= chromium:880207 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
TBR=jarin@chromium.org

Change-Id: I453d23ff1bfe355c792b39d7e69d48cc5414cd47
Reviewed-on: https://chromium-review.googlesource.com/c/1316047
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.1@{#32}
Cr-Branched-From: f70aaa8ab2e8815505a6145c745e50d8328cd28c-refs/heads/7.1.302@{#1}
Cr-Branched-From: 1dbcc78efa17a9047f7e923958087ef9eec43066-refs/heads/master@{#56462}
[modify] https://crrev.com/76df2c50d0e37ab0c42d0d05a637afe999fffc49/src/compiler/typer.cc
[modify] https://crrev.com/76df2c50d0e37ab0c42d0d05a637afe999fffc49/test/mjsunit/regress/regress-crbug-880207.js

Comment 20 by bmeu...@chromium.org, Nov 4

Labels: -Merge-Approved-71

Comment 21 by awhalley@google.com, Dec 3

Labels: Release-0-M71

Comment 22 by sheriffbot@chromium.org, Dec 28

Project Member
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