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

Issue 645028 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 644426



Sign in to add a comment

Web accessible resources checks should work with blob: and filesystem: URLs that have chrome-extension:// inner URLs

Project Member Reported by alex...@chromium.org, Sep 8 2016

Issue description

Currently, trying to load a URL with an inner chrome-extension:// URL, such as blob:chrome-extension://foo/bar, skips the web accessible resources checks.  This is because in the renderer, ChromeExtensionsRendererClient::WillSendRequest checks the outer url for the extensions scheme:

  if (url.SchemeIs(extensions::kExtensionScheme) &&
      !resource_request_policy_->CanRequestResource(url, frame,
                                                    transition_type)) {
    *new_url = GURL(chrome::kExtensionInvalidRequestURL);
    return true;
  }

The scheme check here (and the one below it) won't work properly for blob: and filesystem: URLs, and so it will skip CanRequestResource(), which is what checks the web accessible resources.

Presumably, there's similar validation on the browser side -- I'm guessing it's  ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL, which looks like it will similarly fail with blob or filesystem URLs:

  if (to_url.SchemeIs(kExtensionScheme) &&
      (from_url.SchemeIsHTTPOrHTTPS() || from_url.SchemeIs(kExtensionScheme))) {
    ...
    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(
            extension, to_url.path())) {
      *result = false;
      return true;
    }
  }

It seems these should be fixed to handle blob: and filesystem:, since I think those are considered to have the origin of their inner URLs.  Devlin, would you be able to look at this further?  Charlie had some suggestions in 2) of https://crbug.com/644426#4.
 

Comment 1 by creis@chromium.org, Sep 8 2016

Cc: nasko@chromium.org
Nasko, you mentioned that web accessible resources might not block all types top-level navigations (or at least, that the browser process might not enforce that)?  Is that right?

If so, we should still do this, but we'll probably also want to block all top-level navigations to blob/filesystem URLs (with extension origins) in the browser process (per comment 17 of issue 644426).  Maybe we can track that in a separate bug.

Comment 2 by nasko@chromium.org, Sep 8 2016

The browser side checks are performed by ExtensionNavigationThrottle::WillStartRequest and url_request_util::AllowCrossRendererResourceLoad. The latter one does not have the bits to know whether the navigation was renderer or browser initiated, so it allows all main frame navigations to chrome-extenion:// resources. However, I think we can add such check to ExtensionNavigationThrottle::WillStartRequest.

The difference between the two is that url_request_util::AllowCrossRendererResourceLoad runs on the IO thread and gates whether we even create a request to begin with. It is the earliest time we can cancel requests we don't deem valid. ExtensionNavigationThrottle::WillStartRequest runs on the UI thread later on, once the resource request is created and we are about to send it to the network stack. It runs later, but being on the UI thread allows us to do more things.

Both methods are oblivious to blob/filesystem URLs and should be updated.

I'm happy to help with creating the fix, but don't have cycles right now to write the tests and herd a CL through the approval process/CQ. If someone can help driving the work, I will gladly contribute the actual fix :).
I'll try to look into this today or early next week.  If I get stuck, I'll bug Nasko. :)

Comment 4 by creis@chromium.org, Sep 9 2016

Labels: M-54 M-53
Great.  It sounds like this will be sufficient to block navigations to attacker-created blob/filesystem URLs in the browser process, so we won't need to track a separate bug (as considered in comment 1).

We'll need this fix merged to M54 and M53.  It should help in both --isolate-extensions and the old default mode.
Cc: -alex...@chromium.org rdevlin....@chromium.org
Owner: alex...@chromium.org
Status: Started (was: Assigned)
I've got a patch for this that will be landing soon (https://codereview.chromium.org/2345473003).  After lots of discussions, the approach we settled on for M53 is to use the ChromeExtensionsNetworkDelegate to block top-level navigations to blob/filesystem URLs with extension origins from non-extension processes on the IO thread.

For the record, here's a quick summary of some discussions around this.  ExtensionNavigationThrottle::WillStartRequest didn't work out because it has no idea who made the request, and it'd be difficult to plumb that information through in a way that's mergeable to M53.  Having information about the initiator on NavigationHandle is something we do want to support longer-term though.

url_request_util::AllowCrossRendererResourceLoad also didn't work out, because it's only invoked by the chrome-extension:// protocol handler, but the requests we want to block have blob: or filesystem: schemes, which instead go through blob: and filesystem: protocol handlers (e.g., the blob one is in content/browser/storage_partition_impl_map, which calls into storage/browser/blob/blob_url_request_job_factory), which don't know/don't care about extensions or what the origin of those URLs is.

Another proposal was to introduce a new throttle on the IO thread to do the blocking, but we thought the  ChromeExtensionsNetworkDelegate approach would be easier/shorter to implement and merge.  On the down side, this adds yet another place that does extensions security checks, so longer-term we should try to consolidate them into fewer places.

It might be worth cleaning up the WebAccessibleResources checks mentioned in the original description to also handle blob:/filesystem: schemes properly for consistency (ChromeExtensionsRendererClient::WillSendRequest and ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL), since as we've discussed, both blob: and filesystem: URL should never be considered web-accessible.  But the check I'm adding into ChromeExtensionsNetworkDelegate is the most complete. (ShouldAllowOpenURL is used for some but not all navigations, namely those that go through the OpenURL path, and Nasko mentioned the WAR checks there might be redundant now with those in AllowCrossRendererResourceLoad.)

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bfdc9292a6161980ba9a7a469d2d4515bebc6dd

commit 4bfdc9292a6161980ba9a7a469d2d4515bebc6dd
Author: alexmos <alexmos@chromium.org>
Date: Thu Sep 15 22:40:54 2016

Block top-level navigations to nested URLs with extension origins from non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG= 645028 

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

[modify] https://crrev.com/4bfdc9292a6161980ba9a7a469d2d4515bebc6dd/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4bfdc9292a6161980ba9a7a469d2d4515bebc6dd/chrome/browser/net/chrome_extensions_network_delegate.cc

Labels: Merge-Request-53 Merge-Request-54
Now that the fix in r419019 has baked on canary over the weekend, requesting a merge to both M54 and M53.  Note that we think this is the only fix for the security vulnerability in issue 644426 that can be merged to M53.

Comment 8 by dimu@chromium.org, Sep 19 2016

Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M53), manual review required.

Comment 9 by dimu@chromium.org, Sep 19 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 19 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bd1a6aec18520db3b7e2859c70b096943bf310e

commit 4bd1a6aec18520db3b7e2859c70b096943bf310e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Sep 19 17:17:52 2016

Block top-level navigations to nested URLs with extension origins from non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG= 645028 

Review-Url: https://codereview.chromium.org/2345473003
Cr-Commit-Position: refs/heads/master@{#419019}
(cherry picked from commit 4bfdc9292a6161980ba9a7a469d2d4515bebc6dd)

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

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

[modify] https://crrev.com/4bd1a6aec18520db3b7e2859c70b096943bf310e/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4bd1a6aec18520db3b7e2859c70b096943bf310e/chrome/browser/net/chrome_extensions_network_delegate.cc

Cc: awhalley@chromium.org
+ awhalley@ (Security TPM)
As of now, there is no M53 release plan for Chrome Desktop and this change is NOT yet baked in Beta. We can take this change in for future M53 respin if any (hopefully by then it will be baked in Beta too).

Comment 12 by nasko@chromium.org, Sep 19 2016

FYI, this is a fix for a sandbox escape vulnerability, which might actually warrant a respin of M53. What is the process of deciding whether we put out a new stable release?
+ awhalley@ (Security TPM), please reply to comment #12.
Labels: -Type-Bug Security_Impact-Stable Security_Severity-High Type-Bug-Security
Status: Fixed (was: Started)
(Changing bug type and adding some labels to get this picked up in queries - also it appears this can now be marked as fixed.)

Let me spin up a thread about it. It will hit Beta on Wednesday which will be good for baking if we do want to spin.
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 20 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: amineer@chromium.org
Cc: ananthak@google.com
+ awhalley@, please update the bug once this change is sufficiently baked in Beta and result looks good. I will approve M53 merge then. Thank you. 
Labels: -Merge-Review-53 Merge-Request-53
>48 hours in beta, requesting merge.

Comment 20 by dimu@chromium.org, Sep 23 2016

Labels: -Merge-Request-53 Merge-Review-53
[Automated comment] Request affecting a post-stable build (M53), manual review required.
Labels: -Merge-Review-53 Merge-Approved-53
Approving merge to M53 branch 2785 based on comment #19. Please merge ASAP. Thank you.
Project Member

Comment 22 by bugdroid1@chromium.org, Sep 23 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbf71ae0ae30ffd84974aebf1bc7fefe329d5091

commit dbf71ae0ae30ffd84974aebf1bc7fefe329d5091
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Sep 23 16:41:57 2016

Block top-level navigations to nested URLs with extension origins from non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG= 645028 

Review-Url: https://codereview.chromium.org/2345473003
Cr-Commit-Position: refs/heads/master@{#419019}
(cherry picked from commit 4bfdc9292a6161980ba9a7a469d2d4515bebc6dd)

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

Cr-Commit-Position: refs/branch-heads/2785@{#919}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/dbf71ae0ae30ffd84974aebf1bc7fefe329d5091/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/dbf71ae0ae30ffd84974aebf1bc7fefe329d5091/chrome/browser/net/chrome_extensions_network_delegate.cc

Labels: Release-3-M53

Comment 24 by k...@google.com, Oct 4 2016

What beta version did this go out in for testing?
I think this was in 54.0.2840.34 which was released 9/21/16.

Comment 26 Deleted

Comment 27 Deleted

Correct, this fix went out in Chrome Desktop beta version 54.0.2840.34 on 09/21/16.
Project Member

Comment 29 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab9e5bfea79174c7289d3c0a96acc04bc2c333fe

commit ab9e5bfea79174c7289d3c0a96acc04bc2c333fe
Author: alexmos <alexmos@chromium.org>
Date: Tue Oct 04 16:56:07 2016

Revert of Block top-level navigations to nested URLs with extension origins from non-extension processes. (patchset #1 id:1 of https://codereview.chromium.org/2366973002/ )

Reason for revert:
Broke blob: loads in <webview>.  See https://bugs.chromium.org/p/chromium/issues/detail?id=652077

Original issue's description:
> Block top-level navigations to nested URLs with extension origins from non-extension processes.
>
> Before this CL, it was possible for a web iframe with an unblessed
> extension frame to exploit the renderer, create a blob: or filesystem:
> URL in the extension frame context, then create a new top-level window
> and navigate it to that URL, which could end up putting the new window
> into a privileged extension process running attacker's code.
>
> BUG= 645028 
>
> Review-Url: https://codereview.chromium.org/2345473003
> Cr-Commit-Position: refs/heads/master@{#419019}
> (cherry picked from commit 4bfdc9292a6161980ba9a7a469d2d4515bebc6dd)
>
> Committed: https://chromium.googlesource.com/chromium/src/+/dbf71ae0ae30ffd84974aebf1bc7fefe329d5091

TBR=nasko@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 645028 
# Adding these as instructed by commit-bot:
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2391643004
Cr-Commit-Position: refs/branch-heads/2785@{#943}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/ab9e5bfea79174c7289d3c0a96acc04bc2c333fe/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/ab9e5bfea79174c7289d3c0a96acc04bc2c333fe/chrome/browser/net/chrome_extensions_network_delegate.cc

Project Member

Comment 30 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd20702d3b3dd5224beaa7fd7b876313b33b6b70

commit cd20702d3b3dd5224beaa7fd7b876313b33b6b70
Author: alexmos <alexmos@chromium.org>
Date: Tue Oct 04 22:09:31 2016

Fix blob URL blocking for apps that use <webview>.

It should be possible for an app to embed a <webview> and navigate it
to a blob URL that the app creates.  In a previous security fix
(r419019), this navigation was unintentionally blocked, due to
<webview> being considered a main frame as well as being in an
untrusted web process which is requesting a blob URL in a
chrome-extension:// scheme.

BUG=652077, 645028 

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

[modify] https://crrev.com/cd20702d3b3dd5224beaa7fd7b876313b33b6b70/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/cd20702d3b3dd5224beaa7fd7b876313b33b6b70/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/cd20702d3b3dd5224beaa7fd7b876313b33b6b70/chrome/test/data/extensions/platform_apps/web_view/shim/main.js

Project Member

Comment 31 by bugdroid1@chromium.org, Oct 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01928f6ddee40f6e2e5041013c2773e515176955

commit 01928f6ddee40f6e2e5041013c2773e515176955
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Oct 05 21:37:20 2016

Fix blob URL blocking for apps that use <webview>.

It should be possible for an app to embed a <webview> and navigate it
to a blob URL that the app creates.  In a previous security fix
(r419019), this navigation was unintentionally blocked, due to
<webview> being considered a main frame as well as being in an
untrusted web process which is requesting a blob URL in a
chrome-extension:// scheme.

BUG=652077, 645028 

Review-Url: https://codereview.chromium.org/2387323002
Cr-Commit-Position: refs/heads/master@{#422954}
(cherry picked from commit cd20702d3b3dd5224beaa7fd7b876313b33b6b70)

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

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

[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/test/data/extensions/platform_apps/web_view/shim/main.js

Cc: michaeln@chromium.org
Project Member

Comment 33 by bugdroid1@chromium.org, Oct 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/419c0f16840f7c9cf0cd600da55c4db4bd30c58d

commit 419c0f16840f7c9cf0cd600da55c4db4bd30c58d
Author: jam <jam@chromium.org>
Date: Thu Oct 13 02:09:16 2016

Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate.

The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread.

This fixes
ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed
with PlzNavigate.

BUG= 504347 , 645028 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/content/public/browser/navigation_handle.h
[modify] https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d/extensions/browser/extension_navigation_throttle.cc

Cc: mmenke@chromium.org
Project Member

Comment 35 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4bd1a6aec18520db3b7e2859c70b096943bf310e

commit 4bd1a6aec18520db3b7e2859c70b096943bf310e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Sep 19 17:17:52 2016

Block top-level navigations to nested URLs with extension origins from non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG= 645028 

Review-Url: https://codereview.chromium.org/2345473003
Cr-Commit-Position: refs/heads/master@{#419019}
(cherry picked from commit 4bfdc9292a6161980ba9a7a469d2d4515bebc6dd)

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

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

[modify] https://crrev.com/4bd1a6aec18520db3b7e2859c70b096943bf310e/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/4bd1a6aec18520db3b7e2859c70b096943bf310e/chrome/browser/net/chrome_extensions_network_delegate.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01928f6ddee40f6e2e5041013c2773e515176955

commit 01928f6ddee40f6e2e5041013c2773e515176955
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Oct 05 21:37:20 2016

Fix blob URL blocking for apps that use <webview>.

It should be possible for an app to embed a <webview> and navigate it
to a blob URL that the app creates.  In a previous security fix
(r419019), this navigation was unintentionally blocked, due to
<webview> being considered a main frame as well as being in an
untrusted web process which is requesting a blob URL in a
chrome-extension:// scheme.

BUG=652077, 645028 

Review-Url: https://codereview.chromium.org/2387323002
Cr-Commit-Position: refs/heads/master@{#422954}
(cherry picked from commit cd20702d3b3dd5224beaa7fd7b876313b33b6b70)

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

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

[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/browser/net/chrome_extensions_network_delegate.cc
[modify] https://crrev.com/01928f6ddee40f6e2e5041013c2773e515176955/chrome/test/data/extensions/platform_apps/web_view/shim/main.js

Project Member

Comment 37 by bugdroid1@chromium.org, Nov 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c3da4f745a8a4d67573ca386fc9ad24ced878d4

commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4
Author: alexmos <alexmos@chromium.org>
Date: Thu Nov 03 06:06:11 2016

Fix web accessible resource checks in ShouldAllowOpenURL

(This is joint work with nick@)

When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.

This CL fixes several problems with that check:

1. It doesn't work properly for blob/filesystem URL, since it
   references schemes on the destination GURL directly.  This leads to
   a security problem where all blob/filesystem URLs are allowed.  To
   fix this, use url::Origin to handle nested URL cases properly,
   similarly to other fixes in this area (e.g.,  issue 645028 ).

2. It ignores cases where the source SiteInstance's site URL isn't an
   HTTP, HTTPS, or extension scheme.  Thus, a new tab with a data or
   about:blank URL bypasses WAR checks.  This artificial restriction
   apparently was added to make a test pass several years ago.  This
   CL removes it, and fixes one test that was wrong and which relied
   on this logic. The test is
   ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
   window.open a non-web-accessible resource while incorrectly
   assuming it was web-accessible.  Since it did so from about:blank
   with a blank SiteInstance, this bug allowed the test to work.

3. The things that ShouldAllowOpenURL does block aren't blocked
   correctly for transfers.  RequestTransferURL rewrites the blocked
   URL to about:blank, and later, NavigatorImpl::NavigateToEntry
   decides that this is a transfer to the same RFH
   (is_transfer_to_same == true), so it happily lets the old
   navigation continue and load the *old* (disallowed) URL.  To fix
   this, we return early instead of rewriting dest_url to
   about:blank.

4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
   via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
   SiteInstance of the frame that's being navigated, rather than the
   SiteInstance in which the navigation on the proxy was started.
   RenderFrameProxyHost::OnOpenURL already passes the right
   SiteInstance as source_site_instance to RequestTransferURL, so it
   just needs to use it when it's available.

Two new tests are added:

NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.

The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two).  That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks.  The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.

Why don't our other checks work?
--------------------------------

1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes.  Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames.  Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.

2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs.  It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.

3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
  // Navigating the main frame to an extension URL is allowed, even if not
  // explicitly listed as web_accessible_resource.
  if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
    *allowed = true;
    return true;
  }

BUG= 656752 ,  645028 ,  652887 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/extension_resource_request_policy_apitest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/browser/extensions/window_open_apitest.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/chrome/test/data/extensions/uitest/window_open/manifest.json
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4/tools/metrics/histograms/histograms.xml

Project Member

Comment 38 by sheriffbot@chromium.org, Dec 27 2016

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