Tighten IO thread blob/filesystem URL checks for apps with webview permission.

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 5c7e33fad911c45680260f656c09cc40b205be15..7860b65f30f6f7bcf1ec6d6ae88c4916870c0234 100644
--- a/chrome/browser/net/chrome_extensions_network_delegate.cc
+++ b/chrome/browser/net/chrome_extensions_network_delegate.cc
@@ -10,12 +10,14 @@
 #include "net/base/net_errors.h"
 
 #if defined(ENABLE_EXTENSIONS)
+#include "base/debug/dump_without_crashing.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/api/proxy/proxy_api.h"
 #include "chrome/browser/extensions/event_router_forwarder.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_navigation_ui_data.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/browser_side_navigation_policy.h"
@@ -209,8 +211,20 @@ int ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest(
         extension &&
         extension->permissions_data()->HasAPIPermission(
             extensions::APIPermission::kWebView);
-    if (!has_webview_permission)
+    // Check whether the request is coming from a <webview> guest process via
+    // ChildProcessSecurityPolicy.  A guest process should have already been
+    // granted permission to request |origin| when its WebContents was created.
+    // See https://crbug.com/656752.
+    auto* policy = content::ChildProcessSecurityPolicy::GetInstance();
+    bool from_guest =
+        policy->HasSpecificPermissionForOrigin(info->GetChildID(), origin);

[ncarter (slow), 2016/10/20 17:46:50]

Does correctness here depend on my fix for the too-early
IsIsolateExtensionsEnabled() call?

I think it doesn't, but interaction is that other the existing tests (which we
want to validate this change against) fail because of that bug?

[alexmos, 2016/10/20 18:40:23]

I think this is independent.  The affected test for that bug
(NestedURLNavigationsToExtensionBlocked) fails with
--force-fieldtrials=SiteIsolationExtensions/Control independently of this CL,
and the new test here actually passes with
--force-fieldtrials=SiteIsolationExtensions/Control.    In practice, the bug
would cause a user in the Control group to kill the renderer when it tries to
create a filesystem URL from an unblessed extension subframe, before it tries to
navigate a main frame to it (so before we get here), right?

+    if (!has_webview_permission || !from_guest) {
+      // TODO(alexmos): Temporary instrumentation to find any regressions for
+      // this blocking.  Remove after verifying that this is not breaking any
+      // legitimate use cases.

[ncarter (slow), 2016/10/20 17:46:50]

I recommend:

base::debug::Alias(&origin);
base::debug::Alias(&from_guest);

[alexmos, 2016/10/20 18:40:23]

Done.  Good idea.

+      base::debug::DumpWithoutCrashing();

[ncarter (slow), 2016/10/20 17:46:50]

There is some risk of ddos from really prolific DWOCs. But since this is
guaranteed to be an extension blob navigation I think it's okay. If it happens
in practice, I don't worry about a storm of them making a bad situation worse
for the user.

I'm thinking we'll need at least 24h bake time on a M55 release, to convince
ourselves that this isn't happening en mass for the non-oopif group. And maybe
we can reach out to the kiosk folks for manual vetting?

[alexmos, 2016/10/20 18:40:23]

That sounds good.  We'll definitely want enough baking time on M55 to look for
these crashes.  I'll also reach out to my kiosk contacts to make sure they test
this.   If we're concerned about volume, we could just not merge the DWOC to
M54, though I agree that it's unlikely to be a problem.  But we can see what
happens in M55 and go from there.

[ncarter (slow), 2016/10/20 19:05:56]

Good, we are on the same page. The DWOC should be fine in M54 -- the ifs()s
above are very specific. I made the opposite decision with the
filesystem-url-origin DWOC that's deep in ChildProcessSecurityPolicy, because it
was called from so many different contexts.

       return net::ERR_ABORTED;
+    }
   }
 
   return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest(