From cd2c0564bf8c8830be407d1d674e12d8290ff734 Mon Sep 17 00:00:00 2001 From: Fabrice de Gans-Riberi Date: Thu, 8 Apr 2021 20:00:27 +0000 Subject: [PATCH] [fuchsia] Fix a use-after-free in WebEngineURLLoaderThrottle It was assumed that all WebEngineURLLoaderThrottles would be destroyed before their corresponding RenderFrame was destroyed. This assumption is incorrect, resulting in a potential use-after-free when attempting to access a UrlRequestRulesReceiver. This object has its lifespan bound to a RenderFrameObserver and can be destroyed before all of the WebEngineURLLoaderThrottles that use it have been destroyed. This CL fixes the issue by passing the UrlRequestRewriteRules directly to the WebEngineURLLoaderThrottle constructor. As a result, URL requests in-flight while a user updates the rewrite rules will use the older version of the rewrite rules. We deem this change in behavior to be acceptable since such an occurrence is bound to be rare and the delay between a WebEngineURLLoaderThrottle creation and its actual use is bound to be short. This CL also modifies the logic a bit to skip creation of the throttle if there are no rewrite rules to apply at the time of the network request creation. Since the rules provider should now be accessed from the same thread in both the renderer and the browser process, the locks around accessing the rules have been removed. Instead, each throttle keeps a reference to a thread-safe ref-counted set of rules. Since this fix is still speculative, the extra logs for debugging this issue are left in place in this CL, until we get data confirming the crashes are no longer occurring. (cherry picked from commit 4c3854799005b3adf792fd762a8fec5fb3ce70cc) Bug: 1181062 Change-Id: I580664f9ed2fdf9874e36b1cbc508c2301ef36e5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2812464 Commit-Queue: Fabrice de Gans-Riberi Commit-Queue: Kevin Marshall Reviewed-by: Kevin Marshall Reviewed-by: Wez Cr-Original-Commit-Position: refs/heads/master@{#870653} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2815225 Reviewed-by: Fabrice de Gans-Riberi Commit-Queue: Srinivas Sista Cr-Commit-Position: refs/branch-heads/4471@{#2} Cr-Branched-From: f5ba97ea1045e2c36e355fc93042225d9c1eebdd-refs/heads/master@{#870382} --- .../url_request_rewrite_rules_manager.cc | 19 +++++--- .../url_request_rewrite_rules_manager.h | 15 +++---- .../web_engine_content_browser_client.cc | 8 +++- .../common/web_engine_url_loader_throttle.cc | 25 +++++------ .../common/web_engine_url_loader_throttle.h | 22 +++------ ...web_engine_url_loader_throttle_unittest.cc | 45 ++----------------- .../renderer/url_request_rules_receiver.cc | 13 +++--- .../renderer/url_request_rules_receiver.h | 34 +++++--------- ...web_engine_url_loader_throttle_provider.cc | 8 +++- 9 files changed, 67 insertions(+), 122 deletions(-) diff --git a/fuchsia/engine/browser/url_request_rewrite_rules_manager.cc b/fuchsia/engine/browser/url_request_rewrite_rules_manager.cc index 9166a7f805630..3e9277e8c620c 100644 --- a/fuchsia/engine/browser/url_request_rewrite_rules_manager.cc +++ b/fuchsia/engine/browser/url_request_rewrite_rules_manager.cc @@ -137,18 +137,20 @@ UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager( content::WebContents* web_contents) : content::WebContentsObserver(web_contents) {} -UrlRequestRewriteRulesManager::~UrlRequestRewriteRulesManager() = default; +UrlRequestRewriteRulesManager::~UrlRequestRewriteRulesManager() { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); +} zx_status_t UrlRequestRewriteRulesManager::OnRulesUpdated( std::vector rules, fuchsia::web::Frame::SetUrlRequestRewriteRulesCallback callback) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + if (!ValidateRules(rules)) { - base::AutoLock auto_lock(lock_); cached_rules_ = nullptr; return ZX_ERR_INVALID_ARGS; } - base::AutoLock auto_lock(lock_); cached_rules_ = base::MakeRefCounted( mojo::ConvertTo>( @@ -165,16 +167,18 @@ zx_status_t UrlRequestRewriteRulesManager::OnRulesUpdated( return ZX_OK; } -scoped_refptr +scoped_refptr& UrlRequestRewriteRulesManager::GetCachedRules() { - base::AutoLock auto_lock(lock_); + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); return cached_rules_; } -UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager() {} +UrlRequestRewriteRulesManager::UrlRequestRewriteRulesManager() = default; void UrlRequestRewriteRulesManager::RenderFrameCreated( content::RenderFrameHost* render_frame_host) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + // Register the frame rules receiver. mojo::AssociatedRemote rules_receiver; render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface( @@ -183,7 +187,6 @@ void UrlRequestRewriteRulesManager::RenderFrameCreated( render_frame_host->GetGlobalFrameRoutingId(), std::move(rules_receiver)); DCHECK(iter.second); - base::AutoLock auto_lock(lock_); if (cached_rules_) { // Send an initial set of rules. iter.first->second->OnRulesUpdated(mojo::Clone(cached_rules_->data)); @@ -192,6 +195,8 @@ void UrlRequestRewriteRulesManager::RenderFrameCreated( void UrlRequestRewriteRulesManager::RenderFrameDeleted( content::RenderFrameHost* render_frame_host) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + size_t removed = active_remotes_.erase(render_frame_host->GetGlobalFrameRoutingId()); DCHECK_EQ(removed, 1u); diff --git a/fuchsia/engine/browser/url_request_rewrite_rules_manager.h b/fuchsia/engine/browser/url_request_rewrite_rules_manager.h index d41503efda3cd..d2bcad2db8877 100644 --- a/fuchsia/engine/browser/url_request_rewrite_rules_manager.h +++ b/fuchsia/engine/browser/url_request_rewrite_rules_manager.h @@ -7,7 +7,7 @@ #include -#include "base/synchronization/lock.h" +#include "base/sequence_checker.h" #include "content/public/browser/global_routing_id.h" #include "content/public/browser/web_contents_observer.h" #include "fuchsia/engine/common/web_engine_url_loader_throttle.h" @@ -23,8 +23,7 @@ class WebContents; // Adapts the UrlRequestRewrite FIDL API to be sent to the renderers over the // over the UrlRequestRewrite Mojo API. class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager - : public content::WebContentsObserver, - public WebEngineURLLoaderThrottle::CachedRulesProvider { + : public content::WebContentsObserver { public: static std::unique_ptr CreateForTesting(); @@ -38,9 +37,8 @@ class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager std::vector rules, fuchsia::web::Frame::SetUrlRequestRewriteRulesCallback callback); - // WebEngineURLLoaderThrottle::CachedRulesProvider implementation. - scoped_refptr - GetCachedRules() override; + scoped_refptr& + GetCachedRules(); private: // Test-only constructor. @@ -50,15 +48,16 @@ class WEB_ENGINE_EXPORT UrlRequestRewriteRulesManager void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override; void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override; - base::Lock lock_; scoped_refptr - cached_rules_ GUARDED_BY(lock_); + cached_rules_; // Map of GlobalRoutingID to their current associated remote. std::map> active_remotes_; + SEQUENCE_CHECKER(sequence_checker_); + DISALLOW_COPY_AND_ASSIGN(UrlRequestRewriteRulesManager); }; diff --git a/fuchsia/engine/browser/web_engine_content_browser_client.cc b/fuchsia/engine/browser/web_engine_content_browser_client.cc index d902748cbf918..1be5181e60d2b 100644 --- a/fuchsia/engine/browser/web_engine_content_browser_client.cc +++ b/fuchsia/engine/browser/web_engine_content_browser_client.cc @@ -257,9 +257,13 @@ WebEngineContentBrowserClient::CreateURLLoaderThrottles( } std::vector> throttles; - throttles.emplace_back(std::make_unique( + scoped_refptr& rules = FrameImpl::FromWebContents(wc_getter.Run()) - ->url_request_rewrite_rules_manager())); + ->url_request_rewrite_rules_manager() + ->GetCachedRules(); + if (rules) { + throttles.emplace_back(std::make_unique(rules)); + } return throttles; } diff --git a/fuchsia/engine/common/web_engine_url_loader_throttle.cc b/fuchsia/engine/common/web_engine_url_loader_throttle.cc index 34a31a764104d..38d207c084e68 100644 --- a/fuchsia/engine/common/web_engine_url_loader_throttle.cc +++ b/fuchsia/engine/common/web_engine_url_loader_throttle.cc @@ -171,9 +171,9 @@ bool IsRequestAllowed( } // namespace WebEngineURLLoaderThrottle::WebEngineURLLoaderThrottle( - CachedRulesProvider* cached_rules_provider) - : cached_rules_provider_(cached_rules_provider) { - DCHECK(cached_rules_provider); + scoped_refptr rules) + : rules_(rules) { + DCHECK(rules_); } WebEngineURLLoaderThrottle::~WebEngineURLLoaderThrottle() = default; @@ -183,19 +183,14 @@ void WebEngineURLLoaderThrottle::DetachFromCurrentSequence() {} void WebEngineURLLoaderThrottle::WillStartRequest( network::ResourceRequest* request, bool* defer) { - scoped_refptr - cached_rules = cached_rules_provider_->GetCachedRules(); - // |cached_rules| may be empty if no rule was ever sent to WebEngine. - if (cached_rules) { - if (!IsRequestAllowed(request, *cached_rules)) { - delegate_->CancelWithError(net::ERR_ABORTED, - "Resource load blocked by embedder policy."); - return; - } - - for (const auto& rule : cached_rules->data) - ApplyRule(request, rule); + if (!IsRequestAllowed(request, *rules_)) { + delegate_->CancelWithError(net::ERR_ABORTED, + "Resource load blocked by embedder policy."); + return; } + + for (const auto& rule : rules_->data) + ApplyRule(request, rule); *defer = false; } diff --git a/fuchsia/engine/common/web_engine_url_loader_throttle.h b/fuchsia/engine/common/web_engine_url_loader_throttle.h index 2d3965aa88c79..070ec9c1188bd 100644 --- a/fuchsia/engine/common/web_engine_url_loader_throttle.h +++ b/fuchsia/engine/common/web_engine_url_loader_throttle.h @@ -7,7 +7,6 @@ #include -#include "base/containers/flat_set.h" #include "base/memory/ref_counted.h" #include "fuchsia/engine/url_request_rewrite.mojom.h" #include "fuchsia/engine/web_engine_export.h" @@ -22,21 +21,14 @@ class WEB_ENGINE_EXPORT WebEngineURLLoaderThrottle using UrlRequestRewriteRules = base::RefCountedData>; - // An interface to provide rewrite rules to the throttle. Its - // implementation must outlive the WebEngineURLLoaderThrottle. - class CachedRulesProvider { - public: - virtual ~CachedRulesProvider() = default; - - // Gets cached rules. This call can be made on any sequence, as - // URLLoaderThrottles are not guaranteed to stay on the same sequence. - virtual scoped_refptr GetCachedRules() = 0; - }; - explicit WebEngineURLLoaderThrottle( - CachedRulesProvider* cached_rules_provider); + scoped_refptr rules); ~WebEngineURLLoaderThrottle() override; + WebEngineURLLoaderThrottle(const WebEngineURLLoaderThrottle&) = delete; + WebEngineURLLoaderThrottle& operator=(const WebEngineURLLoaderThrottle&) = + delete; + // blink::URLLoaderThrottle implementation. void DetachFromCurrentSequence() override; void WillStartRequest(network::ResourceRequest* request, @@ -58,9 +50,7 @@ class WEB_ENGINE_EXPORT WebEngineURLLoaderThrottle network::ResourceRequest* request, const mojom::UrlRequestRewriteAddHeadersPtr& add_headers); - CachedRulesProvider* const cached_rules_provider_; - - DISALLOW_COPY_AND_ASSIGN(WebEngineURLLoaderThrottle); + scoped_refptr rules_; }; #endif // FUCHSIA_ENGINE_COMMON_WEB_ENGINE_URL_LOADER_THROTTLE_H_ diff --git a/fuchsia/engine/common/web_engine_url_loader_throttle_unittest.cc b/fuchsia/engine/common/web_engine_url_loader_throttle_unittest.cc index 5b5285e9e0a43..20bd012978ed9 100644 --- a/fuchsia/engine/common/web_engine_url_loader_throttle_unittest.cc +++ b/fuchsia/engine/common/web_engine_url_loader_throttle_unittest.cc @@ -22,31 +22,6 @@ constexpr char kMixedCaseCorsExemptHeader2[] = "Another-CoRs-ExEmPt-2"; constexpr char kUpperCaseCorsExemptHeader2[] = "ANOTHER-CORS-EXEMPT-2"; constexpr char kRequiresCorsHeader[] = "requires-cors"; -class TestCachedRulesProvider - : public WebEngineURLLoaderThrottle::CachedRulesProvider { - public: - TestCachedRulesProvider() = default; - ~TestCachedRulesProvider() override = default; - - void SetCachedRules( - scoped_refptr - cached_rules) { - cached_rules_ = cached_rules; - } - - // WebEngineURLLoaderThrottle::CachedRulesProvider implementation. - scoped_refptr - GetCachedRules() override { - return cached_rules_; - } - - private: - scoped_refptr - cached_rules_; - - DISALLOW_COPY_AND_ASSIGN(TestCachedRulesProvider); -}; - } // namespace class WebEngineURLLoaderThrottleTest : public testing::Test { @@ -77,12 +52,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, WildcardHosts) { std::vector rules; rules.push_back(std::move(rule)); - TestCachedRulesProvider provider; - provider.SetCachedRules( + WebEngineURLLoaderThrottle throttle( base::MakeRefCounted( std::move(rules))); - - WebEngineURLLoaderThrottle throttle(&provider); bool defer = false; network::ResourceRequest request1; @@ -134,13 +106,10 @@ TEST_F(WebEngineURLLoaderThrottleTest, CorsAwareHeaders) { std::vector rules; rules.push_back(std::move(rule)); - TestCachedRulesProvider provider; - provider.SetCachedRules( + WebEngineURLLoaderThrottle throttle( base::MakeRefCounted( std::move(rules))); - WebEngineURLLoaderThrottle throttle(&provider); - network::ResourceRequest request; request.url = GURL("http://test.net"); bool defer = false; @@ -181,12 +150,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, DataReplacementUrl) { std::vector rules; rules.push_back(std::move(rule)); - TestCachedRulesProvider provider; - provider.SetCachedRules( + WebEngineURLLoaderThrottle throttle( base::MakeRefCounted( std::move(rules))); - - WebEngineURLLoaderThrottle throttle(&provider); bool defer = false; network::ResourceRequest request; @@ -240,12 +206,9 @@ TEST_F(WebEngineURLLoaderThrottleTest, AllowAndDeny) { rules.push_back(std::move(rule)); } - TestCachedRulesProvider provider; - provider.SetCachedRules( + WebEngineURLLoaderThrottle throttle( base::MakeRefCounted( std::move(rules))); - - WebEngineURLLoaderThrottle throttle(&provider); bool defer = false; TestThrottleDelegate delegate; diff --git a/fuchsia/engine/renderer/url_request_rules_receiver.cc b/fuchsia/engine/renderer/url_request_rules_receiver.cc index c1da8c7bb9931..c82e726e2eaca 100644 --- a/fuchsia/engine/renderer/url_request_rules_receiver.cc +++ b/fuchsia/engine/renderer/url_request_rules_receiver.cc @@ -28,6 +28,12 @@ UrlRequestRulesReceiver::~UrlRequestRulesReceiver() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); } +scoped_refptr& +UrlRequestRulesReceiver::GetCachedRules() { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + return cached_rules_; +} + void UrlRequestRulesReceiver::OnUrlRequestRulesReceiverAssociatedReceiver( mojo::PendingAssociatedReceiver receiver) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); @@ -38,14 +44,7 @@ void UrlRequestRulesReceiver::OnUrlRequestRulesReceiverAssociatedReceiver( void UrlRequestRulesReceiver::OnRulesUpdated( std::vector rules) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - base::AutoLock auto_lock(lock_); cached_rules_ = base::MakeRefCounted( std::move(rules)); } - -scoped_refptr -UrlRequestRulesReceiver::GetCachedRules() { - base::AutoLock auto_lock(lock_); - return cached_rules_; -} diff --git a/fuchsia/engine/renderer/url_request_rules_receiver.h b/fuchsia/engine/renderer/url_request_rules_receiver.h index 98125825f3835..10cd16e0a1068 100644 --- a/fuchsia/engine/renderer/url_request_rules_receiver.h +++ b/fuchsia/engine/renderer/url_request_rules_receiver.h @@ -6,9 +6,6 @@ #define FUCHSIA_ENGINE_RENDERER_URL_REQUEST_RULES_RECEIVER_H_ #include "base/sequence_checker.h" -#include "base/synchronization/lock.h" -#include "base/thread_annotations.h" -#include "content/public/renderer/render_frame_observer.h" #include "fuchsia/engine/common/web_engine_url_loader_throttle.h" #include "fuchsia/engine/url_request_rewrite.mojom.h" #include "mojo/public/cpp/bindings/associated_receiver.h" @@ -21,18 +18,18 @@ class RenderFrame; // Provides rewriting rules for network requests. Owned by // WebEngineRenderFrameObserver, this object will be destroyed on RenderFrame -// destruction. This is guaranteed to outlive any WebEngineURLLoaderThrottle -// that uses it as the RenderFrame destruction will have triggered the -// destruction of all pending WebEngineURLLoaderThrottles. -// This class should only be used on the IO thread, with the exception of the -// GetCachedRules() implementation, which can be called from any sequence. -class UrlRequestRulesReceiver - : public mojom::UrlRequestRulesReceiver, - public WebEngineURLLoaderThrottle::CachedRulesProvider { +// destruction. This class should only be used on the IO thread. +class UrlRequestRulesReceiver : public mojom::UrlRequestRulesReceiver { public: - UrlRequestRulesReceiver(content::RenderFrame* render_frame); + explicit UrlRequestRulesReceiver(content::RenderFrame* render_frame); ~UrlRequestRulesReceiver() override; + UrlRequestRulesReceiver(const UrlRequestRulesReceiver&) = delete; + UrlRequestRulesReceiver& operator=(const UrlRequestRulesReceiver&) = delete; + + scoped_refptr& + GetCachedRules(); + private: void OnUrlRequestRulesReceiverAssociatedReceiver( mojo::PendingAssociatedReceiver receiver); @@ -40,23 +37,12 @@ class UrlRequestRulesReceiver // mojom::UrlRequestRulesReceiver implementation. void OnRulesUpdated(std::vector rules) override; - // WebEngineURLLoaderThrottle::CachedRulesProvider implementation. scoped_refptr - GetCachedRules() override; - - base::Lock lock_; - - // This is accessed by WebEngineURLLoaderThrottles, which can be off-sequence - // in the case of synchronous network requests. - scoped_refptr - cached_rules_ GUARDED_BY(lock_); - + cached_rules_; mojo::AssociatedReceiver url_request_rules_receiver_{this}; SEQUENCE_CHECKER(sequence_checker_); - - DISALLOW_COPY_AND_ASSIGN(UrlRequestRulesReceiver); }; #endif // FUCHSIA_ENGINE_RENDERER_URL_REQUEST_RULES_RECEIVER_H_ diff --git a/fuchsia/engine/renderer/web_engine_url_loader_throttle_provider.cc b/fuchsia/engine/renderer/web_engine_url_loader_throttle_provider.cc index eaf0ae2033229..7fda178d227c3 100644 --- a/fuchsia/engine/renderer/web_engine_url_loader_throttle_provider.cc +++ b/fuchsia/engine/renderer/web_engine_url_loader_throttle_provider.cc @@ -36,10 +36,14 @@ WebEngineURLLoaderThrottleProvider::CreateThrottles( CHECK_NE(render_frame_id, MSG_ROUTING_NONE); blink::WebVector> throttles; - throttles.emplace_back(std::make_unique( + scoped_refptr& rules = content_renderer_client_ ->GetWebEngineRenderFrameObserverForRenderFrameId(render_frame_id) - ->url_request_rules_receiver())); + ->url_request_rules_receiver() + ->GetCachedRules(); + if (rules) { + throttles.emplace_back(std::make_unique(rules)); + } return throttles; }