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

chrome/browser/net/chrome_extensions_network_delegate.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/chrome_extensions_network_delegate.h"
 
 #include <stdint.h>
 
 #include "base/macros.h"
 #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"
 #include "extensions/browser/api/web_request/web_request_api.h"
 #include "extensions/browser/extension_navigation_ui_data.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/common/permissions/api_permission.h"
 #include "net/url_request/url_request.h"
 
@@ skipping 173 matching lines @@
       !extension_info_map_->process_map().Contains(info->GetChildID()) &&
       !content::IsBrowserSideNavigationEnabled()) {
     // Relax this restriction for apps that use <webview>.  See
     // https://crbug.com/652077.
     const extensions::Extension* extension =
         extension_info_map_->extensions().GetByID(origin.host());
     bool has_webview_permission =
         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(
       profile_, extension_info_map_.get(),
       GetExtensionNavigationUIData(request), request, callback, new_url);
 }
 
 int ChromeExtensionsNetworkDelegateImpl::OnBeforeStartTransaction(
     net::URLRequest* request,
     const net::CompletionCallback& callback,
@@ skipping 177 matching lines @@
 }
 
 net::NetworkDelegate::AuthRequiredResponse
 ChromeExtensionsNetworkDelegate::OnAuthRequired(
     net::URLRequest* request,
     const net::AuthChallengeInfo& auth_info,
     const AuthCallback& callback,
     net::AuthCredentials* credentials) {
   return net::NetworkDelegate::AUTH_REQUIRED_RESPONSE_NO_ACTION;
 }