[Extensions] Change renderer-side web accessible resource determination

chrome/renderer/extensions/resource_request_policy.cc

Index: chrome/renderer/extensions/resource_request_policy.cc
diff --git a/chrome/renderer/extensions/resource_request_policy.cc b/chrome/renderer/extensions/resource_request_policy.cc
index 0519a63fe5363c77ad5c83e97caa4cfbc6fca418..3c8b58f23a0bd5bc0b487b4f96b1c20c3c04fc8d 100644
--- a/chrome/renderer/extensions/resource_request_policy.cc
+++ b/chrome/renderer/extensions/resource_request_policy.cc
@@ -30,6 +30,28 @@ namespace extensions {
 
 ResourceRequestPolicy::ResourceRequestPolicy(Dispatcher* dispatcher)
     : dispatcher_(dispatcher) {}
+ResourceRequestPolicy::~ResourceRequestPolicy() = default;
+
+void ResourceRequestPolicy::OnExtensionAdded(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.
+      (extension.is_hosted_app() && !IconsInfo::GetIcons(&extension).empty())) {

[nasko, 2017/06/30 18:08:10]

I see this call scattered a few places already. Should we abstract this behind
HasWebAccessibleResources which can implement the hosted apps check and return
true if icons are present? It feels fragile to always have this check all over
the place.

[Devlin, 2017/07/06 00:40:39]

Most places check a specific path to see if it's referencing an icon, so that we
would be able to coalesce many calls (as far as I can see - maybe I'm missing
some?).  Additionally, I'm not sure we'd always want to include the icons in the
web accessible resources check.  I'd like to save that for a separate CL to keep
this one targeted, but I've added a TODO.

+    web_accessible_ids_.insert(extension.id());
+  }
+}
+
+void ResourceRequestPolicy::OnExtensionRemoved(
+    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
@@ -42,11 +64,50 @@ bool ResourceRequestPolicy::CanRequestResource(
     ui::PageTransition transition_type) {
   CHECK(resource_url.SchemeIs(kExtensionScheme));
 
+  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();

[nasko, 2017/06/30 18:08:09]

Ok, I realize this is move only operation here. However, why are we dependent on
the top level page origin?! I think we fixed browser side to check for any
cross-origin ancestor and deny the navigation. Shouldn't we be doing the same
here?

[Devlin, 2017/07/06 00:40:38]

Yes, we probably should.  I tried this (see PS4), but the implementation was
incomplete - specifically, we're missing the allowance for subresources to be
included by an extension frame regardless of web accessible resources (so that
e.g. frame.html can include frame.js even if only frame.html is specified).  On
the browser side, we can check the resource type - is there an analogous check
here?

Regardless, I'd have a slight preference for tackling this separately (mostly
for revert reasons), so I've added a TODO here.  But if it's a super simple
addition, I could be persuaded to do it in this patch. :)

[alexmos, 2017/07/06 18:20:06]

(Pitching in here since Nasko's OOO for a while)

I think we can check that from WebURLRequest  (something similar to how
is_navigational_request is set in RenderFrameImpl::WillSendRequest), but that
will require additional plumbing into here.  So a TODO is probably ok for this
CL.

[Devlin, 2017/07/06 19:14:56]

Acknowledged; will leave the TODO and submit as-is for now.

+
+  GURL extension_origin = resource_url.GetOrigin();
+
+  // We always allow loads in the following cases, regardless of web accessible
+  // resources:
+
+  // Empty origin (needed for some edge cases when we have empty origins).

[nasko, 2017/06/30 18:08:09]

Empty origins or empty URLs?

[Devlin, 2017/07/06 00:40:39]

This was copy-pasted, but code implies url, so changed it.

+  if (frame_url.is_empty())
+    return true;

[nasko, 2017/06/30 18:08:09]

In general, if you want timing neutral code, early returns are bad. Adding a
boolean variable that holds the return value and updating it on each check, then
returning it at the end will ensure all if branches are executed. It is also the
case that if branches can shortcut based on conditions and each method you call
can have different timing, but at least early returns aren't leaking timing
data.

[Devlin, 2017/07/06 00:40:38]

Except if we return true, it's because the navigation is allowed, so it doesn't
matter (see also response to Alex's comment below).  Because of that, I think
the early returns here are fine.

+
+  // Extensions requesting their own resources (frame_url check is for images,
+  // page_url check is for iframes)
+  if (frame_url.GetOrigin() == extension_origin ||

[alexmos, 2017/06/30 21:10:17]

I know you're not changing this logic... but why do we not want
frame->GetSecurityContext()->GetSecurityOrigin() here?  What if an extension
opens an about:blank popup and then scripts it to request a resource?  Its
frame_url will be "about:blank" but SecurityOrigin will be the extension's
origin -- shouldn't this need the latter?

OTOH, the document's URL might be needed to be able to handle sandboxed frames,
so maybe we need to check both?

[Devlin, 2017/07/06 00:40:38]

Sorry, you lost me at the sandboxed frame part - could you elaborate?

[alexmos, 2017/07/06 18:20:06]

If a frame is sandboxed via <iframe sandbox>, the frame's origin will be
unique/"null" (unless it includes the allow-same-origin sandbox attribute).  So,
comparing frame->GetSecurityContext()->GetSecurityOrigin() to extension_origin
won't work in that case.  Blink code deals with this in other places (such as
[1]) by comparing to the origin created from the document's URL instead, which
stays valid for sandboxed frames, except that remote frames don't have that URL
replicated, so it's always been a pain for us. :(

So I guess I'm saying that:
1. To handle sandboxed frames correctly, the current approach of looking at
frame_url.GetOrigin() is actually correct.
2. But to handle inherited origins in about:blank cases, you'd need to also look
at frame->GetSecurityContext()->GetSecurityOrigin().

I realized my original about:blank popup example would probably be handled by
"page_origin == extension_origin", since page_origin comes from SecurityOrigin,
so to break the current check, we could try something like a page embedding an
extension subframe, which embeds an about:blank grandchild frame and scripts it
to load something.

This isn't something for this CL but might be something to consider as part of
your TODO to fix this check.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...

[Devlin, 2017/07/06 19:14:56]

Thanks for the explanation!  I've expanded the TODO, and will try to follow up
in another CL.

+      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);
+  if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) {

[alexmos, 2017/06/30 21:10:17]

nit: no { needed

[alexmos, 2017/06/30 21:10:18]

Maybe add a comment about why you needed to add this?

[Devlin, 2017/07/06 00:40:38]

Done.

[Devlin, 2017/07/06 00:40:39]

Done.

[alexmos, 2017/07/06 18:20:06]

Was it added somewhere where I missed it? :)  I was thinking about something to
explain why web_accessible_ids_ is maintained/checked so that someone doesn't
reintroduce the timing issue later.

[Devlin, 2017/07/06 19:14:56]

Whoops - it *was* added
(https://codereview.chromium.org/2958343002/diff2/60001:80001/chrome/renderer/...).
 It got reverted when I removed the ancestor check that was failing in patch set
4.  Reinstated.

+    return false;

[alexmos, 2017/06/30 21:10:18]

Just to clarify: does this help for an extension that does have some
web-accessible resources?  If it's not installed, we'll return here, and if it
is, we'll exit after the extension lookup and the WAR check, which seems like it
can still be timed.  You said the repro no longer works, is that for all
extensions or the non-WAR ones?  There seem to be plenty of extensions with WARs
on https://poc.tom.vg/extension-fingerprint/

[Devlin, 2017/07/06 00:40:39]

If an extension has a web accessible resource, it is inherently detectable. 
There would be no reason to test an arbitrary, non-accessible file and compare
timing results if you could instead test an accessible file and determine the
result by checking if the frame is allowed.  We don't really care about
protecting the "allowed" case, because if the load's allowed, we already leaked
whether or not the extension is installed.

[alexmos, 2017/07/06 18:20:06]

Acknowledged.

+  }
+
   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.
+  DCHECK(extension);

[alexmos, 2017/06/30 21:10:17]

I'm curious if it's possible to hit this when |is_dev_tools| is true.  Is the
expectation that devtools should never make a chrome-extension:// request to a
non-existing (devtools) extension?  

[Devlin, 2017/07/06 00:40:38]

Umm... I'd hope not, but I'm not 100% sure.  I've added a TODO and a graceful
exit.  In a followup, I'll add UMA or just remove the check (in which case we
can monitor for crashes and revert that small change rather than this one).

[alexmos, 2017/07/06 18:20:06]

Acknowledged.

+
+  // 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;
   }
 
@@ -72,43 +133,15 @@ bool ResourceRequestPolicy::CanRequestResource(
       !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;