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 3 users
Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Chrome, Mac
Pri: 2
Type: Bug-Security



Sign in to add a comment
expose() leaks privateClass via Object[@@hasInstance]
Reported by jannhorn@googlemail.com, Oct 29 2016 Back to list
UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36

Steps to reproduce the problem:
Steps:
1. ensure that there are no extensions installed that inject content scripts into google.com
2. unpack and install test-victim-extension.zip
3. open hijack.html in a new tab
4. click the link in hijack.html
5. wait a bit

What is the expected behavior?

What went wrong?
You should see a notification popup, created by the background script of the test extension in response to a message it received over the port that was opened by the content script.

The issue is the following line in the implementation of utils.expose():

    DCHECK(!(privateClass.prototype instanceof $Object.self));

`privateClass.prototype` is an object that unprivileged code should not be allowed to access, `$Object.self` is the global `Object`. This is buggy because, since ECMAScript 6, the `instanceof` operator performs the following steps (see <http://www.ecma-international.org/ecma-262/7.0/index.html#sec-instanceofoperator>):

1. If Type(C) is not Object, throw a TypeError exception.
2. Let instOfHandler be ? GetMethod(C, @@hasInstance).
3. If instOfHandler is not undefined, then Return ToBoolean(? Call(instOfHandler, C, « O »)).
[...]

In other words, `Object[Symbol.hasInstance](privateClass.prototype)` will be invoked if `Object[Symbol.hasInstance]` isn't `undefined`, meaning that by setting this property, `privateClass.prototype` can be leaked:

    // leak PortImpl
    Object.defineProperty(Object, Symbol.hasInstance, {value: function(obj){
      if (obj.constructor.name === 'PortImpl') {
        window.PortImpl = obj.constructor;
      }
    }});
    chrome.runtime; /* trigger lazy load of extensions::messaging */

Did this work before? N/A 

Chrome version: 54.0.2840.71  Channel: stable
OS Version: 
Flash Version: 

This is a slightly different vector with more or less the same impact as  issue 609569 .

Introduced in <https://chromium.googlesource.com/chromium/src/+/931719c51866a7c2e272114517cc52aa6581adff>.

It might make sense to assign (or CC) rob@robwu.nl on this bug.

From a quick look, I can't find any way to actually abuse this on the dev channel - port IDs are now translated (and implicitly checked) in the renderer. However, in stable, it still works.
 
Comment 1 by ta...@google.com, Oct 31 2016
Cc: devlin@chromium.org
Labels: Security_Severity-Medium Security_Impact-Stable
Owner: rob@robwu.nl
rob@ and devlin@, I wonder if you could take a look at this. Thanks.
Comment 2 by ta...@google.com, Oct 31 2016
Components: Platform>Extensions
Comment 3 by ta...@google.com, Oct 31 2016
Status: Assigned
Comment 4 by rob@robwu.nl, Oct 31 2016
Heh. Initially I did set the prototype of the private constructors to null, but changed it after review. After all hardening in the extension system a few months ago, I don't think that there is any exploitable security issue here, is it? Are you able to use this weakness to do anything unusual?

We could remove the DCHECK, null all constructors or do nothing.
Given that the JS bindings will disappear (bug 653596) and there is no exploitable condition here, I think that the first or third option is the best. At this point it does not make sense to invest much more time in the JS bindings since they're gone soon anyway.

Devlin, WDYT?
> Are you able to use this weakness to do anything unusual?

Not in the dev channel, as far as I can tell - only in the stable channel, to hijack ports of extension scripts.

> Given that the JS bindings will disappear (bug 653596)

Woohoo, awesome! :)

> and there is no exploitable condition here

Well, on the stable channel, there is.

> , I think that the first or third option is the best

I agree, removing the DCHECK for now is probably fine.
Project Member Comment 6 by sheriffbot@chromium.org, Nov 1 2016
Labels: M-55
Project Member Comment 7 by sheriffbot@chromium.org, Nov 1 2016
Labels: -Pri-2 Pri-1
Comment 8 by rob@robwu.nl, Nov 2 2016
Labels: -Pri-1 Pri-2
Status: Started
The DCHECK is not useful on release channels, and the usefulness for development are limited, so I'll just remove the DCHECK. It will be safe to merge since it was supposed to be no-op on release channels anyway.

https://codereview.chromium.org/2474623002/
@4, sorry for the delay.  Removing the DCHECK here sounds fine given the limited usefulness and lack of exploitable attack here.
Project Member Comment 10 by bugdroid1@chromium.org, Nov 3 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d9edd7b2524090fb0a0461e7bd9e5caa1e8a36d

commit 0d9edd7b2524090fb0a0461e7bd9e5caa1e8a36d
Author: rob <rob@robwu.nl>
Date: Thu Nov 03 00:40:16 2016

[extension bindings] Remove leak of internal class

BUG= 660678 

Review-Url: https://codereview.chromium.org/2474623002
Cr-Commit-Position: refs/heads/master@{#429482}

[modify] https://crrev.com/0d9edd7b2524090fb0a0461e7bd9e5caa1e8a36d/extensions/renderer/resources/utils.js

Comment 11 by rob@robwu.nl, Nov 3 2016
Labels: Merge-Request-54 Merge-Request-55 OS-Chrome OS-Mac OS-Windows
Status: Fixed
Requesting merge of the above patch to M55 and M54. The patch is trivial (removing a debug assertion that is not enabled in release builds), so there is no risk of breaking anything.
Project Member Comment 12 by sheriffbot@chromium.org, Nov 3 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: awhalley@chromium.org anan...@chromium.org ligim...@chromium.org bustamante@chromium.org
+ awhalley@ for merge review
I agree the patch is trivial, but there's time to follow bake times for M55, so will give it a couple of days.  There are no plans to spin M54 at this point and we wouldn't spin just for this change, but keep the label on so we can consider it if we do.
Labels: ReleaseBlock-Stable
For M54, we are planning a respin to ship a critical fix for Enterprise support, https://bugs.chromium.org/p/chromium/issues/detail?id=658606.

Richard could you please comment on the respin plans.
Comment 16 by dimu@chromium.org, Nov 3 2016
Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Comment 17 by dimu@chromium.org, Nov 3 2016
Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M54), manual review required.
Comment 18 by dimu@chromium.org, Nov 4 2016
Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M54), manual review required.
Labels: reward-topanel
Project Member Comment 20 by sheriffbot@chromium.org, Nov 7 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 21 by bugdroid1@chromium.org, Nov 7 2016
Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/201becf497377723fb9ec92bb8dcc0cf4f164ca1

commit 201becf497377723fb9ec92bb8dcc0cf4f164ca1
Author: Rob Wu <rob@robwu.nl>
Date: Mon Nov 07 15:08:03 2016

[extension bindings] Remove leak of internal class

BUG= 660678 

Review-Url: https://codereview.chromium.org/2474623002
Cr-Commit-Position: refs/heads/master@{#429482}
(cherry picked from commit 0d9edd7b2524090fb0a0461e7bd9e5caa1e8a36d)

Review URL: https://codereview.chromium.org/2476303002 .

Cr-Commit-Position: refs/branch-heads/2883@{#474}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/201becf497377723fb9ec92bb8dcc0cf4f164ca1/extensions/renderer/resources/utils.js

Labels: -Merge-Review-54 Merge-Approved-54
Approved for merging into M54 (build 2840), thanks for the fix!
Project Member Comment 23 by bugdroid1@chromium.org, Nov 8 2016
Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15c15fc6d0703dcf6eef798b409164db183c05a8

commit 15c15fc6d0703dcf6eef798b409164db183c05a8
Author: Andrew R. Whalley <awhalley@chromium.org>
Date: Tue Nov 08 00:06:42 2016

[M54 merge][extension bindings] Remove leak of internal class

BUG= 660678 

Review-Url: https://codereview.chromium.org/2474623002
Cr-Commit-Position: refs/heads/master@{#429482}
(cherry picked from commit 0d9edd7b2524090fb0a0461e7bd9e5caa1e8a36d)

Review URL: https://codereview.chromium.org/2487543002 .

Cr-Commit-Position: refs/branch-heads/2840@{#828}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/15c15fc6d0703dcf6eef798b409164db183c05a8/extensions/renderer/resources/utils.js

Labels: CVE-2016-5201 Release-3-M54
Labels: -Hotlist-Merge-Approved -ReleaseBlock-Stable
Labels: -reward-topanel reward-unpaid reward-1000
Thanks for this - $1,000 reward!
Please donate the reward to UNICEF.
Labels: -reward-unpaid reward-decline
$2,000 has been donated via unicefusa.org - many thanks!
Project Member Comment 30 by sheriffbot@chromium.org, Feb 9 2017
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