Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/chrome_content_browser_client_extensions_part.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/extensions/chrome_content_browser_client_extensions_part.h"
 
 #include <stddef.h>
 
 #include <set>
 
 #include "base/command_line.h"
+#include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
 #include "chrome/browser/extensions/extension_webkit_preferences.h"
 #include "chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_extension_message_filter.h"
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
@@ skipping 467 matching lines @@
   const Extension* extension =
       extension_info_map->extensions().GetExtensionOrAppByURL(first_party_url);
   // Don't allow a service worker for an extension url with no extension (this
   // could happen in the case of, e.g., an unloaded extension).
   return extension != nullptr;
 }
 
 // static
 bool ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL(
     content::SiteInstance* site_instance,
-    const GURL& from_url,
     const GURL& to_url,
     bool* result) {
   DCHECK(result);
 
+  url::Origin to_origin(to_url);
+  if (to_origin.scheme() != kExtensionScheme) {
+    // We're not responsible for protecting this resource.
+    return false;
+  }
+
   // Do not allow pages from the web or other extensions navigate to
   // non-web-accessible extension resources.
-  if (to_url.SchemeIs(kExtensionScheme) &&
-      (from_url.SchemeIsHTTPOrHTTPS() || from_url.SchemeIs(kExtensionScheme))) {

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

Lifting the (bogus) scheme restriction on from_url only affected
ExtensionBrowserTest.WindowOpenNoPrivileges, which was just an invalid test
(attempted to window.open a non-WAR page; the fix is to make it WAR).  However,
I also added a DWOC for cases that we wouldn't have blocked previously due to
this below, so we can catch any regressions.

-    Profile* profile = Profile::FromBrowserContext(
-        site_instance->GetProcess()->GetBrowserContext());
-    ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
-    if (!registry) {
-      *result = true;
-      return true;
-    }
-    const Extension* extension =
-        registry->enabled_extensions().GetExtensionOrAppByURL(to_url);
-    if (!extension) {
-      *result = true;
-      return true;
-    }
-    const Extension* from_extension =
-        registry->enabled_extensions().GetExtensionOrAppByURL(
-            site_instance->GetSiteURL());
-    if (from_extension && from_extension->id() == extension->id()) {
-      *result = true;
-      return true;
-    }
 
-    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(
-            extension, to_url.path())) {
-      *result = false;
-      return true;
-    }
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->GetProcess()->GetBrowserContext());
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
+  if (!registry) {
+    *result = true;
+    return true;
   }
-  return false;
+
+  const Extension* extension =
+      registry->enabled_extensions().GetByID(to_origin.host());
+  if (!extension) {
+    *result = true;
+    return true;
+  }
+
+  GURL site_url(site_instance->GetSiteURL());
+  const Extension* from_extension =
+      registry->enabled_extensions().GetExtensionOrAppByURL(site_url);
+  if (from_extension && from_extension->id() == extension->id()) {
+    *result = true;
+    return true;
+  }
+
+  // Blob and filesystem URLs are never considered web-accessible.  See
+  // https://crbug.com/656752.
+  if (to_url.SchemeIsFileSystem() || to_url.SchemeIsBlob()) {
+    // TODO(alexmos): Temporary instrumentation to find any regressions for
+    // this blocking.  Remove after verifying that this is not breaking any
+    // legitimate use cases.
+    char site_url_copy[256];
+    base::strlcpy(site_url_copy, site_url.spec().c_str(),
+                  arraysize(site_url_copy));
+    base::debug::Alias(&site_url_copy);
+    char to_origin_copy[256];
+    base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
+                  arraysize(to_origin_copy));
+    base::debug::Alias(&to_origin_copy);
+    base::debug::DumpWithoutCrashing();
+
+    *result = false;
+    return true;
+  }
+
+  if (WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
+                                                          to_url.path())) {
+    *result = true;
+    return true;
+  }
+
+  if (!site_url.SchemeIsHTTPOrHTTPS() && !site_url.SchemeIs(kExtensionScheme)) {
+    // TODO(alexmos): Previous version of this function skipped the
+    // web-accessible resource checks in this case.  Collect data to catch
+    // any regressions, and then remove this.
+    char site_url_copy[256];
+    base::strlcpy(site_url_copy, site_url.spec().c_str(),
+                  arraysize(site_url_copy));
+    base::debug::Alias(&site_url_copy);
+    char to_origin_copy[256];
+    base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
+                  arraysize(to_origin_copy));
+    base::debug::Alias(&to_origin_copy);
+    base::debug::DumpWithoutCrashing();

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

Should we also collect UMA stats for some of these cases, or is the DWOC
sufficient?

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

Let's add UMA metrics too, since they will be mergeable, unlike these DWOCs.

(An extension-id-valued Rappor trial would actually be ideal for this, but I
don't think we have a drop-in way to create them.)

This is really on the edge of what DWOC is for, but I'm okay with it. In general
we shouldn't be using DWOC to collect metrics about webpage behavior, but the
second DWOC here targets a rare and suspicious enough case that I think it is
okay for canary, and the first is targeted to exactly the behavior we're locking
down.

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

Metrics added, PTAL.

+  }
+
+  *result = false;
+  return true;
 }
 
 // static
 std::unique_ptr<content::VpnServiceProxy>
 ChromeContentBrowserClientExtensionsPart::GetVpnServiceProxy(
     content::BrowserContext* browser_context) {
 #if defined(OS_CHROMEOS)
   chromeos::VpnService* vpn_service =
       chromeos::VpnServiceFactory::GetForBrowserContext(browser_context);
   if (!vpn_service)
@@ skipping 141 matching lines @@
     command_line->AppendSwitch(switches::kExtensionProcess);
   }
 }
 
 void ChromeContentBrowserClientExtensionsPart::ResourceDispatcherHostCreated() {
   content::ResourceDispatcherHost::Get()->RegisterInterceptor(
       "Origin", kExtensionScheme, base::Bind(&OnHttpHeaderReceived));
 }
 
 }  // namespace extensions