Have WeakPtr<StoragePartition> in CrossOriginEmbedderPolicyReporter

It is hard to ensure that CrossOriginEmbedderPolicyReporter doesn't
outlive StoragePartition. Let's have a weak pointer instead of a raw
pointer to the StoragePartition to fix the use-after-free bug.

Bug: 1262516, 1262391
Change-Id: Iffe5f2b55342f98c59c1e63cf931184b4ef1d556
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3291105
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Auto-Submit: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944861}

content/browser/net/cross_origin_embedder_policy_reporter.cc

@@ -28,14 +28,14 @@ GURL StripUsernameAndPassword(const GURL& url) {
 
 CrossOriginEmbedderPolicyReporter::CrossOriginEmbedderPolicyReporter(
     Creator creator,
-    StoragePartition* storage_partition,
+    base::WeakPtr<StoragePartition> storage_partition,
     const GURL& context_url,
     const absl::optional<std::string>& endpoint,
     const absl::optional<std::string>& report_only_endpoint,
     const base::UnguessableToken& reporting_source,
     const net::NetworkIsolationKey& network_isolation_key)
     : creator_(creator),
-      storage_partition_(storage_partition),
+      storage_partition_(std::move(storage_partition)),
       context_url_(context_url),
       endpoint_(endpoint),
       report_only_endpoint_(report_only_endpoint),
@@ -125,10 +125,12 @@ void CrossOriginEmbedderPolicyReporter::QueueAndNotify(
     }
     body_to_pass.SetString("disposition", disposition);
 
-    storage_partition_->GetNetworkContext()->QueueReport(
-        kType, *endpoint, context_url_, reporting_source_,
-        network_isolation_key_,
-        /*user_agent=*/absl::nullopt, std::move(body_to_pass));
+    if (auto* storage_partition = storage_partition_.get()) {
+      storage_partition->GetNetworkContext()->QueueReport(
+          kType, *endpoint, context_url_, reporting_source_,
+          network_isolation_key_,
+          /*user_agent=*/absl::nullopt, std::move(body_to_pass));
+    }
   }
 }
 

content/browser/net/cross_origin_embedder_policy_reporter.h

@@ -30,10 +30,7 @@ class StoragePartition;
 // DedicatedWorkerHost. They create a mojo endpoint using Clone and pass it
 // around. For example, it's sent to the Network Service via
 // network.mojom.URLLoaderFactoryParam.coep_reporter.
-// Any functions other than the destructor must not be called after the
-// associated StoragePartition is destructed.
-// TODO(yhirano): This currently only sends reports to the network. Notify
-// the event to the associated ReportingObserver.
+// A CrossOriginEmbedderPolicyReporter lives on the UI thread.
 class CONTENT_EXPORT CrossOriginEmbedderPolicyReporter final
     : public network::mojom::CrossOriginEmbedderPolicyReporter {
  public:
@@ -48,7 +45,7 @@ class CONTENT_EXPORT CrossOriginEmbedderPolicyReporter final
 
   CrossOriginEmbedderPolicyReporter(
       Creator creator,
-      StoragePartition* storage_partition,
+      base::WeakPtr<StoragePartition> storage_partition,
       const GURL& context_url,
       const absl::optional<std::string>& endpoint,
       const absl::optional<std::string>& report_only_endpoint,
@@ -94,8 +91,7 @@ class CONTENT_EXPORT CrossOriginEmbedderPolicyReporter final
 
   const Creator creator_;
 
-  // See the class comment.
-  StoragePartition* const storage_partition_;
+  base::WeakPtr<StoragePartition> storage_partition_;
 
   const GURL context_url_;
   const absl::optional<std::string> endpoint_;

content/browser/net/cross_origin_embedder_policy_reporter_unittest.cc

@@ -97,7 +97,10 @@ class CrossOriginEmbedderPolicyReporterTest : public testing::Test {
     storage_partition_.set_network_context(&network_context_);
   }
 
-  StoragePartition* storage_partition() { return &storage_partition_; }
+  base::WeakPtr<StoragePartition> GetStoragePartition() {
+    return storage_partition_.GetWeakPtr();
+  }
+  void InvalidateWeakPtrs() { storage_partition_.InvalidateWeakPtrs(); }
   const TestNetworkContext& network_context() const { return network_context_; }
   base::Value CreateBodyForCorp(base::StringPiece blocked_url,
                                 RequestDestination destination,
@@ -196,8 +199,9 @@ class CrossOriginEmbedderPolicyReporterTest : public testing::Test {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, NullEndpointsForCorp) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
 
   reporter.QueueCorpViolationReport(GURL("https://www1.example.com/y"),
                                     RequestDestination::kEmpty,
@@ -214,8 +218,8 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, BasicCorp) {
   const auto kNetworkIsolationKey = net::NetworkIsolationKey::CreateTransient();
   const auto kReportingSource = base::UnguessableToken::Create();
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2", kReportingSource,
-      kNetworkIsolationKey);
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
+      kReportingSource, kNetworkIsolationKey);
 
   reporter.QueueCorpViolationReport(
       GURL("https://www1.example.com/x#foo?bar=baz"),
@@ -247,7 +251,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, BasicCorp) {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, UserAndPassForCorp) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
 
   reporter.QueueCorpViolationReport(GURL("https://u:p@www1.example.com/x"),
@@ -280,8 +284,9 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, ObserverForCorp) {
   TestObserver observer(observer_remote.InitWithNewPipeAndPassReceiver());
 
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
   reporter.BindObserver(std::move(observer_remote));
   reporter.QueueCorpViolationReport(GURL("https://u:p@www1.example.com/x"),
                                     RequestDestination::kImage,
@@ -311,7 +316,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, ObserverForCorp) {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, Clone) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
 
   mojo::Remote<network::mojom::CrossOriginEmbedderPolicyReporter> remote;
@@ -346,8 +351,9 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, Clone) {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, NullEndpointsForNavigation) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
 
   reporter.QueueNavigationReport(GURL("https://www1.example.com/y"),
                                  /*report_only=*/false);
@@ -360,7 +366,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, NullEndpointsForNavigation) {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, BasicNavigation) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
 
   reporter.QueueNavigationReport(GURL("https://www1.example.com/x#foo?bar=baz"),
@@ -390,8 +396,9 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, ObserverForNavigation) {
   TestObserver observer(observer_remote.InitWithNewPipeAndPassReceiver());
 
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
   reporter.BindObserver(std::move(observer_remote));
   reporter.QueueNavigationReport(GURL("https://www1.example.com/x#foo?bar=baz"),
                                  /*report_only=*/false);
@@ -418,7 +425,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, ObserverForNavigation) {
 TEST_F(CrossOriginEmbedderPolicyReporterTest, UserAndPassForNavigation) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
   reporter.QueueNavigationReport(GURL("https://u:p@www1.example.com/x"),
                                  /*report_only=*/false);
@@ -445,8 +452,9 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest,
        NullEndpointsForWorkerInitialization) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
 
   reporter.QueueWorkerInitializationReport(
       GURL("https://www1.example.com/x.js"),
@@ -461,7 +469,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest,
 TEST_F(CrossOriginEmbedderPolicyReporterTest, BasicWorkerInitialization) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
 
   reporter.QueueWorkerInitializationReport(
@@ -493,8 +501,9 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest, ObserverForWorkerInitialization) {
   TestObserver observer(observer_remote.InitWithNewPipeAndPassReceiver());
 
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, absl::nullopt, absl::nullopt,
-      base::UnguessableToken::Create(), net::NetworkIsolationKey());
+      kCreator, GetStoragePartition(), kContextUrl, absl::nullopt,
+      absl::nullopt, base::UnguessableToken::Create(),
+      net::NetworkIsolationKey());
   reporter.BindObserver(std::move(observer_remote));
   reporter.QueueWorkerInitializationReport(
       GURL("https://www1.example.com/x.js#foo?bar=baz"),
@@ -524,7 +533,7 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest,
        UserAndPassForWorkerInitialization) {
   const GURL kContextUrl("https://example.com/path");
   CrossOriginEmbedderPolicyReporter reporter(
-      kCreator, storage_partition(), kContextUrl, "e1", "e2",
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
       base::UnguessableToken::Create(), net::NetworkIsolationKey());
   reporter.QueueWorkerInitializationReport(
       GURL("https://u:p@www1.example.com/x.js"),
@@ -549,5 +558,26 @@ TEST_F(CrossOriginEmbedderPolicyReporterTest,
                          "https://www2.example.com/y.js", "reporting"));
 }
 
+TEST_F(CrossOriginEmbedderPolicyReporterTest, StoragePartitionInvalidated) {
+  const GURL kContextUrl("https://example.com/path");
+  const auto kNetworkIsolationKey = net::NetworkIsolationKey::CreateTransient();
+  const auto kReportingSource = base::UnguessableToken::Create();
+  CrossOriginEmbedderPolicyReporter reporter(
+      kCreator, GetStoragePartition(), kContextUrl, "e1", "e2",
+      kReportingSource, kNetworkIsolationKey);
+
+  InvalidateWeakPtrs();
+
+  reporter.QueueCorpViolationReport(
+      GURL("https://www1.example.com/x#foo?bar=baz"),
+      RequestDestination::kScript,
+      /*report_only=*/false);
+  reporter.QueueCorpViolationReport(GURL("http://www2.example.com:41/y"),
+                                    RequestDestination::kEmpty,
+                                    /*report_only=*/true);
+
+  ASSERT_EQ(0u, network_context().reports().size());
+}
+
 }  // namespace
 }  // namespace content

content/browser/renderer_host/navigation_request.cc

@@ -2308,7 +2308,8 @@ void NavigationRequest::CreateCoepReporter(
   DCHECK(!isolation_info_for_subresources_.IsEmpty());
 
   coep_reporter_ = std::make_unique<CrossOriginEmbedderPolicyReporter>(
-      CrossOriginEmbedderPolicyReporter::Creator::kDocument, storage_partition,
+      CrossOriginEmbedderPolicyReporter::Creator::kDocument,
+      static_cast<StoragePartitionImpl*>(storage_partition)->GetWeakPtr(),
       common_params_->url, cross_origin_embedder_policy_.reporting_endpoint,
       cross_origin_embedder_policy_.report_only_reporting_endpoint,
       render_frame_host_->GetFrameToken().value(),

content/browser/renderer_host/render_frame_host_impl.cc

@@ -9831,9 +9831,11 @@ RenderFrameHostImpl::CreateNavigationRequestForSynchronousRendererCommit(
   // We don't switch the COEP reporter on same-document navigations, so create
   // one only for cross-document navigations.
   if (!is_same_document) {
+    auto* storage_partition =
+        static_cast<StoragePartitionImpl*>(GetProcess()->GetStoragePartition());
     coep_reporter = std::make_unique<CrossOriginEmbedderPolicyReporter>(
         CrossOriginEmbedderPolicyReporter::Creator::kDocument,
-        GetProcess()->GetStoragePartition(), url,
+        storage_partition->GetWeakPtr(), url,
         cross_origin_embedder_policy_.reporting_endpoint,
         cross_origin_embedder_policy_.report_only_reporting_endpoint,
         GetReportingSource(), isolation_info.network_isolation_key());

content/browser/service_worker/embedded_worker_instance.cc

@@ -307,9 +307,11 @@ void EmbeddedWorkerInstance::Start(
           reporting_observer_remote;
       owner_version_->set_reporting_observer_receiver(
           reporting_observer_remote.InitWithNewPipeAndPassReceiver());
+      auto* storage_partition =
+          static_cast<StoragePartitionImpl*>(rph->GetStoragePartition());
       coep_reporter_ = std::make_unique<CrossOriginEmbedderPolicyReporter>(
           CrossOriginEmbedderPolicyReporter::Creator::kServiceWorker,
-          rph->GetStoragePartition(), params->script_url,
+          storage_partition->GetWeakPtr(), params->script_url,
           owner_version_->cross_origin_embedder_policy()->reporting_endpoint,
           owner_version_->cross_origin_embedder_policy()
               ->report_only_reporting_endpoint,
@@ -867,9 +869,11 @@ EmbeddedWorkerInstance::CreateFactoryBundles() {
     owner_version_->set_reporting_observer_receiver(
         reporting_observer_remote.InitWithNewPipeAndPassReceiver());
 
+    auto* storage_partition =
+        static_cast<StoragePartitionImpl*>(rph->GetStoragePartition());
     coep_reporter_ = std::make_unique<CrossOriginEmbedderPolicyReporter>(
         CrossOriginEmbedderPolicyReporter::Creator::kServiceWorker,
-        rph->GetStoragePartition(), owner_version_->script_url(),
+        storage_partition->GetWeakPtr(), owner_version_->script_url(),
         owner_version_->cross_origin_embedder_policy()->reporting_endpoint,
         owner_version_->cross_origin_embedder_policy()
             ->report_only_reporting_endpoint,

content/browser/storage_partition_impl.cc

@@ -2594,6 +2594,10 @@ void StoragePartitionImpl::SetNetworkContextForTesting(
   network_context_.Bind(std::move(network_context_remote));
 }
 
+base::WeakPtr<StoragePartition> StoragePartitionImpl::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 BrowserContext* StoragePartitionImpl::browser_context() const {
   return browser_context_;
 }

content/browser/storage_partition_impl.h

@@ -219,6 +219,8 @@ class CONTENT_EXPORT StoragePartitionImpl
   void SetNetworkContextForTesting(
       mojo::PendingRemote<network::mojom::NetworkContext>
           network_context_remote) override;
+
+  base::WeakPtr<StoragePartition> GetWeakPtr();
   BackgroundFetchContext* GetBackgroundFetchContext();
   PaymentAppContextImpl* GetPaymentAppContext();
   BroadcastChannelService* GetBroadcastChannelService();

content/browser/worker_host/dedicated_worker_host.cc

@@ -136,7 +136,8 @@ DedicatedWorkerHost::~DedicatedWorkerHost() {
   }
 
   // Send any final reports and allow the reporting configuration to be
-  // removed.
+  // removed. Note that the RenderProcessHost and the associated
+  // StoragePartition outlives `this`.
   worker_process_host_->GetStoragePartition()
       ->GetNetworkContext()
       ->SendReportsAndRemoveSource(reporting_source_);
@@ -365,10 +366,12 @@ void DedicatedWorkerHost::DidStartScriptLoad(
         final_response_url, main_script_load_params->response_head.get());
   }
 
+  auto* storage_partition = static_cast<StoragePartitionImpl*>(
+      worker_process_host_->GetStoragePartition());
   // Create a COEP reporter with worker's policy.
   coep_reporter_ = std::make_unique<CrossOriginEmbedderPolicyReporter>(
       CrossOriginEmbedderPolicyReporter::Creator::kDedicatedWorker,
-      worker_process_host_->GetStoragePartition(), final_response_url,
+      storage_partition->GetWeakPtr(), final_response_url,
       worker_cross_origin_embedder_policy_->reporting_endpoint,
       worker_cross_origin_embedder_policy_->report_only_reporting_endpoint,
       reporting_source_, isolation_info_.network_isolation_key());

content/browser/worker_host/dedicated_worker_service_impl_unittest.cc

@@ -44,7 +44,8 @@ class MockDedicatedWorker
     auto coep_reporter = std::make_unique<CrossOriginEmbedderPolicyReporter>(
         CrossOriginEmbedderPolicyReporter::Creator::kDedicatedWorker,
         RenderFrameHostImpl::FromID(render_frame_host_id)
-            ->GetStoragePartition(),
+            ->GetStoragePartition()
+            ->GetWeakPtr(),
         GURL(), absl::nullopt, absl::nullopt, base::UnguessableToken::Create(),
         net::NetworkIsolationKey());
 

content/browser/worker_host/shared_worker_host.cc

@@ -167,6 +167,8 @@ SharedWorkerHost::~SharedWorkerHost() {
   // Send any final reports and allow the reporting configuration to be
   // removed.
   if (site_instance_->HasProcess()) {
+    // Note that the RenderProcessHost and the associated StoragePartition
+    // outlives `this`.
     GetProcessHost()
         ->GetStoragePartition()
         ->GetNetworkContext()
@@ -235,10 +237,12 @@ void SharedWorkerHost::Start(
         break;
     }
 
+    auto* storage_partition = static_cast<StoragePartitionImpl*>(
+        GetProcessHost()->GetStoragePartition());
     // Create a COEP reporter with worker's policy.
     coep_reporter_ = std::make_unique<CrossOriginEmbedderPolicyReporter>(
         CrossOriginEmbedderPolicyReporter::Creator::kSharedWorker,
-        GetProcessHost()->GetStoragePartition(), final_response_url,
+        storage_partition->GetWeakPtr(), final_response_url,
         worker_cross_origin_embedder_policy_->reporting_endpoint,
         worker_cross_origin_embedder_policy_->report_only_reporting_endpoint,
         GetReportingSource(), GetNetworkIsolationKey());

content/public/test/test_storage_partition.cc

@@ -237,4 +237,12 @@ void TestStoragePartition::SetNetworkContextForTesting(
     mojo::PendingRemote<network::mojom::NetworkContext>
         network_context_remote) {}
 
+base::WeakPtr<StoragePartition> TestStoragePartition::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void TestStoragePartition::InvalidateWeakPtrs() {
+  weak_factory_.InvalidateWeakPtrs();
+}
+
 }  // namespace content

content/public/test/test_storage_partition.h

@@ -217,6 +217,9 @@ class TestStoragePartition : public StoragePartition {
       mojo::PendingRemote<network::mojom::NetworkContext>
           network_context_remote) override;
 
+  base::WeakPtr<StoragePartition> GetWeakPtr();
+  void InvalidateWeakPtrs();
+
  private:
   base::FilePath file_path_;
   mojo::Remote<network::mojom::NetworkContext> network_context_remote_;
@@ -243,6 +246,9 @@ class TestStoragePartition : public StoragePartition {
   HostZoomLevelContext* host_zoom_level_context_ = nullptr;
   ZoomLevelDelegate* zoom_level_delegate_ = nullptr;
   int data_removal_observer_count_ = 0;
+
+  // This member must be the last member.
+  base::WeakPtrFactory<TestStoragePartition> weak_factory_{this};
 };
 
 }  // namespace content