From c0a7f0136462ce6b2c74fb1a0b3b62d94ed7c83d Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Tue, 14 Jan 2020 16:33:38 +0100 Subject: [Backport] CVE-2019-13754: Insufficient policy enforcement in extensions. (1/3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Change-Id: I861344f323ebbe328770a49e95102b7f9d1300b1 Reviewed-by: Michal Klocek --- .../browser/extension_navigation_throttle.cc | 102 ++++++++++----------- .../browser/extensions_browser_client.cc | 5 + .../extensions/browser/extensions_browser_client.h | 6 ++ 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