New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 10 users
Status: Fixed
Owner:
Last visit 15 days ago
Closed: Sep 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: V8 JIT escape analysis bug
Reported by jo...@microsoft.com, Sep 14 Back to list
Tested on:
	Chrome (Windows x64) stable & beta channel as of 9/14/2017 (V8 version 6.1.534.32)
	(bug also applies to other architectures and operating systems)

Small test case:
	var f = function()
	{
		var o = { a: {}, b: { ba: { baa: 0, bab: [] }, bb: {}, bc: { bca: {bcaa: 0, bcab: 0, bcac: this} } } };
		o.b.bc.bca.bcab = 0;
		o.b.bb.bba = Array.prototype.slice.apply(o.b.ba.bab);
	};
	while(true) f(f);

Observed behavior:
	In our tests, the test case resulted in one of multiple possible failures:
		- a crash upon dereferencing an invalid pointer
		- a call to V8_Fatal in MaybeHandle::Check
		- a "TypeError: Array.prototype.slice called on null or undefined" esception being thrown despite o.b.ba.bab always being an array

Causes:
	Meeting certain conditions can cause the escape analysis pass in the V8 JIT to optimize away the initialization of certain local variables in the final JITed code, which results in the behavior observed above. This can be turned into very powerful primitives by playing around with object o's internal layout as well as member variable types. These primitives include:
		- leaking the address of arbitrary objects
		- reading memory from arbitrary addresses
		- interpreting arbitrary attacker-controlled pointers as javascript objects
	With these primitives in hand, writing an exploit which achieves arbitrary code execution within the renderer process is trivial, partly thanks to the availability of RWX pages.
	A full, reliable remote code execution exploit for this bug is available upon request.

 
Project Member Comment 1 by ClusterFuzz, Sep 14
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5269842000347136.
Project Member Comment 2 by ClusterFuzz, Sep 14
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4797254937608192.
Cc: u...@chromium.org tebbi@chromium.org
Components: Blink>JavaScript>Compiler
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: adamk@chromium.org
Status: Assigned
Thank you for the report. We would certainly love to see the full exploit, so please feel free to attach it to this bug.

adamk: Can you please take a look? 
Status: Started
Looking now.
Cc: jarin@chromium.org adamk@chromium.org
Owner: tebbi@chromium.org
Status: Assigned
This doesn't reproduce at tip-of-tree (at least, not quickly, and when this test fails it fails very quickly from my experimentation).

From a bisect, it looks like it was fixed by the new escape analysis implementation, turned on in 40a9eabc44204c353dce4bec08e8bd87b3fc0cc7. ASsigning to tebbi@ for further analysis.
In a debug build, this consistently hits a DCHECK failure:

#
# Fatal error in ../../src/compiler/escape-analysis-reducer.cc, line 399
# Check failed: !escape_analysis_->IsVirtual(node).
#

#0  v8::base::OS::Abort () at ../../src/base/platform/platform-posix.cc:248
#1  0x00007f66fddce647 in V8_Fatal (file=0x7f66fd5f3a85 "../../src/compiler/escape-analysis-reducer.cc", line=399, 
    format=0x7f66fd5ce035 "Check failed: %s.") at ../../src/base/logging.cc:126
#2  0x00007f66fc77d2c5 in v8::internal::compiler::EscapeAnalysisReducer::VerifyReplacement (this=0x7ffd4bcd77c8)
    at ../../src/compiler/escape-analysis-reducer.cc:399
#3  0x00007f66fc9484e3 in v8::internal::compiler::EscapeAnalysisPhase::Run (this=0x7ffd4bcd7860, data=0x55ea1818f958, 
    temp_zone=0x55ea1817d9c0) at ../../src/compiler/pipeline.cc:1125
#4  0x00007f66fc93dc54 in v8::internal::compiler::PipelineImpl::Run<v8::internal::compiler::EscapeAnalysisPhase> (this=0x55ea1818fac0)
    at ../../src/compiler/pipeline.cc:849
#5  0x00007f66fc9362c6 in v8::internal::compiler::PipelineImpl::OptimizeGraph (this=0x55ea1818fac0, linkage=0x55ea1818fb88)
    at ../../src/compiler/pipeline.cc:1743
#6  0x00007f66fc93619c in v8::internal::compiler::PipelineCompilationJob::ExecuteJobImpl (this=0x55ea1818f7b0)
    at ../../src/compiler/pipeline.cc:684
#7  0x00007f66fc679e67 in v8::internal::CompilationJob::ExecuteJob (this=0x55ea1818f7b0) at ../../src/compiler.cc:134
#8  0x00007f66fc6814ae in v8::internal::(anonymous namespace)::GetOptimizedCodeNow (job=0x55ea1818f7b0) at ../../src/compiler.cc:707
#9  0x00007f66fc67c372 in v8::internal::(anonymous namespace)::GetOptimizedCode (function=..., 
    mode=v8::internal::ConcurrencyMode::kNotConcurrent, osr_ast_id=..., osr_frame=0x7ffd4bcd81d0) at ../../src/compiler.cc:897
#10 0x00007f66fc67fe0d in v8::internal::Compiler::GetOptimizedCodeForOSR (function=..., osr_ast_id=..., osr_frame=0x7ffd4bcd81d0)
    at ../../src/compiler.cc:1682
#11 0x00007f66fd0ac9f5 in v8::internal::__RT_impl_Runtime_CompileForOnStackReplacement (args=..., isolate=0x55ea1811f560)
    at ../../src/runtime/runtime-compiler.cc:324
#12 0x00007f66fd0ac2d0 in v8::internal::Runtime_CompileForOnStackReplacement (args_length=1, args_object=0x7ffd4bcd84c8, 
    isolate=0x55ea1811f560) at ../../src/runtime/runtime-compiler.cc:294

Labels: M-61
Owner: jarin@chromium.org
Looks like tebbi is OOO, assigning to jarin@ for handling in MUC.
Additional information from reporter - http://shortn/_AZDYahmQ1t
Project Member Comment 10 by ClusterFuzz, Sep 15
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=4816435858898944.
Project Member Comment 11 by ClusterFuzz, Sep 15
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5699773595385856.
Reuploaded using the actual debug job type, please remember to use linux_asan_d8_dbg for d8 debug job type.
Cc: bmeu...@chromium.org mvstan...@chromium.org
This is a bug in the old escape analysis that we have completely rewritten since then. The good news is that this is fixed by the new escape analysis, the bad news is that the old escape analysis is gone, so there is no way to fix bugs in it. The only reliable way seems to be disabling the escape analysis in 6.1.

The new escape analysis has been enabled by 

commit 40a9eabc44204c353dce4bec08e8bd87b3fc0cc7
Author: Tobias Tebbi <tebbi@chromium.org>
Date:   Tue Aug 1 10:10:24 2017 +0200

    Reland "[turbofan] enable new implementation of escape analysis"
    
    This is a reland of a6c3f143740b34622a89b3824391dc42b9ad2d22
    Original change's description:
    > [turbofan] enable new implementation of escape analysis
    >
    > Bug:
    > Change-Id: I0218ab67bf391deb8f1b1b78811643eb84745b7c
    > Reviewed-on: https://chromium-review.googlesource.com/595508
    > Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    > Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#47032}
    
    Change-Id: Ide3d11f4b25eae2bbcaca9fc3cdb983d73ba846c
    Reviewed-on: https://chromium-review.googlesource.com/599827
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#47121}
Cc: danno@chromium.org
Thanks for the look, Jaro...
Cc: ofrobots@google.com
Adding ofrobots to judge the impact on node.js
AFAICT, security impact to Node.js should be low as this isn't remotely exploitable for Node.
Cc: mathias@chromium.org
Labels: ReleaseBlock-Stable Pri-0
Background:
In 6.1 we have an old, removed implementation of Escape Analysis active. We replaced this implementation with a new one starting 6.2. Repairing the old one in 6.1 is not feasible especially as we can't test it on Canary anymore.

The current plan:
1.) Switch off (the new) Escape Analysis of on master to test alternative code path without Escape Analysis
2.) Analyse impact on Monday and switch off Escape Analysis in 6.1 too
3.) Switch Escape Analysis back on on master


Cc: mstarzinger@chromium.org
Project Member Comment 21 by bugdroid1@chromium.org, Sep 15
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/2b15425b0c131869dbd5c9c486d402b6b911fca4

commit 2b15425b0c131869dbd5c9c486d402b6b911fca4
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Fri Sep 15 10:59:26 2017

[turbofan] Temporarily turn off escape analysis.

Bug:  chromium:765433 
Change-Id: Iecc9540f6305bc24a0a5210c149b55403b9ce09d
Reviewed-on: https://chromium-review.googlesource.com/667106
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48032}
[modify] https://crrev.com/2b15425b0c131869dbd5c9c486d402b6b911fca4/src/flag-definitions.h

A bit more background:

The problem here is too aggressive pruning of escape analysis replacement reductions. Here is an example that roughly illustrates the problem:

o0 = { f : 42 }
o1 = { g : o0 }
a = load(o1, "g")
return load(a, "f")


After we analyze the code for escapes, we try to replace the loads with the contents of the variable that represents the field. 

If we first visit load 'b', we do not do anything (because it does not load from a non-escaping allocation, but from a load), and mark it as visited (EscapeAnalysisReducer::fully_reduced_). 

Later, when we replace load a by o0, the load b will be rewritten to load(o0, "f"). Since load b was marked as visited, we will not reduce it further to 42, but instead keep it as a load. This means that we will load from object o0, which was already marked as non-escaping, and the code for the allocation and initialization was removed from the effect chain. As a result, we will load from uninitialized memory.


Cc: awhalley@chromium.org
+awhalley@ (Security TPM)
Cc: hablich@chromium.org
+ hablich@ (V8 TPM)
Cc: amineer@chromium.org
Labels: -M-61 M-62
Tracking this for M62, though a candidate for a 61 merge if there's a respin after it's had bake time.
Labels: Security_Impact-Stable Security_Severity-High
Labels: -M-62 M-61
This does not affect M62 (see #19 for the summary).
Labels: -ReleaseBlock-Stable
ah, sorry for not spotted, thanks adamk@ Removing ReleaseBlock-Stable
Labels: -Security_Severity-High Security_Severity-Critical
Upgrading to Severity-Critical (comment #9 contains the full chain)
Labels: -Security_Severity-Critical Security_Severity-High
The v8 bug is High as execution is contained within the renderer sandbox. the PoC demonstrates what can be done with arbitrary code execution from renderer but does not demonstrate any new sandbox escape.
Project Member Comment 32 by sheriffbot@chromium.org, Sep 16
Labels: -Pri-0 Pri-1
Labels: Merge-Request-61
We should merge the "fix" to 6.1 - Canary experiment with escape analysis off looks good, turning off escape analysis is low risk. (We have turned off escape analysis several times before without any problems.)

There are some performance implications, mostly affecting benchmarks and some ES6 heavy code.
Labels: -Merge-Request-61 Merge-Approved-61 OS-iOS
Project Member Comment 36 by bugdroid1@chromium.org, Sep 18
Labels: merge-merged-6.1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a503d626788aa142551180ab802c488a5a3e9227

commit a503d626788aa142551180ab802c488a5a3e9227
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Mon Sep 18 10:58:06 2017

Merged: [turbofan] Temporarily turn off escape analysis.

Revision: 2b15425b0c131869dbd5c9c486d402b6b911fca4

BUG= chromium:765433 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=hablich@chromium.org

Change-Id: Iae7a44d2441f51d51423fba9b9c46367937cdad7
Reviewed-on: https://chromium-review.googlesource.com/670563
Reviewed-by: Michael Hablich <hablich@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.1@{#80}
Cr-Branched-From: 1bf2e10ddb194d4c2871a87a4732613419de892d-refs/heads/6.1.534@{#1}
Cr-Branched-From: e825c4318eb2065ffdf9044aa6a5278635c36427-refs/heads/master@{#46746}
[modify] https://crrev.com/a503d626788aa142551180ab802c488a5a3e9227/src/flag-definitions.h

Labels: -Merge-Approved-61
Project Member Comment 38 by bugdroid1@chromium.org, Sep 18
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d6ff08223bab0a0ca8b5c8413b13f87257cf1a58

commit d6ff08223bab0a0ca8b5c8413b13f87257cf1a58
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Mon Sep 18 12:16:36 2017

Revert "[turbofan] Temporarily turn off escape analysis."

This reverts commit 2b15425b0c131869dbd5c9c486d402b6b911fca4.

Reason for revert: Re-enabling escape analysis after merging the flag change to 6.1.

Original change's description:
> [turbofan] Temporarily turn off escape analysis.
> 
> Bug:  chromium:765433 
> Change-Id: Iecc9540f6305bc24a0a5210c149b55403b9ce09d
> Reviewed-on: https://chromium-review.googlesource.com/667106
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#48032}

TBR=mstarzinger@chromium.org,jarin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:765433 
Change-Id: Icac44fd76e2965df1e143700941b628ea7a69166
Reviewed-on: https://chromium-review.googlesource.com/670864
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48064}
[modify] https://crrev.com/d6ff08223bab0a0ca8b5c8413b13f87257cf1a58/src/flag-definitions.h

Project Member Comment 39 by sheriffbot@chromium.org, Sep 18
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Thanks for the merges. We won't be re-spinning M61 for this, but it will be picked up if we respin for another reason.
Note: If Node 8 is going for 6.1, we should definitely turn on escape analysis there.
Labels: reward-topanel
#41 What do you mean, exactly? Do you have a fix for the bug? Even if you have, how do you test the fix?
Project Member Comment 44 by sheriffbot@chromium.org, Sep 19
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Release-2-M61
Labels: -reward-topanel reward-unpaid reward-7500
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Cc: mborins@google.com
Labels: CVE-2017-5121
Labels: -reward-unpaid reward-decline
Project Member Comment 50 by sheriffbot@chromium.org, Dec 25
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
Issue 808483 has been merged into this issue.
Sign in to add a comment