CVE-2026-95333
Overview
Background
- `FcpHttpRequestManager`
- a
private_insightshelper that owns anetwork::SharedURLLoaderFactoryand a map of activeFcpHttpRequestRunnerobjects, and marshals request start/cancel/complete work onto the UI sequence. - `base::Unretained(this)`
- a callback binding that passes a raw
thispointer with no lifetime guarantee, so the callee must be certain the object still exists when the task runs. - `RefCountedDeleteOnSequence`
- a reference-counting base that keeps an object alive until the last
scoped_refptris dropped and then destroys it on a designated task runner (here the UI sequence). - `CountdownLatch`
- a synchronization primitive whose
CountDown()releases a waiter (the FCP client thread) once an in-flight request is accounted for as complete.
Root Cause Analysis
FcpHttpRequestManager posted UI-thread tasks for StartRequestOnUI, request cancellation, and OnRequestComplete while binding itself with base::Unretained(this), relying on an unenforced comment-level invariant that the manager “outlives all network requests initiated through it.” That invariant was violated during service shutdown: when private_insights_service tore down while HTTP requests and their posted UI tasks were still pending, the manager could be destroyed on one sequence while a queued task holding a raw this still ran on the UI sequence, dereferencing freed memory. The old destructor even tried to hand off url_loader_factory_ and runners_ to a UI task at destruction time, which does not protect earlier already-posted callbacks that captured the raw pointer.
The fix makes FcpHttpRequestManager inherit base::RefCountedDeleteOnSequence and replaces every base::Unretained(this) with base::WrapRefCounted(this) (and every raw-pointer member/parameter with scoped_refptr), so each posted task holds a strong reference that keeps the manager alive until the task completes. It additionally reorders OnRequestComplete so the runner is erased before latch->CountDown(), ensuring the waiter is not released until cleanup is done.
base::Unretained(this) for cross-sequence callbacks whose lifetime guarantee existed only as a comment, allowing the manager to be freed before its posted UI tasks ran; the fix converts the manager to reference counting so every outstanding callback co-owns the object.Attack Path
- Trigger concurrent activity
Drive the
private_insightsfeature so thatFcpHttpRequestManagerhas HTTP requests in flight, causingStartRequestOnUI/cancel/OnRequestCompletetasks to be posted to the UI sequence withbase::Unretained(this). - Race a shutdown
Cause the owning
private_insights_serviceto shut down and drop the manager while those UI tasks are still queued or executing, so the manager’s destructor runs concurrently with the pending tasks. - Dangling dereference
A queued UI task runs after the manager is freed and dereferences the raw
this(e.g. touchingrunners_orui_sequence_checker_), producing the use-after-free. (inferred) - Exploit the freed allocation Groom the heap so the freed manager’s memory is reallocated with attacker-influenced contents before the stale task reads or writes through it. (inferred)
Impact Assessment
private_insights HTTP activity and service shutdown gains a use-after-free of FcpHttpRequestManager in the browser process, where these UI-sequence tasks execute. Preconditions include the feature being active with in-flight requests plus a concurrent teardown, i.e. a race window. Memory corruption leading to further escalation is (inferred); the advisory reports only “Use after free in Metrics” with no exploited-in-the-wild claim.Changed Functions
| Function | Change | Notes |
|---|---|---|
FcpHttpRequestManagercomponents/private_insights/fcp_http_client.h |
modified | |
FcpHttpRequestHandlecomponents/private_insights/fcp_http_client.h |
modified | |
FcpHttpClientcomponents/private_insights/fcp_http_client.h |
modified | |
CreateHttpClientcomponents/private_insights/fcp_simple_task_environment.cc |
modified | |
ifcomponents/private_insights/private_insights_service.cc |
modified | |
FcpHttpClientTestcomponents/private_insights/fcp_http_client_unittest.cc |
modified | |
TEST_Fcomponents/private_insights/fcp_http_client_unittest.cc |
modified |
Files Changed
components/private_insights/fcp_http_client.cccomponents/private_insights/fcp_http_client_unittest.cccomponents/private_insights/private_insights_service.cccomponents/private_insights/fcp_http_client.hcomponents/private_insights/fcp_simple_task_environment.hcomponents/private_insights/fcp_simple_task_environment.cc
Audit Directions
- `base::Unretained` across sequencesFlag any
base::Unretained(this)(or captured rawthis) in callbacks posted to a different task runner, especially where the lifetime guarantee is only a code comment rather than an ownership or ref-count mechanism. - Shutdown-vs-inflight racesReview services whose destruction can overlap with pending async work (posted tasks, network callbacks, latches) and confirm the async work holds a strong reference or is reliably cancelled before teardown.
- Comment-enforced lifetimesSearch for classes documented as “must outlive all requests/tasks” and verify that invariant is enforced by
scoped_refptr/WeakPtrrather than assumed by callers.
Patch
From 0bcceab6b42aa3191751a9cdd792655916044da6 Mon Sep 17 00:00:00 2001 From: Kamil Jarosz <[email protected]> Date: Mon, 14 Sep 2026 00:38:53 -0700 Subject: [PATCH] private_insights: Use ref counting for FcpHttpRequestManager This makes the lifetime management easier and fixes a potential use after free when shutting down the service and handling HTTP requests at the same time. Bug: 559470822 Bug: 559682346 Change-Id: Ib7cd78d83395042aa998f1eec89c90db520baad3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8393757 Commit-Queue: Matt Dembski <[email protected]> Reviewed-by: Matt Dembski <[email protected]> Cr-Commit-Position: refs/heads/main@{#1697080} --- diff --git a/components/private_insights/fcp_http_client.cc b/components/private_insights/fcp_http_client.cc index 77411bb..4eed694c 100644 --- a/components/private_insights/fcp_http_client.cc +++ b/components/private_insights/fcp_http_client.cc @@ -330,10 +330,10 @@ } FcpHttpRequestHandle::FcpHttpRequestHandle( - FcpHttpRequestManager* manager, + scoped_refptr<FcpHttpRequestManager> manager, uint64_t request_id, std::unique_ptr<fcp::client::http::HttpRequest> request) - : manager_(manager), + : manager_(std::move(manager)), request_id_(request_id), request_(std::move(request)) {} @@ -356,24 +356,14 @@ FcpHttpRequestManager::FcpHttpRequestManager( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, scoped_refptr<base::SequencedTaskRunner> ui_task_runner) - : ui_task_runner_(std::move(ui_task_runner)), + : base::RefCountedDeleteOnSequence<FcpHttpRequestManager>(ui_task_runner), + ui_task_runner_(std::move(ui_task_runner)), url_loader_factory_(std::move(url_loader_factory)) { DCHECK_CALLED_ON_VALID_SEQUENCE(ui_sequence_checker_); } FcpHttpRequestManager::~FcpHttpRequestManager() { - if (url_loader_factory_ || !runners_.empty()) { - ui_task_runner_->PostTask( - FROM_HERE, - base::BindOnce( - [](scoped_refptr<network::SharedURLLoaderFactory> factory, - std::map<uint64_t, std::unique_ptr<FcpHttpRequestRunner>> - runners) { - // Body is intentionally empty; factory and active runners will be - // destructed here on the UI thread sequence. - }, - std::move(url_loader_factory_), std::move(runners_))); - } + DCHECK_CALLED_ON_VALID_SEQUENCE(ui_sequence_checker_); } void FcpHttpRequestManager::StartRequest( @@ -383,10 +373,7 @@ CountdownLatch* latch) { ui_task_runner_->PostTask( FROM_HERE, base::BindOnce(&FcpHttpRequestManager::StartRequestOnUI, - // Note: using base::Unretained(this) is safe - // because the manager outlives all network - // requests initiated through it. - base::Unretained(this), handle, + base::WrapRefCounted(this), handle, std::move(upload_body), callback, latch)); } @@ -394,16 +381,14 @@ ui_task_runner_->PostTask( FROM_HERE, base::BindOnce( - [](FcpHttpRequestManager* manager, uint64_t req_id) { + [](scoped_refptr<FcpHttpRequestManager> manager, uint64_t req_id) { DCHECK_CALLED_ON_VALID_SEQUENCE(manager->ui_sequence_checker_); auto it = manager->runners_.find(req_id); if (it != manager->runners_.end()) { it->second->Cancel(); } }, - // Note: using base::Unretained(this) is safe because the manager - // outlives all network requests initiated through it. - base::Unretained(this), request_id)); + base::WrapRefCounted(this), request_id)); } void FcpHttpRequestManager::StartRequestOnUI( @@ -417,10 +402,8 @@ url_loader_factory_.get(), handle, std::move(upload_body), callback); FcpHttpRequestRunner* runner_ptr = runner.get(); runner->set_on_complete_callback(base::BindOnce( - &FcpHttpRequestManager::OnRequestComplete, - // Note: using base::Unretained(this) is safe because the manager outlives - // all network requests initiated through it. - base::Unretained(this), request_id, runner_ptr, latch)); + &FcpHttpRequestManager::OnRequestComplete, base::WrapRefCounted(this), + request_id, runner_ptr, latch)); runners_[request_id] = std::move(runner); base::UmaHistogramCounts100(kFcpHttpClientRunnerCountHistogram, runners_.size()); @@ -432,17 +415,18 @@ DCHECK_CALLED_ON_VALID_SEQUENCE(ui_sequence_checker_); DCHECK(runner); - latch->CountDown(); - auto it = runners_.find(request_id); if (it != runners_.end()) { ui_task_runner_->DeleteSoon(FROM_HERE, std::move(it->second)); runners_.erase(it); } + + latch->CountDown(); } -FcpHttpClient::FcpHttpClient(FcpHttpRequestManager* request_manager) - : request_manager_(request_manager) {} +FcpHttpClient::FcpHttpClient( + scoped_refptr<FcpHttpRequestManager> request_manager) + : request_manager_(std::move(request_manager)) {} FcpHttpClient::~FcpHttpClient() = default; diff --git a/components/private_insights/fcp_http_client.h b/components/private_insights/fcp_http_client.h index 1588582..3ad7e08 100644 --- a/components/private_insights/fcp_http_client.h +++ b/components/private_insights/fcp_http_client.h @@ -13,6 +13,7 @@ #include "base/functional/callback.h" #include "base/gtest_prod_util.h" #include "base/memory/raw_ptr.h" +#include "base/memory/ref_counted_delete_on_sequence.h" #include "base/memory/scoped_refptr.h" #include "base/sequence_checker.h" #include "base/thread_annotations.h" @@ -114,10 +115,8 @@ // Wraps scoped_refptr<network::SharedURLLoaderFactory> to ensure that network // requests and factory destruction occur on the UI sequence, while managing // active request runners (FcpHttpRequestRunner) and request cancelation. -// -// WARNING: The caller must ensure that an instance of this class outlives all -// network requests initiated through it, including any posted UI tasks. -class FcpHttpRequestManager { +class FcpHttpRequestManager + : public base::RefCountedDeleteOnSequence<FcpHttpRequestManager> { public: FcpHttpRequestManager( scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory, @@ -126,8 +125,6 @@ FcpHttpRequestManager(const FcpHttpRequestManager&) = delete; FcpHttpRequestManager& operator=(const FcpHttpRequestManager&) = delete; - ~FcpHttpRequestManager(); - // Starts the request on the UI thread. Can be called from any thread. // `latch` must remain valid until all requests complete and `latch->Wait()` // unblocks. @@ -142,6 +139,10 @@ void CancelRequest(uint64_t request_id); private: + friend class base::RefCountedDeleteOnSequence<FcpHttpRequestManager>; + friend class base::DeleteHelper<FcpHttpRequestManager>; + ~FcpHttpRequestManager(); + void StartRequestOnUI(FcpHttpRequestHandle* handle, std::string upload_body, fcp::client::http::HttpRequestCallback* callback, @@ -168,7 +169,7 @@ class FcpHttpRequestHandle : public fcp::client::http::HttpRequestHandle { public: // `manager` must outlive `this`. - FcpHttpRequestHandle(FcpHttpRequestManager* manager, + FcpHttpRequestHandle(scoped_refptr<FcpHttpRequestManager> manager, uint64_t request_id, std::unique_ptr<fcp::client::http::HttpRequest> request); ~FcpHttpRequestHandle() override; @@ -199,7 +200,7 @@ void Cancel() override; private: - raw_ptr<FcpHttpRequestManager> manager_; + scoped_refptr<FcpHttpRequestManager> manager_; uint64_t request_id_; std::unique_ptr<fcp::client::http::HttpRequest> request_; std::unique_ptr<fcp::client::http::HttpResponse> response_; @@ -212,7 +213,7 @@ class FcpHttpClient : public fcp::client::http::HttpClient { public: // The caller must ensure that `request_manager` outlives `this`. - explicit FcpHttpClient(FcpHttpRequestManager* request_manager); + explicit FcpHttpClient(scoped_refptr<FcpHttpRequestManager> request_manager); ~FcpHttpClient() override; FcpHttpClient(const FcpHttpClient&) = delete; @@ -228,7 +229,7 @@ override; private: - raw_ptr<FcpHttpRequestManager> request_manager_; + scoped_refptr<FcpHttpRequestManager> request_manager_; }; } // namespace private_insights diff --git a/components/private_insights/fcp_simple_task_environment.cc b/components/private_insights/fcp_simple_task_environment.cc index 4474838..8cfc6513 100644 --- a/components/private_insights/fcp_simple_task_environment.cc +++ b/components/private_insights/fcp_simple_task_environment.cc @@ -42,7 +42,7 @@ FcpSimpleTaskEnvironment::FcpSimpleTaskEnvironment( std::string base_dir, std::string cache_dir, - std::unique_ptr<FcpHttpRequestManager> http_request_manager, + scoped_refptr<FcpHttpRequestManager> http_request_manager, bool use_attestation_transparency_verifier) : base_dir_(std::move(base_dir)), cache_dir_(std::move(cache_dir)), @@ -73,7 +73,7 @@ std::unique_ptr<fcp::client::http::HttpClient> FcpSimpleTaskEnvironment::CreateHttpClient() { - return std::make_unique<FcpHttpClient>(http_request_manager_.get()); + return std::make_unique<FcpHttpClient>(http_request_manager_); } inline constexpr char kEndorsementOptionsParsingOutcomeHistogram[] = diff --git a/components/private_insights/fcp_simple_task_environment.h b/components/private_insights/fcp_simple_task_environment.h index 1bd5d8a..0052603c 100644 --- a/components/private_insights/fcp_simple_task_environment.h +++ b/components/private_insights/fcp_simple_task_environment.h @@ -23,7 +23,7 @@ FcpSimpleTaskEnvironment( std::string base_dir, std::string cache_dir, - std::unique_ptr<FcpHttpRequestManager> http_request_manager, + scoped_refptr<FcpHttpRequestManager> http_request_manager, bool use_attestation_transparency_verifier); FcpSimpleTaskEnvironment(const FcpSimpleTaskEnvironment&) = delete; @@ -53,7 +53,7 @@ std::string base_dir_; std::string cache_dir_; - std::unique_ptr<FcpHttpRequestManager> http_request_manager_; + scoped_refptr<FcpHttpRequestManager> http_request_manager_; bool use_attestation_transparency_verifier_; fcp::client::ExampleQueryResult result_; diff --git a/components/private_insights/private_insights_service.cc b/components/private_insights/private_insights_service.cc index e4bef2e..d17c8b8c 100644 --- a/components/private_insights/private_insights_service.cc +++ b/components/private_insights/private_insights_service.cc @@ -136,9 +136,9 @@ base::FilePath base_dir = private_insights_dir.AppendASCII("base_dir"); base::FilePath cache_dir = private_insights_dir.AppendASCII("cache_dir"); - std::unique_ptr<FcpHttpRequestManager> http_request_manager; + scoped_refptr<FcpHttpRequestManager> http_request_manager; if (url_loader_factory) { - http_request_manager = std::make_unique<FcpHttpRequestManager>( + http_request_manager = base::MakeRefCounted<FcpHttpRequestManager>( std::move(url_loader_factory), base::SequencedTaskRunner::GetCurrentDefault()); } diff --git a/components/private_insights/fcp_http_client_unittest.cc b/components/private_insights/fcp_http_client_unittest.cc index 9f5b0e60b..1e2de73 100644 --- a/components/private_insights/fcp_http_client_unittest.cc +++ b/components/private_insights/fcp_http_client_unittest.cc @@ -28,8 +28,9 @@ class FcpHttpClientTest : public testing::Test { public: FcpHttpClientTest() - : request_manager_(test_url_loader_factory_.GetSafeWeakWrapper(), - task_environment_.GetMainThreadTaskRunner()) {} + : request_manager_(base::MakeRefCounted<FcpHttpRequestManager>( + test_url_loader_factory_.GetSafeWeakWrapper(), + task_environment_.GetMainThreadTaskRunner())) {} ~FcpHttpClientTest() override = default; protected: @@ -72,16 +73,16 @@ base::test::TaskEnvironment task_environment_; network::TestURLLoaderFactory test_url_loader_factory_; - FcpHttpRequestManager request_manager_; + scoped_refptr<FcpHttpRequestManager> request_manager_; }; TEST_F(FcpHttpClientTest, PerformRequestsEmpty) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); EXPECT_TRUE(client.PerformRequests({}).ok()); } TEST_F(FcpHttpClientTest, PerformRequestsCanceledRequest) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com", fcp::client::http::HttpRequest::Method::kGet, @@ -103,7 +104,7 @@ } TEST_F(FcpHttpClientTest, TotalSentReceivedBytesInitial) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com", fcp::client::http::HttpRequest::Method::kGet, @@ -117,7 +118,7 @@ } TEST_F(FcpHttpClientTest, TotalSentReceivedBytesUpdate) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com", fcp::client::http::HttpRequest::Method::kPost, @@ -214,7 +215,7 @@ }; TEST_F(FcpHttpClientTest, PerformRequestsSuccessfulSingle) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com/test", fcp::client::http::HttpRequest::Method::kGet, @@ -250,7 +251,7 @@ } TEST_F(FcpHttpClientTest, PerformRequestsSuccessfulBatch) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request1 = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com/req1", fcp::client::http::HttpRequest::Method::kGet, @@ -288,7 +289,7 @@ } TEST_F(FcpHttpClientTest, PerformRequestsNetError) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com/failed", fcp::client::http::HttpRequest::Method::kGet, @@ -315,7 +316,7 @@ } TEST_F(FcpHttpClientTest, PerformRequestsReadBodyError) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto handle = client.EnqueueRequest(std::make_unique<ErrorHttpRequest>()); TestHttpRequestCallback callback; @@ -329,7 +330,7 @@ } TEST_F(FcpHttpClientTest, PerformRequestsCancelViaHandle) { - FcpHttpClient client(&request_manager_); + FcpHttpClient client(request_manager_); auto request = fcp::client::http::InMemoryHttpRequest::Create( "https://example.com/pending", fcp::client::http::HttpRequest::Method::kGet,
Regression Test / PoC
diff --git a/components/private_insights/fcp_http_client_unittest.cc b/components/private_insights/fcp_http_client_unittest.cc
index 9f5b0e60b..1e2de73 100644
--- a/components/private_insights/fcp_http_client_unittest.cc
+++ b/components/private_insights/fcp_http_client_unittest.cc
@@ -28,8 +28,9 @@
class FcpHttpClientTest : public testing::Test {
public:
FcpHttpClientTest()
- : request_manager_(test_url_loader_factory_.GetSafeWeakWrapper(),
- task_environment_.GetMainThreadTaskRunner()) {}
+ : request_manager_(base::MakeRefCounted<FcpHttpRequestManager>(
+ test_url_loader_factory_.GetSafeWeakWrapper(),
+ task_environment_.GetMainThreadTaskRunner())) {}
~FcpHttpClientTest() override = default;
protected:
@@ -72,16 +73,16 @@
base::test::TaskEnvironment task_environment_;
network::TestURLLoaderFactory test_url_loader_factory_;
- FcpHttpRequestManager request_manager_;
+ scoped_refptr<FcpHttpRequestManager> request_manager_;
};
TEST_F(FcpHttpClientTest, PerformRequestsEmpty) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
EXPECT_TRUE(client.PerformRequests({}).ok());
}
TEST_F(FcpHttpClientTest, PerformRequestsCanceledRequest) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request =
fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com", fcp::client::http::HttpRequest::Method::kGet,
@@ -103,7 +104,7 @@
}
TEST_F(FcpHttpClientTest, TotalSentReceivedBytesInitial) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request =
fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com", fcp::client::http::HttpRequest::Method::kGet,
@@ -117,7 +118,7 @@
}
TEST_F(FcpHttpClientTest, TotalSentReceivedBytesUpdate) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request =
fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com", fcp::client::http::HttpRequest::Method::kPost,
@@ -214,7 +215,7 @@
};
TEST_F(FcpHttpClientTest, PerformRequestsSuccessfulSingle) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request = fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com/test",
fcp::client::http::HttpRequest::Method::kGet,
@@ -250,7 +251,7 @@
}
TEST_F(FcpHttpClientTest, PerformRequestsSuccessfulBatch) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request1 = fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com/req1",
fcp::client::http::HttpRequest::Method::kGet,
@@ -288,7 +289,7 @@
}
TEST_F(FcpHttpClientTest, PerformRequestsNetError) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request = fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com/failed",
fcp::client::http::HttpRequest::Method::kGet,
@@ -315,7 +316,7 @@
}
TEST_F(FcpHttpClientTest, PerformRequestsReadBodyError) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto handle = client.EnqueueRequest(std::make_unique<ErrorHttpRequest>());
TestHttpRequestCallback callback;
@@ -329,7 +330,7 @@
}
TEST_F(FcpHttpClientTest, PerformRequestsCancelViaHandle) {
- FcpHttpClient client(&request_manager_);
+ FcpHttpClient client(request_manager_);
auto request = fcp::client::http::InMemoryHttpRequest::Create(
"https://example.com/pending",
fcp::client::http::HttpRequest::Method::kGet,