From 4af826b4d3512f93c6aaf891c9e4434da0f8a7f6 Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Tue, 10 Mar 2020 11:56:30 +0100 Subject: [Backport] Fix for security issue 925035 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hand merged from: CacheStorage: Ignore code cache for origins that do not match the renderer. This CL is inspired by the previously attempted crrev.com/c/1434754. That was reverted due to excessive renderer crashes. As an interim step this CL instead ignores code cache when the origin seems wrong. We also add a UMA to see how often its triggering in practice so we can start trying to isolate the unexpected circumstances. Bug: 925035 Change-Id: Iec4d0206ba5ed74950537d74a4ad180ee6b98905 Commit-Queue: Ben Kelly Reviewed-by: Matt Falkenhagen Reviewed-by: Nasko Oskov Reviewed-by: Robert Kaplow Cr-Commit-Position: refs/heads/master@{#709036} Reviewed-by: Jüri Valdmann --- .../browser/renderer_host/code_cache_host_impl.cc | 40 ++++-- .../browser/renderer_host/code_cache_host_impl.h | 12 +- .../renderer_host/render_process_host_impl.cc | 46 ++++++- .../renderer_host/render_process_host_impl.h | 14 ++ .../service_worker/service_worker_browsertest.cc | 142 +++++++++++++++++++++ chromium/tools/metrics/histograms/histograms.xml | 13 ++ 6 files changed, 242 insertions(+), 25 deletions(-) diff --git a/chromium/content/browser/renderer_host/code_cache_host_impl.cc b/chromium/content/browser/renderer_host/code_cache_host_impl.cc index 22d4b361eef..17ce0990234 100644 --- a/chromium/content/browser/renderer_host/code_cache_host_impl.cc +++ b/chromium/content/browser/renderer_host/code_cache_host_impl.cc @@ -6,6 +6,7 @@ #include "base/bind.h" #include "base/bind_helpers.h" +#include "base/metrics/histogram_functions.h" #include "base/task/post_task.h" #include "base/threading/thread.h" #include "build/build_config.h" @@ -98,27 +99,20 @@ base::Optional GetSecondaryKeyForCodeCache(const GURL& resource_url, CodeCacheHostImpl::CodeCacheHostImpl( int render_process_id, scoped_refptr cache_storage_context, - scoped_refptr generated_code_cache_context) + scoped_refptr generated_code_cache_context, + blink::mojom::CodeCacheHostRequest request) : render_process_id_(render_process_id), cache_storage_context_(std::move(cache_storage_context)), - generated_code_cache_context_(std::move(generated_code_cache_context)) {} + generated_code_cache_context_(std::move(generated_code_cache_context)), + binding_(this, std::move(request)) {} CodeCacheHostImpl::~CodeCacheHostImpl() { DCHECK_CURRENTLY_ON(BrowserThread::UI); } -// static -void CodeCacheHostImpl::Create( - int render_process_id, - scoped_refptr cache_storage_context, - scoped_refptr generated_code_cache_context, - blink::mojom::CodeCacheHostRequest request) { - DCHECK_CURRENTLY_ON(BrowserThread::IO); - mojo::MakeStrongBinding( - std::make_unique( - render_process_id, std::move(cache_storage_context), - std::move(generated_code_cache_context)), - std::move(request)); +void CodeCacheHostImpl::SetCacheStorageContextForTesting( + scoped_refptr context) { + cache_storage_context_ = std::move(context); } void CodeCacheHostImpl::DidGenerateCacheableMetadata( @@ -188,6 +182,24 @@ void CodeCacheHostImpl::DidGenerateCacheableMetadataInCacheStorage( mojo_base::BigBuffer data, const url::Origin& cache_storage_origin, const std::string& cache_storage_cache_name) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + + // We cannot trust the renderer to give us the correct origin here. Validate + // it against the ChildProcessSecurityPolicy. + bool origin_allowed = + ChildProcessSecurityPolicyImpl::GetInstance()->CanAccessDataForOrigin( + render_process_id_, cache_storage_origin); + base::UmaHistogramBoolean( + "ServiceWorkerCache.DidGenerateCacheableMetadataMessageInCacheStorage." + "OriginAllowed", + origin_allowed); + if (!origin_allowed) { + // TODO(crbug/925035): Report a bad mojo message here. Currently we just + // null-route the request since this condition triggers more frequently + // than we expect. + return; + } + int64_t trace_id = blink::cache_storage::CreateTraceId(); TRACE_EVENT_WITH_FLOW1( "CacheStorage", diff --git a/chromium/content/browser/renderer_host/code_cache_host_impl.h b/chromium/content/browser/renderer_host/code_cache_host_impl.h index 1f4a7f5401e..30b05af8512 100644 --- a/chromium/content/browser/renderer_host/code_cache_host_impl.h +++ b/chromium/content/browser/renderer_host/code_cache_host_impl.h @@ -42,16 +42,14 @@ class GeneratedCodeCacheContext; class CONTENT_EXPORT CodeCacheHostImpl : public blink::mojom::CodeCacheHost { public: CodeCacheHostImpl( - int render_process_id, - scoped_refptr cache_storage_context, - scoped_refptr generated_code_cache_context); - ~CodeCacheHostImpl() override; - - static void Create( int render_process_id, scoped_refptr cache_storage_context, scoped_refptr generated_code_cache_context, blink::mojom::CodeCacheHostRequest request); + ~CodeCacheHostImpl() override; + + void SetCacheStorageContextForTesting( + scoped_refptr context); private: // blink::mojom::CodeCacheHost implementation. @@ -91,6 +89,8 @@ class CONTENT_EXPORT CodeCacheHostImpl : public blink::mojom::CodeCacheHost { scoped_refptr generated_code_cache_context_; + mojo::Binding binding_; + base::WeakPtrFactory weak_ptr_factory_{this}; DISALLOW_COPY_AND_ASSIGN(CodeCacheHostImpl); diff --git a/chromium/content/browser/renderer_host/render_process_host_impl.cc b/chromium/content/browser/renderer_host/render_process_host_impl.cc index 8dc869f6971..368ade16b1a 100644 --- a/chromium/content/browser/renderer_host/render_process_host_impl.cc +++ b/chromium/content/browser/renderer_host/render_process_host_impl.cc @@ -1166,6 +1166,13 @@ void RemoveCorbExceptionForPluginOnIOThread(int process_id) { network::CrossOriginReadBlocking::RemoveExceptionForPlugin(process_id); } +RenderProcessHostImpl::CodeCacheHostReceiverHandler& +GetCodeCacheHostReceiverHandler() { + static base::NoDestructor + instance; + return *instance; +} + // This is the entry point (i.e. this is called on the UI thread *before* // we post a task for RemoveCorbExceptionForPluginOnIOThread). void RemoveCorbExceptionForPluginOnUIThread(int process_id) { @@ -1609,6 +1616,11 @@ void RenderProcessHostImpl::SetBroadcastChannelProviderRequestHandlerForTesting( GetBroadcastChannelProviderRequestHandler() = handler; } +void RenderProcessHostImpl::SetCodeCacheHostReceiverHandlerForTesting( + CodeCacheHostReceiverHandler handler) { + GetCodeCacheHostReceiverHandler() = handler; +} + RenderProcessHostImpl::~RenderProcessHostImpl() { DCHECK_CURRENTLY_ON(BrowserThread::UI); #ifndef NDEBUG @@ -2161,11 +2173,8 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() { AddUIThreadInterface( registry.get(), - base::BindRepeating( - &CodeCacheHostImpl::Create, GetID(), - base::RetainedRef(storage_partition_impl_->GetCacheStorageContext()), - base::RetainedRef( - storage_partition_impl_->GetGeneratedCodeCacheContext()))); + base::BindRepeating(&RenderProcessHostImpl::CreateCodeCacheHost, + base::Unretained(this))); #if BUILDFLAG(ENABLE_REPORTING) AddUIThreadInterface( @@ -2346,6 +2355,27 @@ void RenderProcessHostImpl::CreateBroadcastChannelProvider( id_, std::move(request)); } +void RenderProcessHostImpl::CreateCodeCacheHost( + blink::mojom::CodeCacheHostRequest request) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + + // There should be at most one CodeCacheHostImpl for any given + // RenderProcessHost. + DCHECK(!code_cache_host_impl_); + + // Create a new CodeCacheHostImpl and bind it to the given receiver. + code_cache_host_impl_ = std::make_unique( + GetID(), storage_partition_impl_->GetCacheStorageContext(), + storage_partition_impl_->GetGeneratedCodeCacheContext(), + std::move(request)); + + // If there is a callback registered, then invoke it with the newly + // created CodeCacheHostImpl. + if (!GetCodeCacheHostReceiverHandler().is_null()) { + GetCodeCacheHostReceiverHandler().Run(this, code_cache_host_impl_.get()); + } +} + void RenderProcessHostImpl::BindVideoDecoderService( media::mojom::InterfaceFactoryRequest request) { if (!video_decoder_proxy_) @@ -4234,6 +4264,12 @@ void RenderProcessHostImpl::ResetIPC() { // for that. frame_sink_provider_.Unbind(); + // If RenderProcessHostImpl is reused, the next renderer will send a new + // request for CodeCacheHost. Make sure that we clear the stale + // object so that we can clearly create the new CodeCacheHostImpl while + // asserting we don't have any duplicates. + code_cache_host_impl_.reset(); + // It's important not to wait for the DeleteTask to delete the channel // proxy. Kill it off now. That way, in case the profile is going away, the // rest of the objects attached to this RenderProcessHost start going diff --git a/chromium/content/browser/renderer_host/render_process_host_impl.h b/chromium/content/browser/renderer_host/render_process_host_impl.h index 59df1d3b56e..7f6417dceda 100644 --- a/chromium/content/browser/renderer_host/render_process_host_impl.h +++ b/chromium/content/browser/renderer_host/render_process_host_impl.h @@ -67,6 +67,7 @@ #include "third_party/blink/public/mojom/dom_storage/storage_partition_service.mojom.h" #include "third_party/blink/public/mojom/filesystem/file_system.mojom.h" #include "third_party/blink/public/mojom/indexeddb/indexeddb.mojom-shared.h" +#include "third_party/blink/public/mojom/loader/code_cache.mojom.h" #include "third_party/blink/public/mojom/mediastream/media_stream.mojom.h" #include "third_party/blink/public/mojom/webdatabase/web_database.mojom.h" #include "ui/gfx/gpu_memory_buffer.h" @@ -87,6 +88,7 @@ class GpuClient; namespace content { class BrowserPluginMessageFilter; class ChildConnection; +class CodeCacheHostImpl; class FileSystemManagerImpl; class IndexedDBDispatcherHost; class InProcessChildThreadParams; @@ -399,6 +401,14 @@ class CONTENT_EXPORT RenderProcessHostImpl static void SetBroadcastChannelProviderRequestHandlerForTesting( BroadcastChannelProviderRequestHandler handler); + // Allows external code to supply a callback that is invoked immediately + // after the CodeCacheHostImpl is created and bound. Used for swapping + // the binding for a test version of the service. + using CodeCacheHostReceiverHandler = + base::RepeatingCallback; + static void SetCodeCacheHostReceiverHandlerForTesting( + CodeCacheHostReceiverHandler handler); + RenderFrameMessageFilter* render_frame_message_filter_for_testing() const { return render_frame_message_filter_.get(); } @@ -572,6 +582,8 @@ class CONTENT_EXPORT RenderProcessHostImpl blink::mojom::StoragePartitionServiceRequest request); void CreateBroadcastChannelProvider( blink::mojom::BroadcastChannelProviderRequest request); + void CreateCodeCacheHost( + blink::mojom::CodeCacheHostRequest request); void CreateRendererHost(mojom::RendererHostAssociatedRequest request); void BindVideoDecoderService(media::mojom::InterfaceFactoryRequest request); void BindWebDatabaseHostImpl(blink::mojom::WebDatabaseHostRequest request); @@ -858,6 +870,8 @@ class CONTENT_EXPORT RenderProcessHostImpl std::unique_ptr indexed_db_factory_; + std::unique_ptr code_cache_host_impl_; + bool channel_connected_; bool sent_render_process_ready_; diff --git a/chromium/content/browser/service_worker/service_worker_browsertest.cc b/chromium/content/browser/service_worker/service_worker_browsertest.cc index 46b07e520cc..a02157147a9 100644 --- a/chromium/content/browser/service_worker/service_worker_browsertest.cc +++ b/chromium/content/browser/service_worker/service_worker_browsertest.cc @@ -36,7 +36,10 @@ #include "content/browser/cache_storage/cache_storage_cache_handle.h" #include "content/browser/cache_storage/cache_storage_context_impl.h" #include "content/browser/cache_storage/cache_storage_manager.h" +#include "content/browser/code_cache/generated_code_cache_context.h" #include "content/browser/loader/navigation_url_loader_impl.h" +#include "content/browser/renderer_host/code_cache_host_impl.h" +#include "content/browser/renderer_host/render_process_host_impl.h" #include "content/browser/service_worker/embedded_worker_instance.h" #include "content/browser/service_worker/embedded_worker_status.h" #include "content/browser/service_worker/service_worker_context_core.h" @@ -46,6 +49,7 @@ #include "content/browser/service_worker/service_worker_registration.h" #include "content/browser/service_worker/service_worker_test_utils.h" #include "content/browser/service_worker/service_worker_version.h" +#include "content/browser/storage_partition_impl.h" #include "content/browser/web_contents/web_contents_impl.h" #include "content/browser/web_package/signed_exchange_consts.h" #include "content/public/browser/browser_context.h" @@ -96,6 +100,7 @@ #include "third_party/blink/public/common/service_worker/service_worker_status_code.h" #include "third_party/blink/public/common/service_worker/service_worker_type_converters.h" #include "third_party/blink/public/common/service_worker/service_worker_utils.h" +#include "third_party/blink/public/mojom/loader/code_cache.mojom-test-utils.h" #include "third_party/blink/public/mojom/service_worker/service_worker_event_status.mojom.h" #include "third_party/blink/public/mojom/service_worker/service_worker_object.mojom.h" #include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom.h" @@ -3616,6 +3621,143 @@ IN_PROC_BROWSER_TEST_P(ServiceWorkerCodeCacheStrategyIdleTaskTest, EXPECT_EQ("SUCCESS", EvalJs(shell(), "execute_cached_twice_script();")); } +namespace { + +class CodeCacheHostInterceptor + : public blink::mojom::CodeCacheHostInterceptorForTesting, + public RenderProcessHostObserver { + public: + CodeCacheHostInterceptor(RenderProcessHost* rph, + CodeCacheHostImpl* code_cache_host_impl) + : render_process_host_(rph), code_cache_host_impl_(code_cache_host_impl) { + // Intercept messages for the CodeCacheHost. + code_cache_host_impl_->receiver().SwapImplForTesting(this); + + // Register with the RenderProcessHost so we can cleanup properly. + render_process_host_->AddObserver(this); + } + + ~CodeCacheHostInterceptor() override { + if (render_process_host_) + render_process_host_->RemoveObserver(this); + } + + CodeCacheHost* GetForwardingInterface() override { + return code_cache_host_impl_; + } + + void RenderProcessExited(RenderProcessHost* host, + const ChildProcessTerminationInfo& info) override { + DCHECK(host == render_process_host_); + + // The CodeCacheHostImpl will be destroyed when the renderer exits. + // Drop our reference to avoid holding a stale pointer. + code_cache_host_impl_ = nullptr; + + render_process_host_->RemoveObserver(this); + render_process_host_ = nullptr; + } + + void DidGenerateCacheableMetadataInCacheStorage( + const GURL& url, + base::Time expected_response_time, + mojo_base::BigBuffer data, + const url::Origin& cache_storage_origin, + const std::string& cache_storage_cache_name) override { + // Send the message with an overriden, bad origin. + GetForwardingInterface()->DidGenerateCacheableMetadataInCacheStorage( + url, expected_response_time, std::move(data), + url::Origin::Create(GURL("https://bad.com")), cache_storage_cache_name); + } + + private: + // These can be held as raw pointers since we use the + // RenderProcessHostObserver interface to clear them before they are + // destroyed. + RenderProcessHost* render_process_host_; + CodeCacheHostImpl* code_cache_host_impl_; +}; + +class CacheStorageContextForBadOrigin : public CacheStorageContextImpl { + public: + CacheStorageContextForBadOrigin() : CacheStorageContextImpl(nullptr) {} + + scoped_refptr CacheManager() override { + // The CodeCacheHostImpl should not try to access the CacheManager() + // if the origin is bad. + NOTREACHED(); + return nullptr; + } + + private: + ~CacheStorageContextForBadOrigin() override = default; +}; + +} // namespace + +// Test that forces a bad origin to be sent to CodeCacheHost's +// DidGenerateCacheableMetadataInCacheStorage method. +class ServiceWorkerV8CodeCacheForCacheStorageBadOriginTest + : public ServiceWorkerV8CodeCacheForCacheStorageTest { + public: + ServiceWorkerV8CodeCacheForCacheStorageBadOriginTest() { + // Register a callback to be notified of new CodeCacheHostImpl objects. + RenderProcessHostImpl::SetCodeCacheHostReceiverHandlerForTesting( + base::BindRepeating( + &ServiceWorkerV8CodeCacheForCacheStorageBadOriginTest:: + CreateTestCodeCacheHost, + base::Unretained(this))); + } + + void SetUpCommandLine(base::CommandLine* command_line) override { + ServiceWorkerV8CodeCacheForCacheStorageTest::SetUpCommandLine(command_line); + // The purpose of this test is to verify how CodeCacheHostImpl behaves + // when it receives an origin that is different from the site locked to the + // process. In order for this to work properly on platforms like android + // we must explicitly enable site isolation. + IsolateAllSitesForTesting(command_line); + } + + ~ServiceWorkerV8CodeCacheForCacheStorageBadOriginTest() override { + // Disable the callback now that this object is being destroyed. + RenderProcessHostImpl::SetCodeCacheHostReceiverHandlerForTesting( + RenderProcessHostImpl::CodeCacheHostReceiverHandler()); + } + + void CreateTestCodeCacheHost(RenderProcessHost* rph, + CodeCacheHostImpl* code_cache_host_impl) { + // Override the cache_storage context to assert that CodeCacheHostImpl + // does not try to access it when given a bad origin. + code_cache_host_impl->SetCacheStorageContextForTesting( + base::MakeRefCounted()); + + // Create an interceptor that passes a bad origin to CodeCacheHostImpl. + interceptors_.push_back( + std::make_unique(rph, code_cache_host_impl)); + } + + private: + // Track the CodeCacheHostIntercptor objects so we can delete them in + // the test destructor. + std::vector> interceptors_; +}; + +IN_PROC_BROWSER_TEST_F(ServiceWorkerV8CodeCacheForCacheStorageBadOriginTest, + V8CacheOnCacheStorage) { + RegisterAndActivateServiceWorker(); + + // First load: fetch_event_response_via_cache.js returns |cloned_response|. + // The V8 code cache should not be stored in CacheStorage. + NavigateToTestPage(); + WaitUntilSideDataSizeIs(0); + + // Second load: The V8 code cache should still be zero. When a bad origin + // is received by CodeCacheHost it should ignore the provided metadata. + // TODO(crbug/925035): In the future this should instead kill the renderer. + NavigateToTestPage(); + WaitUntilSideDataSizeIs(0); +} + // ServiceWorkerDisableWebSecurityTests check the behavior when the web security // is disabled. If '--disable-web-security' flag is set, we don't check the // origin equality in Blink. So the Service Worker related APIs should succeed diff --git a/chromium/tools/metrics/histograms/histograms.xml b/chromium/tools/metrics/histograms/histograms.xml index 3f9fbedcdf3..f1057568934 100644 --- a/chromium/tools/metrics/histograms/histograms.xml +++ b/chromium/tools/metrics/histograms/histograms.xml @@ -123535,6 +123535,19 @@ should be kept until we remove incident reporting. --> + + wanderview@chromium.org + chrome-owp-storage@google.com + + Whether the origin provided in the + DidGenerateCacheableMetadataInCacheStorage message is allowed for the given + renderer process. This message is sent when the renderer has generated code + cache for a script and wants to store it in cache_storage. + + + wanderview@chromium.org -- cgit v1.2.1