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}
9 files changed
