[Backport] CVE-2019-13754: Insufficient policy enforcement in extensions. (1/3)

Manual backport of relevant parts of
Use |initiator_origin| in ExtensionNavigationThrottle.

Before this CL, ExtensionNavigationThrottle would more-or-less ignore
main frame navigations (except for special-casing some GuestView
scenarios).  This allowed some undesirable navigations as pointed out in
https://crbug.com/442579.  After this CL the throttle continues with
WebAccessibleResources checks even for main frames.

Before this CL, ExtensionNavigationThrottle would check if the target
is one of WebAccessibleResources iff the frame being navigated has an
ancestor that has a different origin than the target of the navigation.
This doesn't work for main frames (which have no parent).  This could
be addressed by also going over opener relationship, but the right fix
here is to directly consider |initiator_origin| (which is trustworthy
and browser-verified).  After this CL, |initiator_origin| is considered.

Applying the throttle checks to more navigations means that the throttle
has to replicate some exceptions from
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
Specifically, the throttle needs to ignore navigations ignored by
origins with chrome://, chrome-search:// and devtools:// schemes.

Additionally the changes mean that opaque origins (even if they have
been derived from an extension origin) cannot navigate to
non-WebAccessibleResources.  This requires tweaking the
sandboxed_pages_csp test.

Bug: 442579
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Change-Id: I861344f323ebbe328770a49e95102b7f9d1300b1
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

3 files changed, 62 insertions, 51 deletions

diff --git a/chromium/extensions/browser/extension_navigation_throttle.cc b/chromium/extensions/browser/extension_navigation_throttle.cc
index 880a1b8b25d..ef8c828700e 100644
--- a/chromium/extensions/browser/extension_navigation_throttle.cc
+++ b/chromium/extensions/browser/extension_navigation_throttle.cc
@@ -11,6 +11,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 #include "extensions/browser/url_request_util.h"
 #include "extensions/common/constants.h"
@@ -119,65 +120,64 @@ ExtensionNavigationThrottle::WillStartOrRedirectRequest() {
       if (!allowed)
         return content::NavigationThrottle::BLOCK_REQUEST;
     }
+  }
 
+  // Browser-initiated requests are always considered trusted, and thus allowed.
+  //
+  // Note that GuestView navigations initiated by the embedder also count as a
+  // browser-initiated navigation.
+  if (!navigation_handle()->IsRendererInitiated())
     return content::NavigationThrottle::PROCEED;
-  }
 
-  // This is a subframe navigation to a |target_extension| resource.
-  // Enforce the web_accessible_resources restriction, and same-origin
-  // restrictions for platform apps.
-  content::RenderFrameHost* parent = navigation_handle()->GetParentFrame();
-
-  // Look to see if all ancestors belong to |target_extension|. If not,
-  // then the web_accessible_resource restriction applies.
-  bool external_ancestor = false;
-  for (auto* ancestor = parent; ancestor; ancestor = ancestor->GetParent()) {
-    // Look for a match on the last committed origin. This handles the
-    // common case, and the about:blank case.
-    if (ancestor->GetLastCommittedOrigin() == target_origin)
-      continue;
-    // Look for an origin match with the last committed URL. This handles the
-    // case of sandboxed extension resources, which commit with a null origin,
-    // but are permitted to load non-webaccessible extension resources in
-    // subframes.
-    if (url::Origin::Create(ancestor->GetLastCommittedURL()) == target_origin)
-      continue;
-    // Ignore DevTools, as it is allowed to embed extension pages.
-    if (ancestor->GetLastCommittedURL().SchemeIs(
-            content::kChromeDevToolsScheme))
-      continue;
-
-    // Otherwise, we have an external ancestor.
-    external_ancestor = true;
-    break;
+  // All renderer-initiated navigations must have an initiator.
+  DCHECK(navigation_handle()->GetInitiatorOrigin().has_value());
+  const url::Origin& initiator_origin =
+      navigation_handle()->GetInitiatorOrigin().value();
+
+  // TODO(lukasza): https://crbug.com/1003957: Get rid of duplication with
+  // ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
+
+  // Navigations from chrome://, devtools:// or chrome-search:// pages need to
+  // be allowed, even if the target |url| is not web-accessible.  See
+  // https://crbug.com/662602.
+  if (initiator_origin.scheme() == content::kChromeUIScheme ||
+      initiator_origin.scheme() == content::kChromeDevToolsScheme ||
+      ExtensionsBrowserClient::Get()->ShouldSchemeBypassNavigationChecks(
+          initiator_origin.scheme())) {
+    return content::NavigationThrottle::PROCEED;
   }
 
-  if (external_ancestor) {
-    // Cancel navigations to nested URLs, to match the main frame behavior.
-    if (!url_has_extension_scheme)
-      return content::NavigationThrottle::CANCEL;
-
-    // |url| must be in the manifest's "web_accessible_resources" section.
-    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(target_extension,
-                                                             url.path()))
-      return content::NavigationThrottle::BLOCK_REQUEST;
+  // An extension can initiate navigations to any of its resources.
+  if (initiator_origin == target_origin)
+    return content::NavigationThrottle::PROCEED;
 
-    // A platform app may not be loaded in an <iframe> by another origin.
-    //
-    // In fact, platform apps may not have any cross-origin iframes at all; for
-    // non-extension origins of |url| this is enforced by means of a Content
-    // Security Policy. But CSP is incapable of blocking the chrome-extension
-    // scheme. Thus, this case must be handled specially here.
-    if (target_extension->is_platform_app())
-      return content::NavigationThrottle::CANCEL;
+  // Cancel cross-origin-initiator navigations to blob: or filesystem: URLs.
+  if (!url_has_extension_scheme)
+    return content::NavigationThrottle::CANCEL;
 
-    // A platform app may not load another extension in an <iframe>.
-    const Extension* parent_extension =
-        registry->enabled_extensions().GetExtensionOrAppByURL(
-            parent->GetSiteInstance()->GetSiteURL());
-    if (parent_extension && parent_extension->is_platform_app())
-      return content::NavigationThrottle::BLOCK_REQUEST;
+  // Cross-origin-initiator navigations require that the |url| is in the
+  // manifest's "web_accessible_resources" section.
+  if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(target_extension,
+                                                           url.path())) {
+    return content::NavigationThrottle::BLOCK_REQUEST;
   }
+  // A platform app may not be loaded in an <iframe> by another origin.
+  //
+  // In fact, platform apps may not have any cross-origin iframes at all;
+  // for non-extension origins of |url| this is enforced by means of a
+  // Content Security Policy. But CSP is incapable of blocking the
+  // chrome-extension scheme. Thus, this case must be handled specially
+  // here.
+  if (target_extension->is_platform_app())
+    return content::NavigationThrottle::CANCEL;
+
+  // A platform app may not load another extension in an <iframe>.
+  const Extension* initiator_extension =
+      registry->enabled_extensions().GetExtensionOrAppByURL(
+          initiator_origin.GetURL());
+  if (initiator_extension && initiator_extension->is_platform_app())
+    return content::NavigationThrottle::BLOCK_REQUEST;
+
 
   return content::NavigationThrottle::PROCEED;
 }
diff --git a/chromium/extensions/browser/extensions_browser_client.cc b/chromium/extensions/browser/extensions_browser_client.cc
index 1edccf4db60..7ecd9e2d94d 100644
--- a/chromium/extensions/browser/extensions_browser_client.cc
+++ b/chromium/extensions/browser/extensions_browser_client.cc
@@ -106,4 +106,9 @@ std::string ExtensionsBrowserClient::GetUserAgent() const {
   return std::string();
 }
 
+bool ExtensionsBrowserClient::ShouldSchemeBypassNavigationChecks(
+    const std::string& scheme) const {
+  return false;
+}
+
 }  // namespace extensions
diff --git a/chromium/extensions/browser/extensions_browser_client.h b/chromium/extensions/browser/extensions_browser_client.h
index 3ed389242f6..ed469a6a1b6 100644
--- a/chromium/extensions/browser/extensions_browser_client.h
+++ b/chromium/extensions/browser/extensions_browser_client.h
@@ -360,6 +360,12 @@ class ExtensionsBrowserClient {
   // Returns the user agent used by the content module.
   virtual std::string GetUserAgent() const;
 
+  // Returns whether |scheme| should bypass extension-specific navigation checks
+  // (e.g. whether the |scheme| is allowed to initiate navigations to extension
+  // resources that are not declared as web accessible).
+  virtual bool ShouldSchemeBypassNavigationChecks(
+      const std::string& scheme) const;
+
  private:
   std::vector<std::unique_ptr<ExtensionsBrowserAPIProvider>> providers_;