Fix web accessible resource checks in ShouldAllowOpenURL

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index b2838218b226b055177d6dd37aed44d9a2ef6a32..4719245a07c23610cf4afa50576ab9ba786edcaa 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -812,9 +812,17 @@ void NavigatorImpl::RequestTransferURL(
   Referrer referrer_to_use(referrer);
   FrameTreeNode* node = render_frame_host->frame_tree_node();
   SiteInstance* current_site_instance = render_frame_host->GetSiteInstance();
-  if (!GetContentClient()->browser()->ShouldAllowOpenURL(current_site_instance,
-                                                         url)) {
-    dest_url = GURL(url::kAboutBlankURL);
+  // It is important to pass in the source_site_instance if it is available
+  // (such as when navigating a proxy).  See https://crbug.com/656752.
+  if (!GetContentClient()->browser()->ShouldAllowOpenURL(
+          source_site_instance ? source_site_instance : current_site_instance,

[alexmos, 2016/10/28 00:29:42]

We could get here two ways: from RFPH::OnOpenURL or from a transfer.  Transfers
always pass in a null source_site_instance, so that case will function the same
as before.  Proxy navigations always pass in a valid source_site_instance (the
proxy's SiteInstance), and I think that one should be used in that case. (If A
is navigating B1 to B2, we want to perform the WAR checks to see if A is allowed
to load B2.)

Can redirects happen for chrome-extension:// URLs?  (I'm trying to think whether
that could be abused to lose the source_site_instance if we navigate a proxy to
a URL that redirects to another URL.)

[ncarter (slow), 2016/10/28 21:45:03]

I looked at the extension protocol handler code, re: redirects.

I don't think we can get a redirect response from a load of a
chrome-extension:// resource, but it looks like we actually do allow an http
server to redirect to a chrome-extension URL, and it seems like tests are
actually doing that:

https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_brows...

Very surprised by that.

blobs and filesystems are not allowed as redirect targets, though, so while
redirects might be another possible avenue around enforcement of web accessible
resources, I don't think redirects could be used as a way around the defenses
for bug 656752.

[alexmos, 2016/10/31 23:34:48]

Yes, that's very surprising.  Sounds like it's worth looking into whether that
can be used to bypass the regular WAR checks on transfers in a follow-up.

+          url)) {
+    // It is important to return here, rather than rewrite the dest_url to
+    // about:blank.  The latter won't actually have any effect when
+    // transferring, as NavigateToEntry will think that the transfer is to the
+    // same RFH that started the navigation and let the existing navigation
+    // (for the disallowed URL) proceed.
+    return;
   }
 
   // TODO(creis): Determine if this transfer started as a browser-initiated