CVE-2026-8522
Overview
Changed Functions
| Function | Change | Notes |
|---|---|---|
ifios/chrome/browser/download/coordinator/download_manager_coordinator.mm |
modified |
Files Changed
ios/chrome/browser/download/coordinator/download_manager_coordinator.hios/chrome/browser/download/coordinator/download_manager_coordinator.mm
Patch
From 31a488274dd4b1f524f3dd1de814cc669b1164a4 Mon Sep 17 00:00:00 2001 From: Quentin Pubert <[email protected]> Date: Mon, 20 Apr 2026 08:48:22 -0700 Subject: [PATCH] [iOS] Replace DownloadTask* with WeakPtr in DownloadManagerCoordinator This CL addresses a potential use-after-free by replacing the property ``` @property(nonatomic) web::DownloadTask* downloadTask; ``` in DownloadManagerCoordinator with instead the following ``` @property(nonatomic, assign) base::WeakPtr<web::DownloadTask> downloadTask; ``` so that the weak pointer factory of DownloadTask will now be responsible for cleaning up `downloadTask`. Since the coordinator used to rely on a dangling pointer to the previous download task when it was being replaced by a new task, the tab helper is also refactored so it always informs its delegate (the coordinator) when the task is cleaned up. `ScheduleTaskDestruction` is removed since cleanup only needs to be postponed when it is done from the download task observer method. Bug: 504185107 Change-Id: If35e2ec631e34cfedb52cf025f5a30548f3dcb7e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7776871 Reviewed-by: Ewann Pellé <[email protected]> Commit-Queue: Quentin Pubert <[email protected]> Auto-Submit: Quentin Pubert <[email protected]> Cr-Commit-Position: refs/heads/main@{#1617516} --- diff --git a/ios/chrome/browser/download/coordinator/download_manager_coordinator.h b/ios/chrome/browser/download/coordinator/download_manager_coordinator.h index 97193f3..4930be61 100644 --- a/ios/chrome/browser/download/coordinator/download_manager_coordinator.h +++ b/ios/chrome/browser/download/coordinator/download_manager_coordinator.h @@ -5,6 +5,7 @@ #ifndef IOS_CHROME_BROWSER_DOWNLOAD_COORDINATOR_DOWNLOAD_MANAGER_COORDINATOR_H_ #define IOS_CHROME_BROWSER_DOWNLOAD_COORDINATOR_DOWNLOAD_MANAGER_COORDINATOR_H_ +#import "base/memory/weak_ptr.h" #import "ios/chrome/browser/download/model/download_manager_tab_helper_delegate.h" #import "ios/chrome/browser/shared/coordinator/chrome_coordinator/chrome_coordinator.h" @@ -26,7 +27,7 @@ // Download Manager supports only one download task at a time. Set to null when // stop method is called. -@property(nonatomic) web::DownloadTask* downloadTask; +@property(nonatomic, assign) base::WeakPtr<web::DownloadTask> downloadTask; // Underlying UIViewController presented by this coordinator. @property(nonatomic, readonly) UIViewController* viewController; diff --git a/ios/chrome/browser/download/coordinator/download_manager_coordinator.mm b/ios/chrome/browser/download/coordinator/download_manager_coordinator.mm index f465478..efd25076 100644 --- a/ios/chrome/browser/download/coordinator/download_manager_coordinator.mm +++ b/ios/chrome/browser/download/coordinator/download_manager_coordinator.mm @@ -19,6 +19,7 @@ #import "base/metrics/histogram_functions.h" #import "base/metrics/user_metrics.h" #import "base/metrics/user_metrics_action.h" +#import "base/not_fatal_until.h" #import "base/strings/stringprintf.h" #import "base/strings/sys_string_conversions.h" #import "base/strings/utf_string_conversions.h" @@ -106,7 +107,7 @@ @implementation DownloadManagerCoordinator - (void)dealloc { - DCHECK(_stopped); + CHECK(_stopped, base::NotFatalUntil::M150); } - (void)start { @@ -115,8 +116,8 @@ // Similar to start but can be called after pause. - (void)restart { - DCHECK(self.presenter); - DCHECK(self.browser); + CHECK(self.presenter, base::NotFatalUntil::M150); + CHECK(self.browser, base::NotFatalUntil::M150); if (IsGeminiCopresenceEnabled()) { _geminiHandler = HandlerForProtocol(self.browser->GetCommandDispatcher(), BWGCommands); @@ -159,7 +160,7 @@ DownloadRecordServiceFactory::GetForProfile(profile)); } - _mediator.SetDownloadTask(_downloadTask); + _mediator.SetDownloadTask(_downloadTask.get()); _mediator.SetConsumer(_viewController); if (base::FeatureList::IsEnabled(kIOSDownloadNoUIUpdateInBackground)) { _mediator.StartObservingNotifications(); @@ -236,7 +237,7 @@ } BOOL replacingExistingDownload = _downloadTask ? YES : NO; - _downloadTask = download; + _downloadTask = download->GetWeakPtr(); if (web::GetWebClient()->EnableFullscreenAPI()) { // Exit fullscreen since download UI will be behind fullscreen mode. @@ -245,7 +246,7 @@ } if (replacingExistingDownload) { - _mediator.SetDownloadTask(_downloadTask); + _mediator.SetDownloadTask(_downloadTask.get()); } else { self.animatesPresentation = YES; [self restart]; @@ -285,7 +286,7 @@ - (void)downloadManagerTabHelper:(DownloadManagerTabHelper*)tabHelper didHideDownload:(web::DownloadTask*)download animated:(BOOL)animated { - DCHECK_EQ(_downloadTask, download); + CHECK_EQ(_downloadTask.get(), download, base::NotFatalUntil::M150); self.animatesPresentation = animated; [self stop]; self.animatesPresentation = YES; @@ -294,8 +295,8 @@ - (void)downloadManagerTabHelper:(DownloadManagerTabHelper*)tabHelper didShowDownload:(web::DownloadTask*)download animated:(BOOL)animated { - DCHECK_NE(_downloadTask, download); - _downloadTask = download; + CHECK_NE(_downloadTask.get(), download, base::NotFatalUntil::M150); + _downloadTask = download->GetWeakPtr(); self.animatesPresentation = animated; [self start]; self.animatesPresentation = YES; @@ -309,7 +310,7 @@ // observer is called. return; } - DCHECK_EQ(_downloadTask, download); + CHECK_EQ(_downloadTask.get(), download, base::NotFatalUntil::M150); self.animatesPresentation = NO; [self pause]; self.animatesPresentation = YES; @@ -329,7 +330,7 @@ - (void)downloadManagerTabHelper:(DownloadManagerTabHelper*)tabHelper wantsToStartDownload:(web::DownloadTask*)download { - DCHECK_EQ(_downloadTask, download); + CHECK_EQ(_downloadTask.get(), download, base::NotFatalUntil::M150); [self tryDownload]; } @@ -344,11 +345,11 @@ } - (void)containedPresenterDidPresent:(id<ContainedPresenter>)presenter { - DCHECK(presenter == self.presenter); + CHECK_EQ(presenter, self.presenter, base::NotFatalUntil::M150); } - (void)containedPresenterDidDismiss:(id<ContainedPresenter>)presenter { - DCHECK(presenter == self.presenter); + CHECK_EQ(presenter, self.presenter, base::NotFatalUntil::M150); // The view controller may not be dealloced immediately. presenter.presentedViewController = nil; if (_restartPending) { @@ -394,7 +395,8 @@ } })); - web::WebState* webState = self.downloadTask->GetWebState(); + CHECK(_downloadTask); + web::WebState* webState = _downloadTask->GetWebState(); OverlayRequestQueue::FromWebState(webState, OverlayModality::kWebContentArea) ->AddRequest(std::move(request)); } @@ -413,7 +415,7 @@ } id<SaveToDriveCommands> saveToDriveHandler = HandlerForProtocol( self.browser->GetCommandDispatcher(), SaveToDriveCommands); - [saveToDriveHandler showSaveToDriveForDownload:_downloadTask]; + [saveToDriveHandler showSaveToDriveForDownload:_downloadTask.get()]; } - (void)downloadManagerViewControllerDidRetry:(UIViewController*)controller { @@ -519,6 +521,7 @@ // Attempts to start the current download task, either for the first time or // after one or several previously failed attempts. - (void)tryDownload { + CHECK(_downloadTask); DownloadManagerTabHelper* tabHelper = DownloadManagerTabHelper::FromWebState(_downloadTask->GetWebState()); if (_downloadTask->GetErrorCode() != net::OK) { @@ -528,7 +531,7 @@ base::UserMetricsAction("IOSDownloadStartDownloadToDrive")); } else { base::RecordAction(base::UserMetricsAction("IOSDownloadStartDownload")); - _unopenedDownloads.Add(_downloadTask); + _unopenedDownloads.Add(_downloadTask.get());
Regression Test / PoC
diff --git a/ios/chrome/browser/download/coordinator/download_manager_coordinator_unittest.mm b/ios/chrome/browser/download/coordinator/download_manager_coordinator_unittest.mm
index ee7094c..60dc2356 100644
--- a/ios/chrome/browser/download/coordinator/download_manager_coordinator_unittest.mm
+++ b/ios/chrome/browser/download/coordinator/download_manager_coordinator_unittest.mm
@@ -152,7 +152,7 @@
// DownloadManagerViewController is propertly configured and presented.
TEST_F(DownloadManagerCoordinatorTest, Start) {
auto task = CreateTestTask();
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
[coordinator_ start];
// By default coordinator presents without animation.
@@ -184,7 +184,7 @@
// stale raw pointer).
TEST_F(DownloadManagerCoordinatorTest, Stop) {
auto task = CreateTestTask();
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
[coordinator_ start];
@autoreleasepool {
// Calling -stop will retain and autorelease coordinator_. task_environment_
@@ -204,7 +204,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -250,7 +250,7 @@
webStateIsVisible:YES];
// Verify that coordinator's properties are set up.
- EXPECT_EQ(task.get(), coordinator_.downloadTask);
+ EXPECT_EQ(task.get(), coordinator_.downloadTask.get());
EXPECT_TRUE(coordinator_.animatesPresentation);
// First presentation of Download Manager UI should be animated.
@@ -402,7 +402,7 @@
// cancelled.
TEST_F(DownloadManagerCoordinatorTest, Close) {
auto task = CreateTestTask();
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -439,7 +439,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -526,7 +526,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -569,7 +569,7 @@
auto task = CreateTestTask();
web::DownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
auto web_state = std::make_unique<web::FakeWebState>();
browser_->GetWebStateList()->InsertWebState(std::move(web_state));
[coordinator_ start];
@@ -621,7 +621,7 @@
TEST_F(DownloadManagerCoordinatorTest, CloseInProgressDownload) {
auto task = CreateTestTask();
task->Start(base::FilePath());
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -679,7 +679,7 @@
// Coordinator should present the confirmation dialog.
TEST_F(DownloadManagerCoordinatorTest, DecidePolicyForDownload) {
auto task = CreateTestTask();
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
OverlayRequestQueue* queue = OverlayRequestQueue::FromWebState(
web_state_.get(), OverlayModality::kWebContentArea);
@@ -766,7 +766,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
DownloadManagerViewController* viewController =
@@ -806,7 +806,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
// First download is a failure.
@@ -862,7 +862,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
// Start and immediately fail the download.
@@ -899,7 +899,7 @@
auto task = CreateTestTask();
web::FakeDownloadTask* task_ptr = task.get();
tab_helper()->SetCurrentDownload(std::move(task));
- coordinator_.downloadTask = task_ptr;
+ coordinator_.downloadTask = task_ptr->GetWeakPtr();
[coordinator_ start];
EXPECT_EQ(1U, base_view_controller_.childViewControllers.count);
@@ -936,7 +936,7 @@
// started and nil when stopped.
TEST_F(DownloadManagerCoordinatorTest, ViewController) {
auto task = CreateTestTask();
- coordinator_.downloadTask = task.get();
+ coordinator_.downloadTask = task->GetWeakPtr();
ASSERT_FALSE(coordinator_.viewController);
[coordinator_ start];
Original Bug Report
Potential Use-After-Free of web::DownloadTask in iOS DownloadManagerCoordinator
Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports without the Chrome Security team. Please see go/chrome-ai-generated-security-bugs-faq for more information.
Overview: A potential Use-After-Free vulnerability exists in iOS Chrome’s Download Manager due to a race condition during the dismissal animation of the download UI. A sequence of downloading, closing the UI, downloading again, and closing the tab can result in a dangling raw pointer that bypasses MiraclePtr and is accessed virtually.
Affected files:
ios/chrome/browser/download/coordinator/download_manager_coordinator.mmios/chrome/browser/download/model/download_manager_tab_helper.mmios/chrome/browser/download/coordinator/download_manager_mediator.mmios/chrome/browser/presenters/ui_bundled/vertical_animation_container.mm
Estimated timestamp from git blame: 2025-04-22
Technical Details
A potential Use-After-Free (UAF) vulnerability exists in DownloadManagerCoordinator on iOS. The coordinator manages the lifecycle of the download UI and holds a bare Objective-C pointer to the active web::DownloadTask in the _downloadTask ivar. This pointer is not protected by MiraclePtr (base::raw_ptr).
The vulnerability is triggered by a race condition during the dismissal of the download manager UI:
-
UI Dismissal Initiated: When a user taps ‘Close’ on the download bar,
DownloadManagerCoordinatorcalls-pause. This sets_stopped = YES, clears_downloadTask, and initiates a 200msUIViewanimation inVerticalAnimationContainerto dismiss the UI. -
New Download Arrival: If a new download (D2) arrives while the dismissal animation is still running,
didCreateDownload:is called. It updates the raw_downloadTaskivar to point to D2 and calls-restart. Because_stoppedisYESand the presenter is still displaying the view controller (as the animation is not finished),-restartsets_restartPending = YESand returns early. Crucially, it does not call_mediator.SetDownloadTask(D2). The mediator (which usesraw_ptr<web::DownloadTask>) never receives a pointer to D2. -
Tab Closure: If the tab is closed (e.g., via
window.close()) during this same animation window, theWebStatedetachment process eventually callsDownloadManagerCoordinator::pause. However, because_stoppedis alreadyYES,pausereturns early (ios/chrome/browser/download/coordinator/download_manager_coordinator.mm:186) without clearing the now-populated_downloadTaskor the_restartPendingflag. -
Task Destruction: When the
WebStateis destroyed,DownloadManagerTabHelper::WebStateDestroyedclears itsstd::unique_ptr<web::DownloadTask>, which destroys D2. Since noraw_ptrever held a reference to D2, the object’s BackupRefPtr refcount drops to zero, and the memory is unquarantined. -
Use-After-Free: When the 200ms dismissal animation completes,
containedPresenterDidDismiss:is called. Seeing_restartPendingisYES, it calls-restart, which now proceeds to call_mediator.SetDownloadTask(_downloadTask). This method performs a virtual call (AddObserver) on the freed_downloadTaskobject through its vtable pointer.
Impact
This vulnerability could potentially allow for Remote Code Execution (RCE) in the browser process of iOS Chrome. An attacker who can time a tab closure within the 200ms window after a user closes the download bar can trigger this UAF. By reclaiming the freed memory with controlled data (e.g., via heap spraying during the animation window), the attacker might gain control of the vtable and redirect execution in the unsandboxed browser process.
Potential Reproduction Steps
- Host an attacker-controlled page (T0) that opens a popup (T1).
- In T1, trigger a fast download (D1) to show the download bar.
- Prompt the user to tap the ‘X’ button to close the download bar for D1.
- In T1, set up a script that triggers a second download (D2) and calls
window.close()within the 200ms animation window following the ‘X’ tap. - Upon completion of the animation, the browser will attempt to use the freed
web::DownloadTaskobject, leading to a crash or code execution.
Suggested Fix
- Convert the
_downloadTaskivar inDownloadManagerCoordinatorfrom a bare pointer to abase::raw_ptr<web::DownloadTask>orbase::WeakPtr<web::DownloadTask>to ensure it is protected by MiraclePtr or safely nullified. - In
DownloadManagerCoordinator::pause, do not return early if_stoppedis alreadyYES. Ensure_downloadTaskand_restartPendingare cleared regardless of the_stoppedstate to prevent dangling pointers and stale state.
Evaluated with Chrome root at commit: 7353d249d9cacf9c7218e1d7b8a39cf39c72d646
Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.