From 95d3d668d0ba7c6512eb0aaefbf3de1c55999633 Mon Sep 17 00:00:00 2001
From: Hayato Ito <hayato@chromium.org>
Date: Thu, 13 Aug 2026 02:58:34 -0700
Subject: [PATCH] Fix potential UAF and dangling pointer in WebBundleURLLoaderFactory

WebBundleURLLoaderFactory stored coep_reporter_ as a raw pointer
annotated with FlakyDanglingUntriaged. It pointed to a
mojom::CrossOriginEmbedderPolicyReporter owned by CorsURLLoaderFactory.

Because CorsURLLoaderFactory is scoped to the lifetime of the caller (or
NetworkContext) while WebBundleURLLoaderFactory is managed by
WebBundleManager and scoped to its WebBundleHandle pipe,
CorsURLLoaderFactory could be destroyed before
WebBundleURLLoaderFactory, leaving coep_reporter_ dangling.

To fix this, we update WebBundleURLLoaderFactory to maintain its own
independent ownership of a
mojo::Remote<mojom::CrossOriginEmbedderPolicyReporter> by cloning the
remote handle when creating the factory in CorsURLLoaderFactory.

Bug: 349994197
Change-Id: I99f663089897f89c340948d44b7457cc1feb7da4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8247938
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Hayato Ito <hayato@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1678719}
---

diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc
index 72ebd3f..f71710c 100644
--- a/services/network/cors/cors_url_loader_factory.cc
+++ b/services/network/cors/cors_url_loader_factory.cc
@@ -403,12 +403,21 @@
       network::mojom::RequestDestination::kWebBundle) {
     DCHECK(resource_request.web_bundle_token_params.has_value());
 
+    // Clone the COEP reporter to pass independent ownership to the WebBundle
+    // loader factory.
+    mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter>
+        coep_reporter_remote;
+    if (coep_reporter_) {
+      coep_reporter_->Clone(
+          coep_reporter_remote.InitWithNewPipeAndPassReceiver());
+    }
+
     // TODO(crbug.com/379869738) Remove GetUnsafeValue.
     base::WeakPtr<WebBundleURLLoaderFactory> web_bundle_url_loader_factory =
         context_->GetWebBundleManager().CreateWebBundleURLLoaderFactory(
             resource_request.url, *resource_request.web_bundle_token_params,
             process_id_.GetUnsafeValue(), cross_origin_embedder_policy_,
-            coep_reporter());
+            std::move(coep_reporter_remote));
     client = web_bundle_url_loader_factory->MaybeWrapURLLoaderClient(
         std::move(client));
     if (!client) {
diff --git a/services/network/web_bundle/web_bundle_manager.cc b/services/network/web_bundle/web_bundle_manager.cc
index b699459..ab51309 100644
--- a/services/network/web_bundle/web_bundle_manager.cc
+++ b/services/network/web_bundle/web_bundle_manager.cc
@@ -60,7 +60,8 @@
     const ResourceRequest::WebBundleTokenParams& web_bundle_token_params,
     int32_t process_id,
     const CrossOriginEmbedderPolicy& cross_origin_embedder_policy,
-    mojom::CrossOriginEmbedderPolicyReporter* coep_reporter) {
+    mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter>
+        coep_reporter) {
   Key key = GetKey(web_bundle_token_params, process_id);
   DCHECK(factories_.find(key) == factories_.end());
   DCHECK(web_bundle_token_params.handle.is_valid());
@@ -81,7 +82,7 @@
       bundle_url, web_bundle_token_params, std::move(remote),
       std::make_unique<MemoryQuotaConsumer>(weak_ptr_factory_.GetWeakPtr(),
                                             process_id),
-      cross_origin_embedder_policy, coep_reporter);
+      cross_origin_embedder_policy, std::move(coep_reporter));
 
   // Process pending subresource loaders if there are.
   // These subresource requests arrived earlier than the request for the bundle.
diff --git a/services/network/web_bundle/web_bundle_manager.h b/services/network/web_bundle/web_bundle_manager.h
index 9bc9059..666b983 100644
--- a/services/network/web_bundle/web_bundle_manager.h
+++ b/services/network/web_bundle/web_bundle_manager.h
@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/unguessable_token.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/cross_origin_embedder_policy.h"
 #include "services/network/public/cpp/resource_request.h"
@@ -37,7 +38,8 @@
       const ResourceRequest::WebBundleTokenParams& params,
       int32_t process_id,
       const CrossOriginEmbedderPolicy& cross_origin_embedder_policy,
-      mojom::CrossOriginEmbedderPolicyReporter* coep_reporter);
+      mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter>
+          coep_reporter);
 
   void StartSubresourceRequest(
       mojo::PendingReceiver<mojom::URLLoader> receiver,
diff --git a/services/network/web_bundle/web_bundle_manager_unittest.cc b/services/network/web_bundle/web_bundle_manager_unittest.cc
index 7a42f07..e2f8ca55 100644
--- a/services/network/web_bundle/web_bundle_manager_unittest.cc
+++ b/services/network/web_bundle/web_bundle_manager_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/test/task_environment.h"
 #include "base/unguessable_token.h"
 #include "components/web_package/web_bundle_builder.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
@@ -99,8 +100,7 @@
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
       manager.CreateWebBundleURLLoaderFactory(
           GURL(kBundleUrl), create_params, process_id,
-          CrossOriginEmbedderPolicy(),
-          /*coep_reporter=*/nullptr);
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   return std::forward_as_tuple(std::move(factory), std::move(handle));
 }
@@ -172,9 +172,8 @@
                                                       std::move(handle));
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), create_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), create_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
   ASSERT_TRUE(factory);
 
   ResourceRequest::WebBundleTokenParams find_params(GURL(kBundleUrl), token,
@@ -193,9 +192,8 @@
                                                       std::move(handle));
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), create_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), create_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
   ASSERT_TRUE(factory);
 
   ResourceRequest::WebBundleTokenParams find_params1(GURL(kBundleUrl), token,
@@ -224,8 +222,7 @@
 
     auto factory = manager.CreateWebBundleURLLoaderFactory(
         GURL(kBundleUrl), create_params, process_id1,
-        CrossOriginEmbedderPolicy(),
-        /*coep_reporter=*/nullptr);
+        CrossOriginEmbedderPolicy(), mojo::NullRemote());
     ASSERT_TRUE(factory);
     ASSERT_TRUE(
         GetWebBundleURLLoaderFactory(manager, find_params, process_id1));
@@ -311,9 +308,8 @@
       token_params.handle.InitWithNewPipeAndPassReceiver();
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), token_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), token_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
 
   // Then, simulate that the bundle is loaded from the network, calling
   // SetBundleStream manually.
@@ -678,10 +674,9 @@
   // is readirected by WebRequest extension API.
   GURL redirected_bundle_url("https://redirected.example.com/bundle.wbn");
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
-      manager.CreateWebBundleURLLoaderFactory(redirected_bundle_url,
-                                              create_params, process_id1,
-                                              CrossOriginEmbedderPolicy(),
-                                              /*coep_reporter=*/nullptr);
+      manager.CreateWebBundleURLLoaderFactory(
+          redirected_bundle_url, create_params, process_id1,
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   // TestWebBundleHandle must receive an error.
   handle->RunUntilBundleError();
@@ -758,7 +753,7 @@
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
       manager.CreateWebBundleURLLoaderFactory(
           redirected_bundle_url, create_params, process_id1,
-          CrossOriginEmbedderPolicy(), /*coep_reporter=*/nullptr);
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   handle->RunUntilBundleError();
   ASSERT_TRUE(handle->last_bundle_error().has_value());
diff --git a/services/network/web_bundle/web_bundle_url_loader_factory.cc b/services/network/web_bundle/web_bundle_url_loader_factory.cc
index 504e55e..f822884 100644
--- a/services/network/web_bundle/web_bundle_url_loader_factory.cc
+++ b/services/network/web_bundle/web_bundle_url_loader_factory.cc
@@ -547,14 +547,14 @@
     std::unique_ptr<WebBundleMemoryQuotaConsumer>
         web_bundle_memory_quota_consumer,
     const CrossOriginEmbedderPolicy& cross_origin_embedder_policy,
-    mojom::CrossOriginEmbedderPolicyReporter* coep_reporter)

diff --git a/services/network/web_bundle/web_bundle_manager_unittest.cc b/services/network/web_bundle/web_bundle_manager_unittest.cc
index 7a42f07..e2f8ca55 100644
--- a/services/network/web_bundle/web_bundle_manager_unittest.cc
+++ b/services/network/web_bundle/web_bundle_manager_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/test/task_environment.h"
 #include "base/unguessable_token.h"
 #include "components/web_package/web_bundle_builder.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
@@ -99,8 +100,7 @@
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
       manager.CreateWebBundleURLLoaderFactory(
           GURL(kBundleUrl), create_params, process_id,
-          CrossOriginEmbedderPolicy(),
-          /*coep_reporter=*/nullptr);
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   return std::forward_as_tuple(std::move(factory), std::move(handle));
 }
@@ -172,9 +172,8 @@
                                                       std::move(handle));
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), create_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), create_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
   ASSERT_TRUE(factory);
 
   ResourceRequest::WebBundleTokenParams find_params(GURL(kBundleUrl), token,
@@ -193,9 +192,8 @@
                                                       std::move(handle));
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), create_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), create_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
   ASSERT_TRUE(factory);
 
   ResourceRequest::WebBundleTokenParams find_params1(GURL(kBundleUrl), token,
@@ -224,8 +222,7 @@
 
     auto factory = manager.CreateWebBundleURLLoaderFactory(
         GURL(kBundleUrl), create_params, process_id1,
-        CrossOriginEmbedderPolicy(),
-        /*coep_reporter=*/nullptr);
+        CrossOriginEmbedderPolicy(), mojo::NullRemote());
     ASSERT_TRUE(factory);
     ASSERT_TRUE(
         GetWebBundleURLLoaderFactory(manager, find_params, process_id1));
@@ -311,9 +308,8 @@
       token_params.handle.InitWithNewPipeAndPassReceiver();
 
   auto factory = manager.CreateWebBundleURLLoaderFactory(
-      GURL(kBundleUrl), token_params, process_id1,
-      CrossOriginEmbedderPolicy(),
-      /*coep_reporter=*/nullptr);
+      GURL(kBundleUrl), token_params, process_id1, CrossOriginEmbedderPolicy(),
+      mojo::NullRemote());
 
   // Then, simulate that the bundle is loaded from the network, calling
   // SetBundleStream manually.
@@ -678,10 +674,9 @@
   // is readirected by WebRequest extension API.
   GURL redirected_bundle_url("https://redirected.example.com/bundle.wbn");
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
-      manager.CreateWebBundleURLLoaderFactory(redirected_bundle_url,
-                                              create_params, process_id1,
-                                              CrossOriginEmbedderPolicy(),
-                                              /*coep_reporter=*/nullptr);
+      manager.CreateWebBundleURLLoaderFactory(
+          redirected_bundle_url, create_params, process_id1,
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   // TestWebBundleHandle must receive an error.
   handle->RunUntilBundleError();
@@ -758,7 +753,7 @@
   base::WeakPtr<WebBundleURLLoaderFactory> factory =
       manager.CreateWebBundleURLLoaderFactory(
           redirected_bundle_url, create_params, process_id1,
-          CrossOriginEmbedderPolicy(), /*coep_reporter=*/nullptr);
+          CrossOriginEmbedderPolicy(), mojo::NullRemote());
 
   handle->RunUntilBundleError();
   ASSERT_TRUE(handle->last_bundle_error().has_value());
diff --git a/services/network/web_bundle/web_bundle_url_loader_factory_unittest.cc b/services/network/web_bundle/web_bundle_url_loader_factory_unittest.cc
index 7dbe9aa..7816a35 100644
--- a/services/network/web_bundle/web_bundle_url_loader_factory_unittest.cc
+++ b/services/network/web_bundle/web_bundle_url_loader_factory_unittest.cc
@@ -8,11 +8,14 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
 #include "components/web_package/web_bundle_builder.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "mojo/public/cpp/system/functions.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/mojom/cross_origin_embedder_policy.mojom.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
 #include "services/network/test/test_url_loader_client.h"
@@ -155,10 +158,62 @@
   mojo::internal::MessageDispatchContext context_;
 };
 
+// Test implementation of mojom::CrossOriginEmbedderPolicyReporter that records
+// reported blocked URLs from CORP violations.
+class TestCoepReporter : public mojom::CrossOriginEmbedderPolicyReporter {
+ public:
+  TestCoepReporter() = default;
+  ~TestCoepReporter() override = default;
+
+  void QueueCorpViolationReport(const GURL& blocked_url,
+                                mojom::RequestDestination destination,
+                                bool report_only) override {
+    reports_.push_back(blocked_url);
+    if (quit_closure_) {
+      std::move(quit_closure_).Run();
+    }
+  }
+
+  void Clone(mojo::PendingReceiver<mojom::CrossOriginEmbedderPolicyReporter>
+                 receiver) override {
+    receivers_.Add(this, std::move(receiver));
+  }
+
+  mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter>
+  CreatePendingRemote() {
+    mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter> remote;
+    receivers_.Add(this, remote.InitWithNewPipeAndPassReceiver());
+    return remote;
+  }
+
+  void RunUntilReport() {
+    if (!reports_.empty()) {
+      return;
+    }
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+  const std::vector<GURL>& reports() const { return reports_; }
+
+ private:
+  std::vector<GURL> reports_;
+  base::OnceClosure quit_closure_;
+  mojo::ReceiverSet<mojom::CrossOriginEmbedderPolicyReporter> receivers_;
+};
+
 }  // namespace
 
 class WebBundleURLLoaderFactoryTest : public ::testing::Test {
  public:
+  WebBundleURLLoaderFactoryTest()
+      : WebBundleURLLoaderFactoryTest(CrossOriginEmbedderPolicy()) {}
+
+  explicit WebBundleURLLoaderFactoryTest(
+      const CrossOriginEmbedderPolicy& cross_origin_embedder_policy)
+      : cross_origin_embedder_policy_(cross_origin_embedder_policy) {}
+
   void SetUp() override {
     mojo::ScopedDataPipeConsumerHandle consumer;
     ASSERT_EQ(CreateDataPipe(nullptr, bundle_data_destination_, consumer),
@@ -167,15 +222,24 @@
     handle_ = std::make_unique<TestWebBundleHandle>(
         handle.BindNewPipeAndPassReceiver());
 
+    // In production, CorsURLLoaderFactory owns the COEP reporter remote and
+    // clones it for WebBundleURLLoaderFactory. We mirror that ownership here.
+    mojo::PendingRemote<mojom::CrossOriginEmbedderPolicyReporter> cloned_remote;
+    original_coep_reporter_.Bind(coep_reporter_.CreatePendingRemote());
+    original_coep_reporter_->Clone(
+        cloned_remote.InitWithNewPipeAndPassReceiver());
+
     const ResourceRequest::WebBundleTokenParams create_params(
         GURL(kBundleUrl), {} /* token */, {} /* handle */);
     factory_ = std::make_unique<WebBundleURLLoaderFactory>(
         GURL(kBundleUrl), create_params, std::move(handle),
         std::make_unique<MockMemoryQuotaConsumer>(),
-        CrossOriginEmbedderPolicy(), nullptr /* coep_reporter */);
+        cross_origin_embedder_policy_, std::move(cloned_remote));
     factory_->SetBundleStream(std::move(consumer));
   }
 
+  void ResetOriginalCoepReporter() { original_coep_reporter_.reset(); }
+
   void WriteBundle(base::span<const uint8_t> data) {
     mojo::BlockingCopyFromString(
         std::string(reinterpret_cast<const char*>(data.data()), data.size()),
@@ -225,13 +289,25 @@
     return handle_->last_bundle_error();
   }
 
+  void RunUntilCoepReport() { coep_reporter_.RunUntilReport(); }
+
+  const std::vector<GURL>& coep_reports() const {
+    return coep_reporter_.reports();
+  }
+
  protected:
   std::unique_ptr<WebBundleURLLoaderFactory> factory_;
 
  private:
+  base::test::TaskEnvironment task_environment;
+
   std::unique_ptr<TestWebBundleHandle> handle_;
   mojo::ScopedDataPipeProducerHandle bundle_data_destination_;
-  base::test::TaskEnvironment task_environment;
+
+  const CrossOriginEmbedderPolicy cross_origin_embedder_policy_;
+  mojo::Remote<mojom::CrossOriginEmbedderPolicyReporter>
+      original_coep_reporter_;
+  TestCoepReporter coep_reporter_;
 };
 
 TEST_F(WebBundleURLLoaderFactoryTest, Basic) {
@@ -534,4 +610,60 @@
                   "WebBundleURLLoaderFactory: Bundle URL does not match"));
 }
 
+// Test fixture configured with CrossOriginEmbedderPolicy: require-corp.
+class WebBundleURLLoaderFactoryRequireCorpTest
+    : public WebBundleURLLoaderFactoryTest {
+ public:
+  WebBundleURLLoaderFactoryRequireCorpTest()
+      : WebBundleURLLoaderFactoryTest(RequireCorpPolicy()) {}
+
+ private:
+  static CrossOriginEmbedderPolicy RequireCorpPolicy() {
+    CrossOriginEmbedderPolicy coep;
+    coep.value = mojom::CrossOriginEmbedderPolicyValue::kRequireCorp;
+    return coep;
+  }
+};
+
+// Regression test for crbug.com/349994197.
+//
+// Previously, WebBundleURLLoaderFactory held a raw pointer to
+// mojom::CrossOriginEmbedderPolicyReporter borrowed from CorsURLLoaderFactory.
+// If CorsURLLoaderFactory was destroyed before WebBundleURLLoaderFactory,
+// attempting to report a CORP violation via the dangling pointer resulted in a
+// Use-After-Free (UAF).
+//
+// This test verifies that WebBundleURLLoaderFactory safely retains its own
+// cloned reporter remote even after the original reporter remote (owned by
+// CorsURLLoaderFactory) is destroyed:
+// 1. Simulate CorsURLLoaderFactory destruction via ResetOriginalCoepReporter().
+// 2. Issue a cross-origin subresource request that violates CORP.
+// 3. Verify that the request is blocked and the CORP violation report is
+//    safely delivered through the cloned remote without crashing.
+TEST_F(WebBundleURLLoaderFactoryRequireCorpTest, ClonedReporterDestruction) {
+  // Step 1: Simulate CorsURLLoaderFactory being destroyed while
+  // WebBundleURLLoaderFactory is still alive.
+  ResetOriginalCoepReporter();
+
+  WriteBundle(CreateCrossOriginBundle());
+  FinishWritingBundle();
+
+  // Step 2: Issue a cross-origin subresource request that lacks a CORP header.
+  // Under COEP: require-corp, this request will be blocked by CORP, which
+  // invokes coep_reporter_->QueueCorpViolationReport().
+  network::ResourceRequest request =
+      CreateRequest(GURL(kCrossOriginJsUrl), kResourceRequestId);
+  request.mode = mojom::RequestMode::kNoCors;
+  request.destination = mojom::RequestDestination::kScript;
+
+  auto result = StartRequest(request);
+  result.client->RunUntilComplete();
+  RunUntilCoepReport();
+
+  // Step 3: Verify the request is blocked and the report is safely delivered.
+  EXPECT_EQ(net::ERR_BLOCKED_BY_RESPONSE,
+            result.client->completion_status().error_code);
+  EXPECT_THAT(coep_reports(), ::testing::ElementsAre(GURL(kCrossOriginJsUrl)));
+}
+
 }  // namespace network