Security: incorrect type information on Math.expm1 |
|||||||||||||||
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.
,
Sep 4
Jaro, WDYT?
,
Sep 4
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.
,
Sep 5
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
,
Sep 21
,
Sep 21
,
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
,
Oct 15
,
Oct 15
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
,
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?
,
Oct 31
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?
,
Oct 31
Thanks for spotting! Upgrading severity and requesting merge to M71
,
Oct 31
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
,
Oct 31
Approving merge to M71 branch 3578 based on comment #13. Please merge ASAP. Thank you.
,
Nov 1
,
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.
,
Nov 1
Pls merge your change to M71 branch 3578 ASAP so we can pick it up for next beta release. Thank you.
,
Nov 2
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
,
Nov 4
,
Dec 3
,
Dec 28
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 tsepez@chromium.org
, Sep 4Owner: hablich@chromium.org
Status: Assigned (was: Unconfirmed)