Fix UAF in CachedStorageArea

CachedStorageArea is ref-counted and was retaining an unsafe reference
to a GCed object which it may outlive.

This changes that reference to a WeakPersistent and ensures that we
don't dereference it once it's no longer valid.

Fixed: 1048234
Change-Id: I9971b35b9aa7db790236f92c353a2320740f7729
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036383
Commit-Queue: Ken Rockot <rockot@google.com>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Auto-Submit: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#738424}

third_party/blink/renderer/modules/storage/cached_storage_area.cc

@@ -65,9 +65,10 @@ scoped_refptr<CachedStorageArea> CachedStorageArea::CreateForLocalStorage(
     scoped_refptr<const SecurityOrigin> origin,
     mojo::PendingRemote<mojom::blink::StorageArea> area,
     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-    InspectorEventListener* listener) {
-  return base::AdoptRef(new CachedStorageArea(
-      std::move(origin), std::move(area), std::move(ipc_runner), listener));
+    StorageNamespace* storage_namespace) {
+  return base::AdoptRef(
+      new CachedStorageArea(std::move(origin), std::move(area),
+                            std::move(ipc_runner), storage_namespace));
 }
 
 // static
@@ -75,9 +76,10 @@ scoped_refptr<CachedStorageArea> CachedStorageArea::CreateForSessionStorage(
     scoped_refptr<const SecurityOrigin> origin,
     mojo::PendingAssociatedRemote<mojom::blink::StorageArea> area,
     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-    InspectorEventListener* listener) {
-  return base::AdoptRef(new CachedStorageArea(
-      std::move(origin), std::move(area), std::move(ipc_runner), listener));
+    StorageNamespace* storage_namespace) {
+  return base::AdoptRef(
+      new CachedStorageArea(std::move(origin), std::move(area),
+                            std::move(ipc_runner), storage_namespace));
 }
 
 unsigned CachedStorageArea::GetLength() {
@@ -199,9 +201,9 @@ CachedStorageArea::CachedStorageArea(
     scoped_refptr<const SecurityOrigin> origin,
     mojo::PendingRemote<mojom::blink::StorageArea> area,
     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-    InspectorEventListener* listener)
+    StorageNamespace* storage_namespace)
     : origin_(std::move(origin)),
-      inspector_event_listener_(listener),
+      storage_namespace_(storage_namespace),
       ipc_task_runner_(std::move(ipc_runner)),
       mojo_area_remote_(std::move(area), ipc_task_runner_),
       mojo_area_(mojo_area_remote_.get()),
@@ -217,9 +219,9 @@ CachedStorageArea::CachedStorageArea(
     scoped_refptr<const SecurityOrigin> origin,
     mojo::PendingAssociatedRemote<mojom::blink::StorageArea> area,
     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-    InspectorEventListener* listener)
+    StorageNamespace* storage_namespace)
     : origin_(std::move(origin)),
-      inspector_event_listener_(listener),
+      storage_namespace_(storage_namespace),
       ipc_task_runner_(std::move(ipc_runner)),
       mojo_area_associated_remote_(std::move(area), ipc_task_runner_),
       mojo_area_(mojo_area_associated_remote_.get()),
@@ -578,8 +580,10 @@ void CachedStorageArea::EnqueueStorageEvent(const String& key,
     }
   }
   areas_->RemoveAll(areas_to_remove_);
-  inspector_event_listener_->DidDispatchStorageEvent(origin_.get(), key,
-                                                     old_value, new_value);
+  if (storage_namespace_) {
+    storage_namespace_->DidDispatchStorageEvent(origin_.get(), key, old_value,
+                                                new_value);
+  }
 }
 
 // static

third_party/blink/renderer/modules/storage/cached_storage_area.h

@@ -15,6 +15,7 @@
 #include "third_party/blink/public/platform/scheduler/web_scoped_virtual_time_pauser.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
 #include "third_party/blink/renderer/modules/storage/storage_area_map.h"
+#include "third_party/blink/renderer/modules/storage/storage_namespace.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/weborigin/security_origin.h"
@@ -56,26 +57,16 @@ class MODULES_EXPORT CachedStorageArea
         WebScopedVirtualTimePauser::VirtualTaskDuration duration) = 0;
   };
 
-  // Used to send events to the InspectorDOMStorageAgent.
-  class InspectorEventListener {
-   public:
-    virtual ~InspectorEventListener() = default;
-    virtual void DidDispatchStorageEvent(const SecurityOrigin* origin,
-                                         const String& key,
-                                         const String& old_value,
-                                         const String& new_value) = 0;
-  };
-
   static scoped_refptr<CachedStorageArea> CreateForLocalStorage(
       scoped_refptr<const SecurityOrigin> origin,
       mojo::PendingRemote<mojom::blink::StorageArea> area,
       scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-      InspectorEventListener* listener);
+      StorageNamespace* storage_namespace);
   static scoped_refptr<CachedStorageArea> CreateForSessionStorage(
       scoped_refptr<const SecurityOrigin> origin,
       mojo::PendingAssociatedRemote<mojom::blink::StorageArea> area,
       scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-      InspectorEventListener* listener);
+      StorageNamespace* storage_namespace);
 
   // These correspond to blink::Storage.
   unsigned GetLength();
@@ -104,12 +95,12 @@ class MODULES_EXPORT CachedStorageArea
   CachedStorageArea(scoped_refptr<const SecurityOrigin> origin,
                     mojo::PendingRemote<mojom::blink::StorageArea> area,
                     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-                    InspectorEventListener* listener);
+                    StorageNamespace* storage_namespace);
   CachedStorageArea(
       scoped_refptr<const SecurityOrigin> origin,
       mojo::PendingAssociatedRemote<mojom::blink::StorageArea> area,
       scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-      InspectorEventListener* listener);
+      StorageNamespace* storage_namespace);
 
   friend class RefCounted<CachedStorageArea>;
   ~CachedStorageArea() override;
@@ -180,7 +171,7 @@ class MODULES_EXPORT CachedStorageArea
                                              FormatOption format_option);
 
   const scoped_refptr<const SecurityOrigin> origin_;
-  InspectorEventListener* const inspector_event_listener_;
+  const WeakPersistent<StorageNamespace> storage_namespace_;
   const scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
 
   std::unique_ptr<StorageAreaMap> map_;

third_party/blink/renderer/modules/storage/cached_storage_area_test.cc

@@ -16,8 +16,7 @@ namespace blink {
 
 using FormatOption = CachedStorageArea::FormatOption;
 
-class CachedStorageAreaTest : public testing::Test,
-                              public CachedStorageArea::InspectorEventListener {
+class CachedStorageAreaTest : public testing::Test {
  public:
   const scoped_refptr<SecurityOrigin> kOrigin =
       SecurityOrigin::CreateFromString("http://dom_storage/");
@@ -33,11 +32,11 @@ class CachedStorageAreaTest : public testing::Test,
     if (IsSessionStorage()) {
       cached_area_ = CachedStorageArea::CreateForSessionStorage(
           kOrigin, mock_storage_area_.GetAssociatedInterfaceRemote(),
-          scheduler::GetSingleThreadTaskRunnerForTesting(), this);
+          scheduler::GetSingleThreadTaskRunnerForTesting(), nullptr);
     } else {
       cached_area_ = CachedStorageArea::CreateForLocalStorage(
           kOrigin, mock_storage_area_.GetInterfaceRemote(),
-          scheduler::GetSingleThreadTaskRunnerForTesting(), this);
+          scheduler::GetSingleThreadTaskRunnerForTesting(), nullptr);
     }
     source_area_ = MakeGarbageCollected<FakeAreaSource>(kPageUrl);
     source_area_id_ = cached_area_->RegisterSource(source_area_);
@@ -46,11 +45,6 @@ class CachedStorageAreaTest : public testing::Test,
     cached_area_->RegisterSource(source_area2_);
   }
 
-  void DidDispatchStorageEvent(const SecurityOrigin* origin,
-                               const String& key,
-                               const String& old_value,
-                               const String& new_value) override {}
-
   virtual bool IsSessionStorage() { return false; }
 
   bool IsCacheLoaded() { return cached_area_->map_.get(); }

third_party/blink/renderer/modules/storage/storage_namespace.cc

@@ -52,8 +52,6 @@ StorageNamespace::StorageNamespace(StorageController* controller,
                                    const String& namespace_id)
     : controller_(controller), namespace_id_(namespace_id) {}
 
-StorageNamespace::~StorageNamespace() = default;
-
 // static
 void StorageNamespace::ProvideSessionStorageNamespaceTo(Page& page,
                                                         WebViewClient* client) {

third_party/blink/renderer/modules/storage/storage_namespace.h

@@ -34,7 +34,6 @@
 #include "third_party/blink/public/mojom/dom_storage/storage_partition_service.mojom-blink-forward.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
-#include "third_party/blink/renderer/modules/storage/cached_storage_area.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/supplementable.h"
@@ -44,6 +43,7 @@
 
 namespace blink {
 
+class CachedStorageArea;
 class InspectorDOMStorageAgent;
 class StorageController;
 class SecurityOrigin;
@@ -66,8 +66,7 @@ class WebViewClient;
 // is used to get the storage area for an origin.
 class MODULES_EXPORT StorageNamespace final
     : public GarbageCollected<StorageNamespace>,
-      public Supplement<Page>,
-      public CachedStorageArea::InspectorEventListener {
+      public Supplement<Page> {
   USING_GARBAGE_COLLECTED_MIXIN(StorageNamespace);
 
  public:
@@ -83,8 +82,6 @@ class MODULES_EXPORT StorageNamespace final
   // Creates a namespace for SessionStorage.
   StorageNamespace(StorageController*, const String& namespace_id);
 
-  ~StorageNamespace() override;
-
   scoped_refptr<CachedStorageArea> GetCachedArea(const SecurityOrigin* origin);
 
   // Only valid to call this if |this| and |target| are session storage
@@ -108,7 +105,7 @@ class MODULES_EXPORT StorageNamespace final
   void DidDispatchStorageEvent(const SecurityOrigin* origin,
                                const String& key,
                                const String& old_value,
-                               const String& new_value) override;
+                               const String& new_value);
 
  private:
   void EnsureConnected();