From 98cf28d9d42fab272a35163899c7041163c0ab9e Mon Sep 17 00:00:00 2001
From: S Ganesh <[email protected]>
Date: Tue, 19 May 2026 12:45:22 -0700
Subject: [PATCH] winhttp: Safe single-use NetworkFetcher & UAF fixes

Safely resolve Use-After-Free (UAF) and double-free security
vulnerabilities in winhttp::NetworkFetcher by avoiding object reuse,
enforcing a strict single-use contract, and disconnecting callbacks
during destruction.

Specifically:
1. Re-create the winhttp::NetworkFetcher instance in
   WinNetworkFetcherImpl on every Fetch() call, pooled using a
   cached SharedHInternet session.
2. Disconnect the active status callback inside ~NetworkFetcher()
   by calling WinHttpSetStatusCallback with nullptr. This prevents
   asynchronous HANDLE_CLOSING notifications from triggering on a
   deleted NetworkFetcher instance pointer.
3. Enforce a strict single-use constraint in PostRequest and
   DownloadToFile by CHECKing that url_ is empty. This makes the
   API fail-closed by design. Document this constraint in
   network_fetcher.h.
4. Refactor legacy CompleteFetch recursive re-entrancy by directly
   bind-moving fetch_complete_callback_ inside PostTaskAndReply, and
   transferring base::File ownership using std::move().
5. Resolve upstream kToolbarProfileChipResizing duplicate flag
   compiler error in chrome/browser/ui/ui_features.cc/.h.
6. Add CancelledFetcherDestruction and PostRequestTwiceCHECK
   unit tests to verify early cancellation safety and single-use
   enforcements.

Bug: 513737335
Change-Id: Ic26476d6117b3cb1cbc9254a02369c923ff2c397
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7858339
Reviewed-by: Sorin Jianu <[email protected]>
Reviewed-by: Lina Ismail <[email protected]>
Commit-Queue: S Ganesh <[email protected]>
Reviewed-by: Sebastien Lalancette <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1633063}
---

diff --git a/chrome/browser/enterprise/connectors/device_trust/key_management/core/network/fetcher/win_network_fetcher_impl.cc b/chrome/browser/enterprise/connectors/device_trust/key_management/core/network/fetcher/win_network_fetcher_impl.cc
index b5342c8..4833ac5 100644
--- a/chrome/browser/enterprise/connectors/device_trust/key_management/core/network/fetcher/win_network_fetcher_impl.cc
+++ b/chrome/browser/enterprise/connectors/device_trust/key_management/core/network/fetcher/win_network_fetcher_impl.cc
@@ -26,15 +26,15 @@
   // TODO(b/202321214): need to pass in winhttp::ProxyInfo somehow.
   // If specified use it to create an winhttp::ProxyConfiguration instance.
   // Otherwise create an winhttp::AutoProxyConfiguration instance.
-  if (!winhttp_network_fetcher_) {
-    auto proxy_config = base::MakeRefCounted<winhttp::ProxyConfiguration>();
+  auto proxy_config = base::MakeRefCounted<winhttp::ProxyConfiguration>();
+  if (!winhttp_session_) {
     winhttp_session_ = base::MakeRefCounted<winhttp::SharedHInternet>(
         winhttp::CreateSessionHandle(
             L"DeviceTrustKeyManagement", proxy_config->access_type(),
             proxy_config->proxy(), proxy_config->proxy_bypass()));
-    winhttp_network_fetcher_ = base::MakeRefCounted<winhttp::NetworkFetcher>(
-        winhttp_session_, std::move(proxy_config));
   }
+  winhttp_network_fetcher_ = base::MakeRefCounted<winhttp::NetworkFetcher>(
+      winhttp_session_, std::move(proxy_config));
 
   winhttp_network_fetcher_->PostRequest(
       url_, body_, std::string(), headers_,
diff --git a/components/winhttp/network_fetcher.cc b/components/winhttp/network_fetcher.cc
index f906ae8..cc92799 100644
--- a/components/winhttp/network_fetcher.cc
+++ b/components/winhttp/network_fetcher.cc
@@ -75,6 +75,10 @@
 
 NetworkFetcher::~NetworkFetcher() {
   DVLOG(3) << __func__;
+  if (request_handle_.is_valid()) {
+    ::WinHttpSetStatusCallback(request_handle_.get(), nullptr,
+                               WINHTTP_CALLBACK_FLAG_ALL_NOTIFICATIONS, 0);
+  }
 }
 
 void NetworkFetcher::HandleClosing() {
@@ -92,14 +96,17 @@
 
 void NetworkFetcher::CompleteFetch() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!fetch_complete_callback_) {
+    return;
+  }
   if (!file_.IsValid()) {
     std::move(fetch_complete_callback_).Run(response_code_);
     return;
   }
   base::ThreadPool::PostTaskAndReply(
       FROM_HERE, kTaskTraits,
-      base::BindOnce([](base::File& file) { file.Close(); }, std::ref(file_)),
-      base::BindOnce(&NetworkFetcher::CompleteFetch, this));
+      base::BindOnce([](base::File file) { file.Close(); }, std::move(file_)),
+      base::BindOnce(std::move(fetch_complete_callback_), response_code_));
 }
 
 HRESULT NetworkFetcher::QueryHeaderString(const std::wstring& name,
@@ -151,6 +158,7 @@
     FetchProgressCallback fetch_progress_callback,
     FetchCompleteCallback fetch_complete_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  CHECK(url_.is_empty());
 
   url_ = url;
   fetch_started_callback_ = std::move(fetch_started_callback);
@@ -178,6 +186,7 @@
     FetchProgressCallback fetch_progress_callback,
     FetchCompleteCallback fetch_complete_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  CHECK(url_.is_empty());
 
   url_ = url;
   file_path_ = file_path;
diff --git a/components/winhttp/network_fetcher.h b/components/winhttp/network_fetcher.h
index ce8a6945..0feee50 100644
--- a/components/winhttp/network_fetcher.h
+++ b/components/winhttp/network_fetcher.h
@@ -35,6 +35,10 @@
 
 // Implements a network fetcher in terms of WinHTTP. The class is ref-counted
 // as it is accessed from the main sequence and the worker threads in WinHTTP.
+//
+// Each NetworkFetcher instance is strictly single-use and cannot be reused to
+// send multiple requests consecutively or concurrently. Subsequent attempts to
+// start a fetch will trigger a CHECK failure.
 class NetworkFetcher : public base::RefCountedThreadSafe<NetworkFetcher> {
  public:
   using FetchCompleteCallback = base::OnceCallback<void(int response_code)>;
diff --git a/components/winhttp/network_fetcher_unittest.cc b/components/winhttp/network_fetcher_unittest.cc
index 93fa470..df7999a 100644
--- a/components/winhttp/network_fetcher_unittest.cc
+++ b/components/winhttp/network_fetcher_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
+#include "base/test/gtest_util.h"
 #include "base/test/task_environment.h"
 #include "components/winhttp/scoped_hinternet.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -136,4 +137,73 @@
   ASSERT_EQ(network_fetcher->GetResponseBody(), kResponse);
 }
 
+TEST(WinHttpNetworkFetcher, CancelledFetcherDestruction) {
+  net::EmbeddedTestServer test_server;
+  test_server.RegisterRequestHandler(base::BindRepeating(
+      [](const net::test_server::HttpRequest& request)
+          -> std::unique_ptr<net::test_server::HttpResponse> {
+        auto response = std::make_unique<net::test_server::BasicHttpResponse>();
+        response->set_content("stub response");
+        return response;
+      }));
+  ASSERT_TRUE(test_server.Start());
+
+  base::test::TaskEnvironment environment;
+  {
+    auto network_fetcher = base::MakeRefCounted<NetworkFetcher>(
+        base::MakeRefCounted<SharedHInternet>(CreateSessionHandle(
+            L"WinHttpNetworkFetcherTest.CancelledFetcherDestruction",
+            WINHTTP_ACCESS_TYPE_NO_PROXY)),
+        base::MakeRefCounted<ProxyConfiguration>());
+
+    // Initiate a request.
+    network_fetcher->PostRequest(
+        /*url=*/test_server.GetURL("/"),
+        /*content_type=*/"text/plain",
+        /*post_data=*/"request data",
+        /*post_additional_headers=*/{},
+        /*fetch_started_callback=*/base::DoNothing(),
+        /*fetch_progress_callback=*/base::DoNothing(),
+        /*fetch_complete_callback=*/base::DoNothing());
+
+    // Drop the only reference to network_fetcher while the request is
+    // in-flight. This invokes the destructor and tears down the ScopedHInternet
+    // request handle.
+  }
+
+  base::RunLoop run_loop;
+  base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(), base::Milliseconds(100));
+  run_loop.Run();
+}
+
+TEST(WinHttpNetworkFetcher, PostRequestTwiceCHECK) {
+  base::test::TaskEnvironment environment;
+  auto network_fetcher = base::MakeRefCounted<NetworkFetcher>(
+      base::MakeRefCounted<SharedHInternet>(CreateSessionHandle(
+          L"WinHttpNetworkFetcherTest.PostRequestTwiceCHECK",
+          WINHTTP_ACCESS_TYPE_NO_PROXY)),
+      base::MakeRefCounted<ProxyConfiguration>());
+
+  network_fetcher->PostRequest(
+      /*url=*/GURL("http://aurl"),
+      /*content_type=*/"text/plain",
+      /*post_data=*/"request data",
+      /*post_additional_headers=*/{},

diff --git a/components/winhttp/network_fetcher_unittest.cc b/components/winhttp/network_fetcher_unittest.cc
index 93fa470..df7999a 100644
--- a/components/winhttp/network_fetcher_unittest.cc
+++ b/components/winhttp/network_fetcher_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
+#include "base/test/gtest_util.h"
 #include "base/test/task_environment.h"
 #include "components/winhttp/scoped_hinternet.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -136,4 +137,73 @@
   ASSERT_EQ(network_fetcher->GetResponseBody(), kResponse);
 }
 
+TEST(WinHttpNetworkFetcher, CancelledFetcherDestruction) {
+  net::EmbeddedTestServer test_server;
+  test_server.RegisterRequestHandler(base::BindRepeating(
+      [](const net::test_server::HttpRequest& request)
+          -> std::unique_ptr<net::test_server::HttpResponse> {
+        auto response = std::make_unique<net::test_server::BasicHttpResponse>();
+        response->set_content("stub response");
+        return response;
+      }));
+  ASSERT_TRUE(test_server.Start());
+
+  base::test::TaskEnvironment environment;
+  {
+    auto network_fetcher = base::MakeRefCounted<NetworkFetcher>(
+        base::MakeRefCounted<SharedHInternet>(CreateSessionHandle(
+            L"WinHttpNetworkFetcherTest.CancelledFetcherDestruction",
+            WINHTTP_ACCESS_TYPE_NO_PROXY)),
+        base::MakeRefCounted<ProxyConfiguration>());
+
+    // Initiate a request.
+    network_fetcher->PostRequest(
+        /*url=*/test_server.GetURL("/"),
+        /*content_type=*/"text/plain",
+        /*post_data=*/"request data",
+        /*post_additional_headers=*/{},
+        /*fetch_started_callback=*/base::DoNothing(),
+        /*fetch_progress_callback=*/base::DoNothing(),
+        /*fetch_complete_callback=*/base::DoNothing());
+
+    // Drop the only reference to network_fetcher while the request is
+    // in-flight. This invokes the destructor and tears down the ScopedHInternet
+    // request handle.
+  }
+
+  base::RunLoop run_loop;
+  base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(), base::Milliseconds(100));
+  run_loop.Run();
+}
+
+TEST(WinHttpNetworkFetcher, PostRequestTwiceCHECK) {
+  base::test::TaskEnvironment environment;
+  auto network_fetcher = base::MakeRefCounted<NetworkFetcher>(
+      base::MakeRefCounted<SharedHInternet>(CreateSessionHandle(
+          L"WinHttpNetworkFetcherTest.PostRequestTwiceCHECK",
+          WINHTTP_ACCESS_TYPE_NO_PROXY)),
+      base::MakeRefCounted<ProxyConfiguration>());
+
+  network_fetcher->PostRequest(
+      /*url=*/GURL("http://aurl"),
+      /*content_type=*/"text/plain",
+      /*post_data=*/"request data",
+      /*post_additional_headers=*/{},
+      /*fetch_started_callback=*/base::DoNothing(),
+      /*fetch_progress_callback=*/base::DoNothing(),
+      /*fetch_complete_callback=*/base::DoNothing());
+
+  // The second PostRequest call on the same instance must crash the process via
+  // CHECK!
+  EXPECT_CHECK_DEATH(network_fetcher->PostRequest(
+      /*url=*/GURL("http://aurl"),
+      /*content_type=*/"text/plain",
+      /*post_data=*/"request data",
+      /*post_additional_headers=*/{},
+      /*fetch_started_callback=*/base::DoNothing(),
+      /*fetch_progress_callback=*/base::DoNothing(),
+      /*fetch_complete_callback=*/base::DoNothing()));
+}
+
 }  // namespace winhttp