[Backport] Fix for security issue 925035

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 <wanderview@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709036}
Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>

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<GURL> GetSecondaryKeyForCodeCache(const GURL& resource_url,
 CodeCacheHostImpl::CodeCacheHostImpl(
     int render_process_id,
     scoped_refptr<CacheStorageContextImpl> cache_storage_context,
-    scoped_refptr<GeneratedCodeCacheContext> generated_code_cache_context)
+    scoped_refptr<GeneratedCodeCacheContext> 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<CacheStorageContextImpl> cache_storage_context,
-    scoped_refptr<GeneratedCodeCacheContext> generated_code_cache_context,
-    blink::mojom::CodeCacheHostRequest request) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  mojo::MakeStrongBinding(
-      std::make_unique<CodeCacheHostImpl>(
-          render_process_id, std::move(cache_storage_context),
-          std::move(generated_code_cache_context)),
-      std::move(request));
+void CodeCacheHostImpl::SetCacheStorageContextForTesting(
+    scoped_refptr<CacheStorageContextImpl> 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
@@ -44,14 +44,12 @@ class CONTENT_EXPORT CodeCacheHostImpl : public blink::mojom::CodeCacheHost {
   CodeCacheHostImpl(
       int render_process_id,
       scoped_refptr<CacheStorageContextImpl> cache_storage_context,
-      scoped_refptr<GeneratedCodeCacheContext> generated_code_cache_context);
-  ~CodeCacheHostImpl() override;
-
-  static void Create(
-      int render_process_id,
-      scoped_refptr<CacheStorageContextImpl> cache_storage_context,
       scoped_refptr<GeneratedCodeCacheContext> generated_code_cache_context,
       blink::mojom::CodeCacheHostRequest request);
+  ~CodeCacheHostImpl() override;
+
+  void SetCacheStorageContextForTesting(
+      scoped_refptr<CacheStorageContextImpl> context);
 
  private:
   // blink::mojom::CodeCacheHost implementation.
@@ -91,6 +89,8 @@ class CONTENT_EXPORT CodeCacheHostImpl : public blink::mojom::CodeCacheHost {
 
   scoped_refptr<GeneratedCodeCacheContext> generated_code_cache_context_;
 
+  mojo::Binding<blink::mojom::CodeCacheHost> binding_;
+
   base::WeakPtrFactory<CodeCacheHostImpl> 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<RenderProcessHostImpl::CodeCacheHostReceiverHandler>
+      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<CodeCacheHostImpl>(
+      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<void(RenderProcessHost*, CodeCacheHostImpl*)>;
+  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<IndexedDBDispatcherHost, base::OnTaskRunnerDeleter>
       indexed_db_factory_;
 
+  std::unique_ptr<CodeCacheHostImpl> 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<CacheStorageManager> 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<CacheStorageContextForBadOrigin>());
+
+    // Create an interceptor that passes a bad origin to CodeCacheHostImpl.
+    interceptors_.push_back(
+        std::make_unique<CodeCacheHostInterceptor>(rph, code_cache_host_impl));
+  }
+
+ private:
+  // Track the CodeCacheHostIntercptor objects so we can delete them in
+  // the test destructor.
+  std::vector<std::unique_ptr<CodeCacheHostInterceptor>> 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. -->
   </summary>
 </histogram>
 
+<histogram
+    name="ServiceWorkerCache.DidGenerateCacheableMetadataMessageInCacheStorage.OriginAllowed"
+    enum="Boolean" expires_after="M85">
+  <owner>wanderview@chromium.org</owner>
+  <owner>chrome-owp-storage@google.com</owner>
+  <summary>
+    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.
+  </summary>
+</histogram>
+
 <histogram name="ServiceWorkerCache.DiskCacheCreateEntryResult"
     enum="NetErrorCodes" expires_after="2019-12-31">
   <owner>wanderview@chromium.org</owner>