Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 657568 Security: Heap-use-after-free in InspectedContext::createInjectedScript
Starred by 0 users Project Member Reported by rob@robwu.nl, Oct 19 2016 Back to list
Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
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 rob@robwu.nl, Oct 19 2016
uaf-inspector-injected-script.html
161 bytes View Download
Comment 2 by rob@robwu.nl, Oct 19 2016
Owner: rob@robwu.nl
Status: Started
Patch: https://codereview.chromium.org/2432163004
Comment 3 by rob@robwu.nl, 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 example.com is opened and crashes.

(alternatively I can upload the extension to the Chrome Web Store and victims can install it with a single click).
manifest.json
242 bytes View Download
background.js
697 bytes View Download
Comment 4 by rob@robwu.nl, Oct 19 2016
Cc: kozyatin...@chromium.org
+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 ( https://crbug.com/635948 ), 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/v8gen.py x64.release
  $ ninja -C out.gn/x64.release
  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 inspector-test.cc.
I think that inspector is disabled by default. You need to enable it, I'm not sure that v8gen.py support passing additional arguments but you can run:
$ gn args out.gn/x64.release
and add v8_enable_inspector = true manually and then rebuild with ninja.
Labels: Security_Severity-Medium Security_Impact-Stable
Comment 7 by rob@robwu.nl, Oct 20 2016
Thanks for the tips. I managed to compile the inspector-test target and managed to run a test as follows:
./out.gn/x64.release/inspector-test 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 inspector-test.cc, 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 sheriffbot@chromium.org, Oct 20 2016
Labels: M-54
Project Member Comment 9 by sheriffbot@chromium.org, 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:
InspectorTest.addScript(`
function foo() {
}`);
and then call this function with the same command:
Protocol.Runtime.evaluate({ expression: "foo()" });

Comment 11 by rob@robwu.nl, Oct 28 2016
Status: Fixed
Although bugdroid did not visit the issue, the patch landed at https://chromium.googlesource.com/v8/v8.git/+/cb2a39d36762da3a800d9b5e734319d1141070b9
Verified fixed in Chromium 56.0.2904.0 (V8 5.6.163 according to chrome://version)https://chromium.googlesource.com/v8/v8.git/+/cb2a39d36762da3a800d9b5e734319d1141070b9 ), 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 sheriffbot@chromium.org, Oct 28 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Project Member Comment 14 by sheriffbot@chromium.org, Oct 30 2016
Labels: Merge-Request-55
Comment 15 by dimu@chromium.org, 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. 
Cc: hablich@chromium.org
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 hablich@chromium.org to cc.

[1] https://github.com/v8/v8/wiki/Merging-&-Patching
Comment 18 by dimu@chromium.org, 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 rob@robwu.nl, 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.
Cc: -hablich@chromium.org
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 rob@robwu.nl, Nov 2 2016
Cc: hablich@chromium.org
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 rob@robwu.nl, 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 rob@robwu.nl, Dec 1 2016
Labels: -Merge-Request-54
Labels: CVE-2016-5219
Project Member Comment 34 by sheriffbot@chromium.org, Feb 3
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
Labels: -Merge-Review
Sign in to add a comment