[Extensions] Change renderer-side web accessible resource determination

chrome/renderer/extensions/resource_request_policy.cc

 // Copyright (c) 2012 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/renderer/extensions/resource_request_policy.h"
 
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/common/extensions/chrome_manifest_url_handlers.h"
 #include "chrome/common/url_constants.h"
@@ skipping 12 matching lines @@
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
 namespace extensions {
 
 ResourceRequestPolicy::ResourceRequestPolicy(Dispatcher* dispatcher)
     : dispatcher_(dispatcher) {}
+ResourceRequestPolicy::~ResourceRequestPolicy() = default;
+
+void ResourceRequestPolicy::OnExtensionLoaded(const Extension& extension) {
+  if (WebAccessibleResourcesInfo::HasWebAccessibleResources(&extension) ||
+      // Extensions below manifest version 2 had all resources accessible by
+      // default.
+      // TODO(devlin): Two things - first, we might not have any v1 extensions
+      // anymore; second, this should maybe be included in
+      // HasWebAccessibleResources().
+      extension.manifest_version() < 2 ||
+      WebviewInfo::HasWebviewAccessibleResources(
+          extension, dispatcher_->webview_partition_id()) ||
+      // Hosted app icons are accessible.
+      // TODO(devlin): Should we incorporate this into
+      // WebAccessibleResourcesInfo?
+      (extension.is_hosted_app() && !IconsInfo::GetIcons(&extension).empty())) {
+    web_accessible_ids_.insert(extension.id());
+  }
+}
+
+void ResourceRequestPolicy::OnExtensionUnloaded(
+    const ExtensionId& extension_id) {
+  web_accessible_ids_.erase(extension_id);
+}
 
 // This method does a security check whether chrome-extension:// URLs can be
 // requested by the renderer. Since this is in an untrusted process, the browser
 // has a similar check to enforce the policy, in case this process is exploited.
 // If you are changing this function, ensure equivalent checks are added to
 // extension_protocols.cc's AllowExtensionResourceLoad.
 bool ResourceRequestPolicy::CanRequestResource(
     const GURL& resource_url,
     blink::WebLocalFrame* frame,
     ui::PageTransition transition_type) {
   CHECK(resource_url.SchemeIs(kExtensionScheme));
 
-  const Extension* extension =
-      RendererExtensionRegistry::Get()->GetExtensionOrAppByURL(resource_url);
-  if (!extension) {
-    // Allow the load in the case of a non-existent extension. We'll just get a
-    // 404 from the browser process.
+  GURL frame_url = frame->GetDocument().Url();
+
+  // The page_origin may be GURL("null") for unique origins like data URLs,
+  // but this is ok for the checks below.  We only care if it matches the
+  // current extension or has a devtools scheme.
+  GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL();
+
+  GURL extension_origin = resource_url.GetOrigin();
+
+  // We always allow loads in the following cases, regardless of web accessible
+  // resources:
+
+  // Empty urls (needed for some edge cases when we have empty urls).
+  if (frame_url.is_empty())
+    return true;
+
+  // Extensions requesting their own resources (frame_url check is for images,
+  // page_url check is for iframes).
+  // TODO(devlin): We should be checking the ancestor chain, not just the
+  // top-level frame. Additionally, we should be checking the security origin
+  // of the frame, to account for about:blank subframes being scripted by an
+  // extension parent (though we'll still need the frame origin check for
+  // sandboxed frames).
+  if (frame_url.GetOrigin() == extension_origin ||
+      page_origin == extension_origin) {
     return true;
   }
 
+  if (!ui::PageTransitionIsWebTriggerable(transition_type))
+    return true;
+
+  // Unreachable web page error page (to allow showing the icon of the
+  // unreachable app on this page).
+  if (frame_url == content::kUnreachableWebDataURL)
+    return true;
+
+  bool is_dev_tools = page_origin.SchemeIs(content::kChromeDevToolsScheme);
+  // Note: we check web_accessible_ids (rather than first looking up the

[lazyboy, 2017/07/10 22:36:39]

nit: |web_accessible_ids_|

I'd also expand this comment a bit.
This "return false" is the code that's protecting us from exposing
installed-ness info of extensions without WAR entries, right?
Also note the fact that we are not as concerned about extensions with WAR
entries.

[Devlin, 2017/07/10 22:52:30]

Done.

+  // extension in the registry and checking that) to be more resistant against
+  // timing attacks.
+  if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host()))
+    return false;

[lazyboy, 2017/07/10 22:36:39]

This used to show "Denying load of ... " message before (line 151) if extension
was installed. Not sure if that's a good thing or bad thing.

[Devlin, 2017/07/10 22:52:30]

It may have in some situations been useful, but adding it requires a difference
in timing again, so I don't think it's worth keeping (since web accessible
resources are documented elsewhere).

+
+  const Extension* extension =
+      RendererExtensionRegistry::Get()->GetExtensionOrAppByURL(resource_url);
+  if (is_dev_tools && !extension) {

[lazyboy, 2017/07/10 22:36:39]

Can you fold this and if in line 124:

if (is_dev_tools) {
  if (!extension) ... return true;
  else ... return true;
}

Then add

DCHECK(extension);

This devtools logic is hard to read.

[Devlin, 2017/07/10 22:52:30]

Done.  That is a bit cleaner; thanks!

+    // Allow the load in the case of a non-existent extension. We'll just get a
+    // 404 from the browser process.
+    // TODO(devlin): Can this happen? Does devtools potentially make requests
+    // to non-existent extensions?
+    return true;
+  }
+
+  // Devtools (chrome-extension:// URLs are loaded into frames of devtools to
+  // support the devtools extension APIs).
+  if (is_dev_tools &&
+      !chrome_manifest_urls::GetDevToolsPage(extension).is_empty()) {
+    return true;
+  }
+
   // Disallow loading of packaged resources for hosted apps. We don't allow
   // hybrid hosted/packaged apps. The one exception is access to icons, since
   // some extensions want to be able to do things like create their own
   // launchers.
   base::StringPiece resource_root_relative_path =
       resource_url.path_piece().empty() ? base::StringPiece()
                                         : resource_url.path_piece().substr(1);
   if (extension->is_hosted_app() &&
       !IconsInfo::GetIcons(extension)
           .ContainsPath(resource_root_relative_path)) {
     LOG(ERROR) << "Denying load of " << resource_url.spec() << " from "
                << "hosted app.";
     return false;
   }
 
   // Disallow loading of extension resources which are not explicitly listed
   // as web or WebView accessible if the manifest version is 2 or greater.
   if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(
           extension, resource_url.path()) &&
       !WebviewInfo::IsResourceWebviewAccessible(
           extension, dispatcher_->webview_partition_id(),
           resource_url.path())) {
-    GURL frame_url = frame->GetDocument().Url();
-
-    // The page_origin may be GURL("null") for unique origins like data URLs,
-    // but this is ok for the checks below.  We only care if it matches the
-    // current extension or has a devtools scheme.
-    GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL();
-
-    // Exceptions are:
-    // - empty origin (needed for some edge cases when we have empty origins)
-    bool is_empty_origin = frame_url.is_empty();
-    // - extensions requesting their own resources (frame_url check is for
-    //     images, page_url check is for iframes)
-    bool is_own_resource = frame_url.GetOrigin() == extension->url() ||
-                           page_origin == extension->url();
-    // - devtools (chrome-extension:// URLs are loaded into frames of devtools
-    //     to support the devtools extension APIs)
-    bool is_dev_tools =
-        page_origin.SchemeIs(content::kChromeDevToolsScheme) &&
-        !chrome_manifest_urls::GetDevToolsPage(extension).is_empty();
-    bool transition_allowed =
-        !ui::PageTransitionIsWebTriggerable(transition_type);
-    // - unreachable web page error page (to allow showing the icon of the
-    //   unreachable app on this page)
-    bool is_error_page = frame_url == content::kUnreachableWebDataURL;
-
-    if (!is_empty_origin && !is_own_resource &&
-        !is_dev_tools && !transition_allowed && !is_error_page) {
-      std::string message = base::StringPrintf(
-          "Denying load of %s. Resources must be listed in the "
-          "web_accessible_resources manifest key in order to be loaded by "
-          "pages outside the extension.",
-          resource_url.spec().c_str());
-      frame->AddMessageToConsole(
-          blink::WebConsoleMessage(blink::WebConsoleMessage::kLevelError,
-                                   blink::WebString::FromUTF8(message)));
-      return false;
-    }
+    std::string message = base::StringPrintf(
+        "Denying load of %s. Resources must be listed in the "
+        "web_accessible_resources manifest key in order to be loaded by "
+        "pages outside the extension.",
+        resource_url.spec().c_str());
+    frame->AddMessageToConsole(
+        blink::WebConsoleMessage(blink::WebConsoleMessage::kLevelError,
+                                 blink::WebString::FromUTF8(message)));
+    return false;
   }
 
   return true;
 }
 
 }  // namespace extensions