Limit protection of clients[0-9]*.google.com to requests from browser.

extensions/browser/api/web_request/web_request_permissions.cc

Index: extensions/browser/api/web_request/web_request_permissions.cc
diff --git a/extensions/browser/api/web_request/web_request_permissions.cc b/extensions/browser/api/web_request/web_request_permissions.cc
index 8b2f2f49742858767d6cbab0465b34f81dd181df..e9d19d90b9862065a0ff14938ba760efeee97271 100644
--- a/extensions/browser/api/web_request/web_request_permissions.cc
+++ b/extensions/browser/api/web_request/web_request_permissions.cc
@@ -8,6 +8,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "chromeos/login/login_state.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/resource_request_info.h"
 #include "extensions/browser/extension_navigation_ui_data.h"
 #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
@@ -42,11 +43,13 @@ bool g_allow_all_extension_locations_in_public_session = false;
 // Returns true if the URL is sensitive and requests to this URL must not be
 // modified/canceled by extensions, e.g. because it is targeted to the webstore
 // to check for updates, extension blacklisting, etc.
-bool IsSensitiveURL(const GURL& url) {
+bool IsSensitiveURL(const GURL& url, bool is_request_from_renderer) {
   // TODO(battre) Merge this, CanExtensionAccessURL and
   // PermissionsData::CanAccessPage into one function.
   bool sensitive_chrome_url = false;
-  const base::StringPiece& host = url.host_piece();
+  base::StringPiece host = url.host_piece();
+  while (host.ends_with("."))
+    host.remove_suffix(1u);
   const char kGoogleCom[] = "google.com";
   const char kClient[] = "clients";
   if (url.DomainIs(kGoogleCom)) {
@@ -54,23 +57,32 @@ bool IsSensitiveURL(const GURL& url) {
     // This protects requests to several internal services such as sync,
     // extension update pings, captive portal detection, fraudulent certificate
     // reporting, autofill and others.
-    if (base::StartsWith(host, kClient, base::CompareCase::SENSITIVE)) {
+    //
+    // These URLs are only protected for requests from the browser, not
+    // for requests from renderers, because clients*.google.com also used
+    // by websites.
+    base::StringPiece::size_type pos = host.rfind(kClient);

[Devlin, 2017/05/04 15:33:29]

Is this change to protect subdomains of clients*.google.com?  Do we want/need to
do that?

[Devlin, 2017/05/04 15:33:29]

nit: given the area of code this is called in and how sensitive it is to
performance, can we only do this find if is_request_from_renderer is true?

[battre, 2017/05/04 15:53:46]

Sure, will do this if Matt is ok with the general direction of this CL.

[battre, 2017/05/04 15:53:46]

Yes, this is to generalize the code. A couple of valid test cases failed.

.clients1.google.com would not match
a.clients1.google.com would not match

[battre, 2017/05/10 09:18:38]

Done.

+    if (!is_request_from_renderer && pos != base::StringPiece::npos) {

[mmenke, 2017/05/04 15:54:41]

So we're only filtering out sensitive requests not from the renderer?  Could we
just filter out all non-renderer requests instead?  Not a big fan of host-based
filtering here.

[Devlin, 2017/05/04 17:31:25]

I'm not comfortable with that (yet).  We have a *ton* of browser-initiated
requests (including some that might send cookies or other sensitive data), and I
don't think I'd be confident in saying that all of them are critical to browser
functionality and shouldn't be interceptable.  As an example, we use a
browser-initiated request to get thumbnail image data in ThumbnailSource, but
that seems like something that *should* be visible to extensions.

[mmenke, 2017/05/04 17:33:16]

So what if those thumbnails are from a clients.google.com URL?

[Devlin, 2017/05/04 18:06:00]

For want of a better solution, we'll protect them.  We've had some discussions
on the bug and internal thread about a better long-term solution for this
(relying on network annotations or using a different request context for
requests that should be hidden) which would handle these cases in a better way,
but for now I think this is a reasonable compromise.

[mmenke, 2017/05/05 14:35:14]

Could we instead have the initiators tag sensitive requests?  I'm not a fan of
using a magic domain, particularly as we have no idea just what services it is
and is not used for.

[Devlin, 2017/05/05 16:16:03]

Yes - that's what I was mentioning about relying on network annotations (which
could have a "sensitive" field) or using a different request context (which
would imply it's sensitive).  The problem is that both of those are much larger
projects, and we should have a fix for this problem sooner rather than later.

[mmenke, 2017/05/05 16:18:03]

Sorry, completely missed that.

[battre, 2017/05/10 09:18:37]

Acknowledged.

       bool match = true;
-      for (base::StringPiece::const_iterator
-               i = host.begin() + strlen(kClient),
-               end = host.end() - (strlen(kGoogleCom) + 1);
-           i != end; ++i) {
-        if (!isdigit(*i)) {
-          match = false;
-          break;
+      if (pos > 0 && host[pos - 1] != '.') {
+        match = false;
+      } else {
+        for (base::StringPiece::const_iterator

[Devlin, 2017/05/04 15:33:29]

I think this loop is right, but it's kind of a shame that we have to get so
close to reparsing the url.  mmenke@, do you know of any utilities that do this
type of thing already?  e.g., this would be much easier if there was a
GetHostPieces() type method that returned a vector of the parts of the URL host.

[battre, 2017/05/10 09:18:37]

Leaving this as is. I did not find anything better.

+                 i = host.begin() + pos + strlen(kClient),
+                 end = host.end() - (strlen(kGoogleCom) + 1);
+             i != end; ++i) {
+          if (!isdigit(*i)) {
+            match = false;
+            break;
+          }
         }
       }
       sensitive_chrome_url = sensitive_chrome_url || match;
     }
-    // This protects requests to safe browsing, link doctor, and possibly
-    // others.
+
+    // Safebrowsing and Chrome Webstore URLs are also protected for requests
+    // from renderers.
     sensitive_chrome_url = sensitive_chrome_url ||
-                           url.DomainIs("clients.google.com") ||
                            url.DomainIs("sb-ssl.google.com") ||
                            (url.DomainIs("chrome.google.com") &&
                             base::StartsWith(url.path_piece(), "/webstore",
@@ -85,7 +97,7 @@ bool WebRequestPermissions::HideRequest(
     const extensions::InfoMap* extension_info_map,
     const net::URLRequest* request,
     extensions::ExtensionNavigationUIData* navigation_ui_data) {
-  // Hide requests from the Chrome WebStore App or signin process.
+  // Hide requests from the Chrome WebStore App, signin process and WebUI.
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
   if (info) {
     int process_id = info->GetChildID();
@@ -100,10 +112,20 @@ bool WebRequestPermissions::HideRequest(
                                                    process_id)) {
       return true;
     }
+
+    // Always hide requests from WebUI. This is ok based on the assumption
+    // that WebUI and regular sites never share the same renderer process
+    // and the assumption that WebUI deserves protection from extensions.
+    if (content::ChildProcessSecurityPolicy::GetInstance()->HasWebUIBindings(
+            process_id)) {

[Devlin, 2017/05/04 15:33:29]

I'd actually prefer we only do this in the case of IsSensitiveURL().  For
instance, the NTP might have webui bindings, but I think that requests from that
to the web *should* be interceptable.

Also, it'd be nice to add webui-related test cases if we can.

[battre, 2017/05/04 15:53:46]

Sure, will do this if Matt is ok with the general direction.
This one will be really difficult because it is a huge integration test. Given
the flakiness of the current webrequest API tests and the fact that many of them
got disabled, this would be a major undertaking. I definitely won't be able to
get that done before Google I/O and the next branch point.

[mmenke, 2017/05/04 15:56:33]

I'm fine with that.

[Devlin, 2017/05/04 17:31:25]

We could just pass a bool is_webui_process to IsSensitiveURL() and stub it out
in the tests (just like you did with is_request_from_renderer).  That's fine for
me, since hopefully HasWebUIBindings() is tested. :)

[battre, 2017/05/10 09:18:37]

Done.

+      return true;
+    }
   }
 
   const GURL& url = request->url();
-  return IsSensitiveURL(url) || !HasWebRequestScheme(url);
+  bool is_request_from_renderer = info != nullptr;
+  return IsSensitiveURL(url, is_request_from_renderer) ||
+         !HasWebRequestScheme(url);
 }
 
 // static