New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 836858 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security

Blocking:
issue 835887



Sign in to add a comment

Security: Privilege Escalation using extension filesystem URLs

Project Member Reported by creis@chromium.org, Apr 25

Issue description

This is copied from the report in issue 835887:

----

2. The fix[1] stopped the "noopener" trick from working for filesystem:chrome-extension: URLs, but there is another way
to bypass the web_accessible_resource restriction. The URL check in ExtensionNavigationThrottle[2] relies on the output
of |GetLastCommittedURL()|. A compromised renderer can commit any chrome-extension: URL through |RenderFrameHostImpl::
DidCommitSameDocumentNavigation()|. There is also an explicit check for filesystem:chrome-extension: URLs, but it
doesn't apply to child frames.

1 - https://chromium.googlesource.com/chromium/src/+/1cb9c33c4afd6df591c6306511bac80ec216d463
2 - https://cs.chromium.org/chromium/src/extensions/browser/extension_navigation_throttle.cc?rcl=7e651fba081b5a1507f36a844cb33c09ed66d2f5&l=35

----

This report bypasses work done in issue 820838,  issue 821138 , and  issue 821596 .
 
Blocking: 835887
The general flow of this part of the attack is that a web page in a compromised process does a window.open to about:blank, but it lies about the origin and claims it is an extension.  It then does a replaceState to change the URL to be an extension URL as well, which allows the attack to embed an extension filesystem URL as a subframe.  That presumably loads as an OOPIF in an extension process, with privileges.

For the first step (about:blank with extension origin), RenderFrameHostImpl::CanCommitOrigin does look at this origin, but our current checks there don't catch that it's an extension origin in a web process.  At the time those checks were written, --isolate-extensions hadn't launched yet and it was possible to have extension URLs/origins commit in web processes.  We should make sure that the CanCommitOrigin and CanCommitURL functions in RFHI catch any attempts to commit extensions (and WebUI) in the wrong process, and kill the process instead.

I have a CL in progress at https://chromium-review.googlesource.com/c/chromium/src/+/1025075.  It's currently facing a problem when error pages commit in the wrong process, in which case we would kill a web process that tries to show an error page for a blocked extension URL.  Nasko happens to be fixing that in https://chromium-review.googlesource.com/c/chromium/src/+/762520, but even then we'll have to make an exception for the error page process and possibly for subframes.

For now, I'll probably make an exception for error pages in CanCommitURL, ensuring that the origin is unique.  That will require us to check for error pages when doing the subframe filesystem URL check for extensions-- normally we allow extension filesystem URLs to be embedded as subframes of extension pages, but we won't allow that for error pages.
Cc: lukasza@chromium.org
I'm still working on the CanCommitURL CL, but it's proving to be a bit tricky given all the exceptions it needs (e.g., for GuestViews in V2 apps, for signin, for error pages).

In parallel, I put together a CL that applies the extension process check in ExtensionNavigationThrottle to all frames instead of just to main frames (as alexmos@ suggested):
https://chromium-review.googlesource.com/c/chromium/src/+/1028511
It seems to be working so far, so I'm writing a test for it and I think we can probably land that first.

That would disrupt the chain by preventing the exploit from loading a filesystem: or blob: URL from a subframe that isn't an extension process, even if the main frame incorrectly has an extension origin.

We'll still proceed with the CanCommitURL check so that the main frame won't have the incorrect extension origin, but we'll at least have something in place.
Project Member

Comment 5 by sheriffbot@chromium.org, Apr 26

Labels: M-66
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 26

Labels: -Pri-0 Pri-1
r553867 should be in tomorrow's Canary (most likely 68.0.3410.0).  Once that has a chance to bake, we can request merge to M67 and M66.

I'll keep working on the CanCommitURL check.  That one may be less urgent to merge to M66 if we have the chain disrupted in at least one place in r553867.
Cc: awhalley@chromium.org gov...@chromium.org abdulsyed@chromium.org
Components: Blink>Storage>FileSystem
Labels: Merge-Request-67 M-67
govind@ / awhalley@: Requesting merge to M67.  r553867 should disrupt one of the privilege escalation steps in issue 835887.  It has been baking on Canary since 68.0.3410.0, and there don't appear to be new crashes from it, or new bugs about filesystem: or blob: URLs.  We think it should be a safe merge and important for disrupting the sandbox escape.

abdulsyed@ / awhalley@: I could request merge to M66 as well.  Should we aim for M66?  If so, should I let this bake in M67 beta this week before I request the merge?
Project Member

Comment 9 by sheriffbot@chromium.org, Apr 30

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
govind@ - good for 67
creis@ - yep, let's get this out in beta first.

Thanks!
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #10. Please merge ASAP. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 30

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb

commit e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb
Author: Charlie Reis <creis@chromium.org>
Date: Mon Apr 30 18:09:26 2018

Merge to M67: Apply ExtensionNavigationThrottle filesystem/blob checks to all frames.

BUG= 836858 

Change-Id: I34333a72501129fd40b5a9aa6378c9f35f1e7fc2
Reviewed-on: https://chromium-review.googlesource.com/1028511
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553867}(cherry picked from commit 7614790c80996d32a28218f4d1605b0908e9ddf6)
Reviewed-on: https://chromium-review.googlesource.com/1035369
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#382}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/content/public/test/browser_test_utils.h
[modify] https://crrev.com/e2dc6347d3b22d9c2e51fa8ec9752ed4805a65eb/extensions/browser/extension_navigation_throttle.cc

Project Member

Comment 13 by sheriffbot@chromium.org, May 1

Status: Fixed (was: Started)
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
Project Member

Comment 14 by sheriffbot@chromium.org, May 2

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-66
Requesting to merge r553867 to M66, now that it has baked on M67 beta since May 2.  This disrupts the privilege escalation used in issue 835887.
Labels: -Merge-Request-66 Merge-Approved-66
Approving merge for M66. Branch:3359

Project Member

Comment 17 by bugdroid1@chromium.org, May 7

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66dd2ea1ea9eba0354a0a66718cc0755965c85f4

commit 66dd2ea1ea9eba0354a0a66718cc0755965c85f4
Author: Charlie Reis <creis@chromium.org>
Date: Mon May 07 22:28:33 2018

Merge to M66: Apply ExtensionNavigationThrottle filesystem/blob checks to all frames.

BUG= 836858 

Change-Id: I34333a72501129fd40b5a9aa6378c9f35f1e7fc2
Reviewed-on: https://chromium-review.googlesource.com/1028511
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#553867}(cherry picked from commit 7614790c80996d32a28218f4d1605b0908e9ddf6)
Reviewed-on: https://chromium-review.googlesource.com/1048645
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#807}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/content/public/test/browser_test_utils.h
[modify] https://crrev.com/66dd2ea1ea9eba0354a0a66718cc0755965c85f4/extensions/browser/extension_navigation_throttle.cc

The filesystem URL fix has been merged to M66 and M67, and I've filed  issue 840857  to track the CanCommitURL fix.  We can consider this one finished.
Labels: Release-2-M66
Labels: CVE-2018-6121
Labels: reward-topanel
(reward-topanel as part of considering issue 835887)
Labels: -reward-topanel
Project Member

Comment 23 by sheriffbot@chromium.org, Aug 8

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