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

chrome/browser/net/chrome_extensions_network_delegate.cc

Index: chrome/browser/net/chrome_extensions_network_delegate.cc
diff --git a/chrome/browser/net/chrome_extensions_network_delegate.cc b/chrome/browser/net/chrome_extensions_network_delegate.cc
index 0f2f38bc43d83e528e3aa7e19dd57e87c6aef00d..6b88acf93169413bf94ecc0de2ac704bfa72d491 100644
--- a/chrome/browser/net/chrome_extensions_network_delegate.cc
+++ b/chrome/browser/net/chrome_extensions_network_delegate.cc
@@ -20,6 +20,7 @@
 #include "extensions/browser/api/web_request/web_request_api.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/browser/process_manager.h"
+#include "extensions/common/constants.h"
 #include "net/url_request/url_request.h"
 
 using content::BrowserThread;
@@ -159,6 +160,21 @@ int ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest(
     net::URLRequest* request,
     const net::CompletionCallback& callback,
     GURL* new_url) {
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request);
+  GURL url(request->url());
+
+  // Block top-level navigations to blob: or filesystem: URLs with extension
+  // origin from non-extension processes.  See https://crbug.com/645028.
+  bool is_nested_url = url.SchemeIsFileSystem() || url.SchemeIsBlob();
+  bool is_navigation =
+      info && content::IsResourceTypeFrame(info->GetResourceType());
+  if (is_nested_url && is_navigation && info->IsMainFrame() &&
+      url::Origin(url).scheme() == extensions::kExtensionScheme) {

[Devlin, 2016/09/15 21:05:10]

nit: Is there a reason to prefer this over GURL::SchemeIs?

[alexmos, 2016/09/15 21:45:59]

Yes, this uses url::Origin(url).scheme() because we want to look at the inner
URL's scheme (i.e., "chrome-extension").  Sadly, GURL::SchemeIs(url) will just
return "blob" or "filesystem", and actually yesterday some of us here were
discussing how feasible it is to change that to return the inner scheme instead.
 Further, GURL::GetOrigin() doesn't work correctly for blob URLs (but works fine
for filesystem: URLs).  url::Origin is the only place where both blob and
filesystem are handled sanely.  It's a mess. :(

[alexmos, 2016/09/15 21:51:40]

Also CC-ing mkwst@ for awareness and any thoughts on how we could improve
blob/filesystem handling in GURLs.

[Devlin, 2016/09/15 21:56:48]

...yuck.  Regardless, fine for this CL, but...wow.

+    if (!extension_info_map_->process_map().Contains(info->GetChildID()))

[Devlin, 2016/09/15 21:05:10]

optional nit: I'd be fine using and && here with the above if, but up to you.

[alexmos, 2016/09/15 21:45:59]

Done.

+      return net::ERR_ABORTED;

[alexmos, 2016/09/15 17:43:02]

I considered returning ERR_BLOCKED_BY_CLIENT here, but ERR_ABORTED seems to
behave a bit safer, as it commits the blocked page as an about:blank page with
the initiator's origin, whereas ERR_BLOCKED_BY_CLIENT shows a sad tab error page
and commits the blocked URL (i.e., blob:...) with a null origin (due to the
chromewebstore data URL used to show the error page), and having the blocked URL
as the last committed URL seems dangerous per our recent discussions with
Charlie and Nasko.

+  }
+
   return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest(
       profile_, extension_info_map_.get(), request, callback, new_url);
 }