Project: chromium Issues People Development process History Sign in
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 0 users
Status: Fixed
Last visit 17 days ago
Closed: Oct 2016
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Sign in to add a comment
Security: Heap-use-after-free in InspectedContext::createInjectedScript
Project Member Reported by, Oct 19 2016 Back to list
Chrome version: 54.0.2840.59 (stable) and latest (56.0.2892.0)

Steps to reproduce (the order of steps does not matter):
1. Open attached file in Chrome.
2. Open the Devtools (F12, inspect element, whatever).

Expected: Nothing special should happen.
Actual: Tab crashes due to a use-after-free.

More info:
When console.log is called while the devtools is active (or when the devtools is opened after console.log has been called), InjectedScriptSource.js (aka injected-script-source.js since  bug 635948 ) is executed. This script runs in the context of the web page, so any script on the web can manipulate the JavaScript objects and change the state in unexpected ways. For example, scripts can destroy the context when the script is run. The C++ caller that injects this script (InspectedContext::createInjectedScript) does not account for this, leading to a UAF.
Comment 1 by, Oct 19 2016
161 bytes View Download
Comment 2 by, Oct 19 2016
Status: Started
Comment 3 by, Oct 19 2016
Another PoC: An extension that automates the crash.

Steps to reproduce:
1. Create a directory.
2. Download attached files, put it in the folder.
3. Visit chrome://extensions, enable developer mode.
4. Click on the Load unpacked extension button.
5. Load the extension -- and observe that is opened and crashes.

(alternatively I can upload the extension to the Chrome Web Store and victims can install it with a single click).
242 bytes View Download
697 bytes View Download
Comment 4 by, Oct 19 2016
+kozyatinskiy for review of the CL.

I've looked into adding a regression test to v8, but there are some things that stop me from doing that:

- The test infrastructure looks very recent ( ), including tests will probably cause test failures on release branches.

- The test scenario is a bit convoluted: The context is destroyed during execution. It seems that v8/inspector does not include logic to destroy the inspected context.

- I don't know how to build the inspector test target.
  I tried:
  $ tools/dev/ x64.release
  $ ninja -C
  After modifying the test files, running ninja again does not cause any new compilation (which is weird because I touched C++ test code).

Manually running the tests from this bug report shows that my CL fixed the bug though.
Thank you for great report.
Our long-term solution: don't call user code from injected-script-source.js. We'll achieve it by migrating this JS code into native.
Tests are not running on try bots now and we run new tests only on revisions after they were added and release branches should not be a problem.
We don't have contextDestoyed logic, but we can introduce it similar with setTimeout function in
I think that inspector is disabled by default. You need to enable it, I'm not sure that support passing additional arguments but you can run:
$ gn args
and add v8_enable_inspector = true manually and then rebuild with ninja.
Labels: Security_Severity-Medium Security_Impact-Stable
Comment 7 by, Oct 20 2016
Thanks for the tips. I managed to compile the inspector-test target and managed to run a test as follows:
./ test/inspector/protocol-test.js test/inspector/console/<filename>.js

If the ultimate goal is to not call user code from injected-script-source.js, then how can I create a test that destroys the context when that specific file is run? I have already added a "detach" global function to the utils extension in, but I don't know how to call this function at the right moment.

I'm still looking for a way to run arbitrary code in the inspected context (correct me if I'm wrong --- I should look for a way to run code in backend_runner->context_, right?).

Do you have any suggestions for either of these two?
Project Member Comment 8 by, Oct 20 2016
Labels: M-54
Project Member Comment 9 by, Oct 20 2016
Labels: Pri-1
I think that root of this issue - navigation happens during InjectedScriptSource initialization because user defined setter on Object.prototype:

Object.defineProperty(Object.prototype, "primitiveTypes", {
  configurable: true,
  set: function(v) {
    // detaching context.

You can put your detaching logic inside of this setter.

To evaluate code in inspected context you can just call:
Protocol.Runtime.evaluate({ expression: "code" }).then(.. process result ..).
Or add function at the begin with:
function foo() {
and then call this function with the same command:
Protocol.Runtime.evaluate({ expression: "foo()" });

Comment 11 by, Oct 28 2016
Status: Fixed
Although bugdroid did not visit the issue, the patch landed at
Verified fixed in Chromium 56.0.2904.0 (V8 5.6.163 according to chrome://version) ), using the automated steps to reproduce from comment 3 (=Chrome does not crash any more).

I'll request a merge once the patch has baked on Canary.
Project Member Comment 12 by, Oct 28 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Project Member Comment 14 by, Oct 30 2016
Labels: Merge-Request-55
Comment 15 by, Oct 31 2016
Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
**** Bulk edit -  please ignore if not applicable ****

Please merge your change to M55 branch 2883 today before 5:00 PM PT or latest by tomorrow, Tuesday (11/01/16) 4:00 PM PT so we can take it for this week Beta release. 
Components: Blink>JavaScript
Labels: merge-request-5.5
Thank you.
Since inspector is inside of V8. You need to request merge in V8 [1] instead of Chromium:
- add Merge-Request-5.5 label,
- add Blink>JavaScript component,
- add to cc.

Comment 18 by, Nov 1 2016
Labels: Merge-Review Hotlist-Merge-Review
[Automated comment] No milestone found on Merge-Request (i.e. merge-request-# label).
Comment 19 by, Nov 1 2016
I contacted hablich by mail before requesting a merge, and he said that a merge request in Chromium's issue tracker is also ok.
Components: -Blink>JavaScript
Labels: -merge-request-5.5
Ok, thank you!
If there is a merge needed for M55, please merge ASAP otherwise remove "Merge-Approved-55" label. Thank you.
Comment 23 by, Nov 2 2016
Do I need to do anything else to get the patch in M55, or will it roll automatically?

And should the patch be taken to M54? It is low-risk and fixes a UAF.
FYI: this code in M54 is located in third_party/WebKit/core/inspector, it was moved to V8 in M55.
Per comment #22, this is already merged to M55. If nothing is pending for M55, please remove "Merge-Approved-55" label. Thank you.
Comment 26 by, Nov 3 2016
Labels: -Merge-Approved-55 Merge-Request-54
Removing Merge-Approved-55 since merge-merged-5.5 is apparently sufficient.

Looks like this landed in 5.5.372.16, so it missed the most recent beta release.

Can we take the patch to stable?
Labels: -reward-topanel reward-unpaid reward-1500
Nice one, the panel has awarded $1,500 for this bug!
Labels: -reward-unpaid reward-inprocess
Labels: -Hotlist-Merge-Review -Hotlist-Merge-Approved M-55
Labels: Release-0-M55
Comment 32 by, Dec 1 2016
Labels: -Merge-Request-54
Labels: CVE-2016-5219
Project Member Comment 34 by, Feb 3 2017
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: -Merge-Review
Sign in to add a comment