From 11f1bad3655132011ad5d74e5c92f1b03466ea48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Br=C3=BCning?= Date: Tue, 14 Jan 2020 17:31:27 +0100 Subject: [Backport] CVE-2019-13754: Insufficient policy enforcement in extensions (2/3) Manual backport of patch: Remove ContentBrowserClient::IsSafeRedirectTarget. The deleted ChromeContentBrowserClient::IsSafeRedirectTarget was mostly redundant wrt ExtensionNavigationThrottle ("mostly", because the throttle considers only the navigation initiator - doesn't consider origins of intermediate hops as initiators; this aspect of behavior is not important to preserve). Bug: 442579 Change-Id: I7a8e36b6152788ca63507f09ba368ae75074e0cf Reviewed-by: Michal Klocek --- .../browser/loader/navigation_url_loader_impl.cc | 20 +++----------------- .../content/public/browser/content_browser_client.cc | 10 ---------- .../content/public/browser/content_browser_client.h | 7 ------- .../browser/extension_navigation_throttle.cc | 10 +++------- 4 files changed, 6 insertions(+), 41 deletions(-) diff --git a/chromium/content/browser/loader/navigation_url_loader_impl.cc b/chromium/content/browser/loader/navigation_url_loader_impl.cc index 3d6c804d28e..1341eba3b01 100644 --- a/chromium/content/browser/loader/navigation_url_loader_impl.cc +++ b/chromium/content/browser/loader/navigation_url_loader_impl.cc @@ -4,7 +4,9 @@ #include "content/browser/loader/navigation_url_loader_impl.h" +#include #include +#include #include #include "base/bind.h" @@ -307,21 +309,6 @@ void UnknownSchemeCallback( handled_externally ? net::ERR_ABORTED : net::ERR_UNKNOWN_URL_SCHEME)); } -// Determines whether it is safe to redirect from |from_url| to |to_url|. -bool IsRedirectSafe(const GURL& from_url, - const GURL& to_url, - BrowserContext* browser_context, - ResourceContext* resource_context) { - if (NavigationURLLoaderImpl::IsNavigationLoaderOnUIEnabled()) { - return IsSafeRedirectTarget(from_url, to_url) && - GetContentClient()->browser()->IsSafeRedirectTarget(to_url, - browser_context); - } - return IsSafeRedirectTarget(from_url, to_url) && - GetContentClient()->browser()->IsSafeRedirectTargetOnIO( - to_url, resource_context); -} - // Runs |task| on the thread specified by |thread_id| if already on that thread, // otherwise posts a task to that thread. void RunOrPostTaskIfNecessary(const base::Location& from_here, @@ -1366,8 +1353,7 @@ class NavigationURLLoaderImpl::URLLoaderRequestController const network::ResourceResponseHead& head) override { if (base::FeatureList::IsEnabled(network::features::kNetworkService) && !bypass_redirect_checks_ && - !IsRedirectSafe(url_, redirect_info.new_url, browser_context_, - resource_context_)) { + !IsSafeRedirectTarget(url_, redirect_info.new_url)) { // Call CancelWithError instead of OnComplete so that if there is an // intercepting URLLoaderFactory (created through the embedder's // ContentBrowserClient::WillCreateURLLoaderFactory) it gets notified. diff --git a/chromium/content/public/browser/content_browser_client.cc b/chromium/content/public/browser/content_browser_client.cc index 78770a86401..742d3b7a50b 100644 --- a/chromium/content/public/browser/content_browser_client.cc +++ b/chromium/content/public/browser/content_browser_client.cc @@ -908,16 +908,6 @@ ContentBrowserClient::CreateWindowForPictureInPicture( return nullptr; } -bool ContentBrowserClient::IsSafeRedirectTargetOnIO(const GURL& url, - ResourceContext* context) { - return true; -} - -bool ContentBrowserClient::IsSafeRedirectTarget(const GURL& url, - BrowserContext* context) { - return true; -} - void ContentBrowserClient::RegisterRendererPreferenceWatcher( BrowserContext* browser_context, blink::mojom::RendererPreferenceWatcherPtr watcher) { diff --git a/chromium/content/public/browser/content_browser_client.h b/chromium/content/public/browser/content_browser_client.h index ec27c9eca33..0d1f322960b 100644 --- a/chromium/content/public/browser/content_browser_client.h +++ b/chromium/content/public/browser/content_browser_client.h @@ -1511,13 +1511,6 @@ class CONTENT_EXPORT ContentBrowserClient { virtual std::unique_ptr CreateWindowForPictureInPicture( PictureInPictureWindowController* controller); - // Returns true if it is safe to redirect to |url|, otherwise returns false. - // This is called on the IO thread. - virtual bool IsSafeRedirectTargetOnIO(const GURL& url, - ResourceContext* context); - // Same as above but called on UI thread. - virtual bool IsSafeRedirectTarget(const GURL& url, BrowserContext* context); - // Registers the watcher to observe updates in RendererPreferences. virtual void RegisterRendererPreferenceWatcher( BrowserContext* browser_context, diff --git a/chromium/extensions/browser/extension_navigation_throttle.cc b/chromium/extensions/browser/extension_navigation_throttle.cc index ef8c828700e..fc7c2d221cd 100644 --- a/chromium/extensions/browser/extension_navigation_throttle.cc +++ b/chromium/extensions/browser/extension_navigation_throttle.cc @@ -4,6 +4,8 @@ #include "extensions/browser/extension_navigation_throttle.h" +#include + #include "components/guest_view/browser/guest_view_base.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/navigation_handle.h" @@ -189,13 +191,7 @@ ExtensionNavigationThrottle::WillStartRequest() { content::NavigationThrottle::ThrottleCheckResult ExtensionNavigationThrottle::WillRedirectRequest() { - ThrottleCheckResult result = WillStartOrRedirectRequest(); - if (result.action() == BLOCK_REQUEST) { - // TODO(nick): https://crbug.com/695421 means that BLOCK_REQUEST does not - // work here. Once PlzNavigate is enabled 100%, just return |result|. - return CANCEL; - } - return result; + return WillStartOrRedirectRequest(); } const char* ExtensionNavigationThrottle::GetNameForLogging() { -- cgit v1.2.1