From b8c6971f9b30397be1b382cf940a14c59304ec96 Mon Sep 17 00:00:00 2001
From: S Ganesh <[email protected]>
Date: Thu, 11 Jun 2026 15:27:13 -0700
Subject: [PATCH] [updater] Fix UAF in network callbacks and net check logical typos

In event_logger.cc and network_fetcher_mac.mm, HTTP status codes shared
between network callbacks were managed using std::unique_ptr. Raw
pointers bound to repeating callbacks were dereferenced even after the
unique_ptrs were moved and freed in completion callbacks, causing a
Use-After-Free (UAF) under out-of-order execution.

This CL resolves the UAF by wrapping these variables in
base::RefCountedData. The scoped_refptrs are bound using
base::RetainedRef, allowing target lambdas to take raw pointers to avoid
ref-count copies on invocation. A negative regression unit test is added
to check callback out-of-order resilience.

Additionally, this CL fixes NSError detection on macOS by verifying
net_error != 0 instead of net_error > 0 (since NSError code values are
negative). It also corrects a logical typo in the non-2xx status code
boundary check logic (changing a dead-code "&&" to "||" check).

Fixed: 521866061
Change-Id: I7ad86ea20bf589fd52428559ef4f7a283a0b14ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7914203
Reviewed-by: Sorin Jianu <[email protected]>
Commit-Queue: S Ganesh <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1645652}
---

diff --git a/chrome/updater/configurator.h b/chrome/updater/configurator.h
index 5da9120..040f7b84e 100644
--- a/chrome/updater/configurator.h
+++ b/chrome/updater/configurator.h
@@ -98,9 +98,11 @@
   std::optional<std::vector<uint8_t>> GetCrxPublicKeyHash() const;
   base::TimeDelta MinimumEventLoggingCooldown() const;
 
+ protected:
+  ~Configurator() override;
+
  private:
   friend class base::RefCountedThreadSafe<Configurator>;
-  ~Configurator() override;
 
   SEQUENCE_CHECKER(sequence_checker_);
   scoped_refptr<UpdaterPrefs> prefs_;
diff --git a/chrome/updater/event_logger.cc b/chrome/updater/event_logger.cc
index f8e6d87..0d458ba 100644
--- a/chrome/updater/event_logger.cc
+++ b/chrome/updater/event_logger.cc
@@ -16,6 +16,7 @@
 #include "base/functional/callback.h"
 #include "base/logging.h"
 #include "base/memory/raw_ptr.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/sequence_checker.h"
 #include "base/strings/strcat.h"
@@ -151,8 +152,8 @@
             // The fetcher and response code are retained by
             // the completion callback.
             auto response_code =
-                std::make_unique<std::optional<int>>(std::nullopt);
-            std::optional<int>* response_code_ptr = response_code.get();
+                base::MakeRefCounted<base::RefCountedData<std::optional<int>>>(
+                    std::nullopt);
             update_client::NetworkFetcher* fetcher_ptr = fetcher.get();
 
             fetcher_ptr->PostRequest(
@@ -163,17 +164,18 @@
                        GetLoggingCookieValue(
                            now, configurator->GetUpdaterPersistedData())})}},
                 base::BindRepeating(
-                    [](std::optional<int>* response_code_out, int response_code,
-                       int64_t content_length) {
-                      *response_code_out = response_code;
+                    [](base::RefCountedData<std::optional<int>>*
+                           response_code_out,
+                       int response_code, int64_t content_length) {
+                      response_code_out->data = response_code;
                     },
-                    response_code_ptr),
+                    base::RetainedRef(response_code)),
                 /*progress_callback=*/base::DoNothing(),
                 base::BindOnce(
                     [](base::Time now,
                        scoped_refptr<PersistedData> persisted_data,
                        HttpRequestCallback callback,
-                       std::unique_ptr<std::optional<int>> response_code,
+                       base::RefCountedData<std::optional<int>>* response_code,
                        std::unique_ptr<update_client::NetworkFetcher> fetcher,
                        std::optional<std::string> response_body, int net_error,
                        const std::string& header_etag,
@@ -183,8 +185,9 @@
                       if (net_error) {
                         VLOG(1)
                             << "Upload failed due to net error " << net_error;
-                        VLOG_IF(1, response_code->has_value())
-                            << "HTTP response code: " << response_code->value();
+                        VLOG_IF(1, response_code->data.has_value())
+                            << "HTTP response code: "
+                            << response_code->data.value();
                         base::SequencedTaskRunner::GetCurrentDefault()
                             ->PostTask(
                                 FROM_HERE,
@@ -199,11 +202,11 @@
                       }
                       base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
                           FROM_HERE,
-                          base::BindOnce(std::move(callback), *response_code,
-                                         response_body));
+                          base::BindOnce(std::move(callback),
+                                         response_code->data, response_body));
                     },
                     now, configurator->GetUpdaterPersistedData(),
-                    std::move(callback), std::move(response_code),
+                    std::move(callback), base::RetainedRef(response_code),
                     std::move(fetcher)));
           },
           configurator_, event_logging_url_, request_body, clock_->Now(),
diff --git a/chrome/updater/event_logger_unittest.cc b/chrome/updater/event_logger_unittest.cc
index 88b4cddd..470401c 100644
--- a/chrome/updater/event_logger_unittest.cc
+++ b/chrome/updater/event_logger_unittest.cc
@@ -8,9 +8,11 @@
 #include <memory>
 #include <string>
 
+#include "base/containers/flat_map.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
+#include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/strings/strcat.h"
@@ -376,4 +378,145 @@
   EXPECT_EQ(persisted_data_->GetRemoteLoggingCookie(), logging_cookie);
 }
 
+namespace {
+
+class FakeNetworkFetcher : public update_client::NetworkFetcher {
+ public:
+  explicit FakeNetworkFetcher(base::OnceClosure on_post_request)
+      : on_post_request_(std::move(on_post_request)) {}
+  ~FakeNetworkFetcher() override = default;
+
+  void PostRequest(
+      const GURL& url,
+      const std::string& post_data,
+      const std::string& content_type,
+      const base::flat_map<std::string, std::string>& post_additional_headers,
+      ResponseStartedCallback response_started_callback,
+      ProgressCallback progress_callback,
+      PostRequestCompleteCallback post_request_complete_callback) override {
+    response_started_callback_ = std::move(response_started_callback);
+    post_request_complete_callback_ = std::move(post_request_complete_callback);
+    if (on_post_request_) {
+      std::move(on_post_request_).Run();
+    }
+  }
+
+  base::OnceClosure DownloadToFile(
+      const GURL& url,
+      const base::FilePath& file_path,
+      ResponseStartedCallback response_started_callback,
+      ProgressCallback progress_callback,
+      DownloadToFileCompleteCallback download_to_file_complete_callback)
+      override {
+    return base::DoNothing();
+  }
+
+  const ResponseStartedCallback& response_started_callback() const {
+    return response_started_callback_;
+  }
+
+  PostRequestCompleteCallback TakeCompleteCallback() {
+    return std::move(post_request_complete_callback_);
+  }
+
+ private:
+  ResponseStartedCallback response_started_callback_;
+  PostRequestCompleteCallback post_request_complete_callback_;
+  base::OnceClosure on_post_request_;
+};
+
+class FakeNetworkFetcherFactory : public update_client::NetworkFetcherFactory {
+ public:
+  explicit FakeNetworkFetcherFactory(
+      std::unique_ptr<update_client::NetworkFetcher> fetcher)
+      : fetcher_(std::move(fetcher)) {}
+
+  std::unique_ptr<update_client::NetworkFetcher> Create() const override {
+    return std::move(fetcher_);
+  }
+
+ protected:
+  ~FakeNetworkFetcherFactory() override = default;

diff --git a/chrome/updater/event_logger_unittest.cc b/chrome/updater/event_logger_unittest.cc
index 88b4cddd..470401c 100644
--- a/chrome/updater/event_logger_unittest.cc
+++ b/chrome/updater/event_logger_unittest.cc
@@ -8,9 +8,11 @@
 #include <memory>
 #include <string>
 
+#include "base/containers/flat_map.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
+#include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/strings/strcat.h"
@@ -376,4 +378,145 @@
   EXPECT_EQ(persisted_data_->GetRemoteLoggingCookie(), logging_cookie);
 }
 
+namespace {
+
+class FakeNetworkFetcher : public update_client::NetworkFetcher {
+ public:
+  explicit FakeNetworkFetcher(base::OnceClosure on_post_request)
+      : on_post_request_(std::move(on_post_request)) {}
+  ~FakeNetworkFetcher() override = default;
+
+  void PostRequest(
+      const GURL& url,
+      const std::string& post_data,
+      const std::string& content_type,
+      const base::flat_map<std::string, std::string>& post_additional_headers,
+      ResponseStartedCallback response_started_callback,
+      ProgressCallback progress_callback,
+      PostRequestCompleteCallback post_request_complete_callback) override {
+    response_started_callback_ = std::move(response_started_callback);
+    post_request_complete_callback_ = std::move(post_request_complete_callback);
+    if (on_post_request_) {
+      std::move(on_post_request_).Run();
+    }
+  }
+
+  base::OnceClosure DownloadToFile(
+      const GURL& url,
+      const base::FilePath& file_path,
+      ResponseStartedCallback response_started_callback,
+      ProgressCallback progress_callback,
+      DownloadToFileCompleteCallback download_to_file_complete_callback)
+      override {
+    return base::DoNothing();
+  }
+
+  const ResponseStartedCallback& response_started_callback() const {
+    return response_started_callback_;
+  }
+
+  PostRequestCompleteCallback TakeCompleteCallback() {
+    return std::move(post_request_complete_callback_);
+  }
+
+ private:
+  ResponseStartedCallback response_started_callback_;
+  PostRequestCompleteCallback post_request_complete_callback_;
+  base::OnceClosure on_post_request_;
+};
+
+class FakeNetworkFetcherFactory : public update_client::NetworkFetcherFactory {
+ public:
+  explicit FakeNetworkFetcherFactory(
+      std::unique_ptr<update_client::NetworkFetcher> fetcher)
+      : fetcher_(std::move(fetcher)) {}
+
+  std::unique_ptr<update_client::NetworkFetcher> Create() const override {
+    return std::move(fetcher_);
+  }
+
+ protected:
+  ~FakeNetworkFetcherFactory() override = default;
+
+ private:
+  mutable std::unique_ptr<update_client::NetworkFetcher> fetcher_;
+};
+
+class TestConfigurator : public Configurator {
+ public:
+  TestConfigurator(scoped_refptr<UpdaterPrefs> prefs,
+                   scoped_refptr<ExternalConstants> external_constants,
+                   UpdaterScope scope,
+                   scoped_refptr<update_client::NetworkFetcherFactory>
+                       network_fetcher_factory)
+      : Configurator(prefs, external_constants, scope),
+        network_fetcher_factory_(network_fetcher_factory) {}
+
+  scoped_refptr<update_client::NetworkFetcherFactory> GetNetworkFetcherFactory()
+      override {
+    return network_fetcher_factory_;
+  }
+
+ protected:
+  ~TestConfigurator() override = default;
+
+ private:
+  friend class base::RefCountedThreadSafe<TestConfigurator>;
+  scoped_refptr<update_client::NetworkFetcherFactory> network_fetcher_factory_;
+};
+
+}  // namespace
+
+TEST(EventLoggerOutOfOrderTest, CallbacksOutOfOrder) {
+  base::test::TaskEnvironment task_environment;
+  base::RunLoop post_request_run_loop;
+  base::RunLoop complete_run_loop;
+
+  auto fetcher =
+      std::make_unique<FakeNetworkFetcher>(post_request_run_loop.QuitClosure());
+  FakeNetworkFetcher* fetcher_ptr = fetcher.get();
+
+  auto pref = std::make_unique<TestingPrefServiceSimple>();
+  update_client::RegisterPrefs(pref->registry());
+  RegisterPersistedDataPrefs(pref->registry());
+  auto configurator = base::MakeRefCounted<TestConfigurator>(
+      base::MakeRefCounted<UpdaterPrefsImpl>(
+          /*prefs_dir=*/base::FilePath(), /*lock=*/nullptr, std::move(pref)),
+      CreateExternalConstants(), GetUpdaterScopeForTesting(),
+      base::MakeRefCounted<FakeNetworkFetcherFactory>(std::move(fetcher)));
+
+  auto delegate = std::make_unique<RemoteLoggingDelegate>(
+      GetUpdaterScopeForTesting(), GURL("https://example.com/event-logging"),
+      /*is_cloud_managed=*/false, configurator,
+      std::make_unique<base::SimpleTestClock>());
+
+  delegate->DoPostRequest(
+      "request body",
+      base::BindLambdaForTesting([&](std::optional<int> http_status,
+                                     std::optional<std::string> response_body) {
+        complete_run_loop.Quit();
+      }));
+
+  // Wait for PostRequest to be called on sequence.
+  post_request_run_loop.Run();
+
+  ASSERT_NE(fetcher_ptr, nullptr);
+  auto response_started = fetcher_ptr->response_started_callback();
+  auto complete = fetcher_ptr->TakeCompleteCallback();
+
+  ASSERT_FALSE(response_started.is_null());
+  ASSERT_FALSE(complete.is_null());
+
+  // Simulate out-of-order execution: complete first, releasing RefCountedData
+  // ownership in once callback.
+  std::move(complete).Run("response body", 0, "etag", "proof", "cookie", 0);
+
+  // Wait for the completion callback task to execute.
+  complete_run_loop.Run();
+
+  // Then start response. Under the old unique_ptr behavior, this would have
+  // dereferenced a dangling pointer and crashed.
+  response_started.Run(200, 100);
+}
+
 }  // namespace updater