Medium chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactInsufficient policy enforcement in Chrome for iOS
DescriptionInsufficient policy enforcement in Chrome for iOS
ComponentChrome for iOS
Bug ClassLogic Error
Tracker522074033
Fix commita08a9cc75ee3 (chromium/src) +86/-3
CISA KEVNot listed
CreditedGoogle
Disclosed2026-07-29

Changed Functions

FunctionChangeNotes
if
ios/chrome/browser/download/model/download_manager_tab_helper.mm
modified
ASSERT_TRUE
ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
modified

Files Changed

  • ios/chrome/browser/download/model/download_manager_tab_helper.h
  • ios/chrome/browser/download/model/download_manager_tab_helper.mm
  • ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
From a08a9cc75ee31621c76e2e651e801b7b4b4a7d2a Mon Sep 17 00:00:00 2001
From: Quentin Pubert <[email protected]>
Date: Mon, 22 Jun 2026 06:46:15 -0700
Subject: [PATCH] [iOS] Bind originating task to download scan callback

DownloadManagerTabHelper binds MaybeMoveDownloadToDownloadsDirectory as
the completion callback for content scanning, but the tab helper holds a
single task that can be replaced while a warning dialog is open. If that
happens the callback acts on the new download instead of the one that
was scanned.

Bind a WeakPtr to the originating DownloadTask into the callback and
ignore the result when it no longer matches the current download.

Fixed: 522074033
Change-Id: Ia8a726800c00440c94a8d77f68e3e52032aa0c4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7964804
Reviewed-by: Olivier Robin <[email protected]>
Auto-Submit: Quentin Pubert <[email protected]>
Commit-Queue: Quentin Pubert <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1650279}
---

diff --git a/ios/chrome/browser/download/model/download_manager_tab_helper.h b/ios/chrome/browser/download/model/download_manager_tab_helper.h
index 9d3531fe..6cab1ed0 100644
--- a/ios/chrome/browser/download/model/download_manager_tab_helper.h
+++ b/ios/chrome/browser/download/model/download_manager_tab_helper.h
@@ -136,8 +136,11 @@
   void MaybeSetDownloadPathForAutoDeletion();
 
   // Move the download to user selected location if `shouldProceed` is set as
-  // true, otherwise clean up the current download task.
-  void MaybeMoveDownloadToDownloadsDirectory(bool shouldProceed);
+  // true, otherwise clean up the current download task. The result is ignored
+  // if `task` no longer matches the current download.
+  void MaybeMoveDownloadToDownloadsDirectory(
+      base::WeakPtr<web::DownloadTask> task,
+      bool shouldProceed);
 
   // Process the complete download task. Move the download item to the user
   // selected location if it's not to be saved to google drive, otherwise stop
diff --git a/ios/chrome/browser/download/model/download_manager_tab_helper.mm b/ios/chrome/browser/download/model/download_manager_tab_helper.mm
index 34edd3f9..316f38b4 100644
--- a/ios/chrome/browser/download/model/download_manager_tab_helper.mm
+++ b/ios/chrome/browser/download/model/download_manager_tab_helper.mm
@@ -384,7 +384,13 @@
 }
 
 void DownloadManagerTabHelper::MaybeMoveDownloadToDownloadsDirectory(
+    base::WeakPtr<web::DownloadTask> task,
     bool shouldProceed) {
+  // Ignore the result if it does not correspond to the current download.
+  if (!task || task.get() != task_.get()) {
+    return;
+  }
+
   if (!shouldProceed) {
     CleanupCurrentDownload();
     return;
@@ -445,7 +451,7 @@
           enterprise_connectors::TriggerType::kSavePrompt,
           base::BindOnce(
               &DownloadManagerTabHelper::MaybeMoveDownloadToDownloadsDirectory,
-              weak_ptr_factory_.GetWeakPtr())));
+              weak_ptr_factory_.GetWeakPtr(), task_->GetWeakPtr())));
 
   // Send the download file for enterprise DLP download content scanning.
   files_request_handler_ = std::make_unique<
diff --git a/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm b/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
index c00c8d6d..4484b0e3 100644
--- a/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
+++ b/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
@@ -130,6 +130,12 @@
     return !!tab_helper()->content_analysis_info_;
   }
 
+  void MaybeMoveDownloadToDownloadsDirectory(
+      base::WeakPtr<web::DownloadTask> task,
+      bool should_proceed) {
+    tab_helper()->MaybeMoveDownloadToDownloadsDirectory(task, should_proceed);
+  }
+
   // Creates a fake download task associated with `web_state_`.
   std::unique_ptr<web::FakeDownloadTask> CreateFakeDownloadTask(
       const GURL& original_url,
@@ -538,3 +544,71 @@
 
   EXPECT_FALSE(tab_helper()->IsScannerProcessing());
 }
+
+// Tests that a scan completion callback bound to a previous download is ignored
+// once that download has been replaced by a new one.
+TEST_F(DownloadManagerTabHelperTest,
+       StaleScanCallbackIgnoredAfterDownloadReplaced) {
+  web_state_->WasShown();
+
+  // Set the first download as the current task.
+  std::unique_ptr<web::FakeDownloadTask> first_task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  first_task->SetDone(true);
+  base::WeakPtr<web::DownloadTask> first_task_weak = first_task->GetWeakPtr();
+  tab_helper()->SetCurrentDownload(std::move(first_task));
+
+  // Replace the first download with a second one.
+  std::unique_ptr<web::FakeDownloadTask> second_task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  web::FakeDownloadTask* second_task_ptr = second_task.get();
+  second_task_ptr->SetIdentifier(@"second_id");
+  second_task_ptr->SetGeneratedFileName(base::FilePath("second.txt"));
+  tab_helper()->SetCurrentDownload(std::move(second_task));
+  ASSERT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+  ASSERT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+
+  // Simulate the scan completion callback for the first download arriving with
+  // `shouldProceed` set to true. The second download must not be affected.
+  MaybeMoveDownloadToDownloadsDirectory(first_task_weak, true);
+  ASSERT_TRUE(base::test::RunUntil([&]() {
+    return tab_helper()->GetDownloadTaskFinalFilePath().empty() &&
+           tab_helper()->GetActiveDownloadTask() == second_task_ptr;
+  }));
+  EXPECT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+  EXPECT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+
+  // Simulate the scan completion callback for the first download arriving with
+  // `shouldProceed` set to false. The second download must not be cleaned up.
+  MaybeMoveDownloadToDownloadsDirectory(first_task_weak, false);
+  EXPECT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+}
+
+// Tests that a scan completion callback bound to a previous download is ignored
+// once the current download has been cleared.
+TEST_F(DownloadManagerTabHelperTest,
+       StaleScanCallbackIgnoredAfterDownloadCleared) {
+  web_state_->WasShown();
+
+  // Set a download as the current task.
+  std::unique_ptr<web::FakeDownloadTask> task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  task->SetDone(true);
+  base::WeakPtr<web::DownloadTask> task_weak = task->GetWeakPtr();
+  tab_helper()->SetCurrentDownload(std::move(task));
+
+  // Clear the current download.
+  tab_helper()->CleanupCurrentDownload();
+  ASSERT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+
+  // Simulate the scan completion callback arriving with `shouldProceed` set to
+  // true. It must be ignored and not cause crashes or unexpected state.
+  MaybeMoveDownloadToDownloadsDirectory(task_weak, true);
+  EXPECT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+  EXPECT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+
+  // Simulate the scan completion callback arriving with `shouldProceed` set to
+  // false. It must be ignored.
+  MaybeMoveDownloadToDownloadsDirectory(task_weak, false);
+  EXPECT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+}
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm b/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
index c00c8d6d..4484b0e3 100644
--- a/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
+++ b/ios/chrome/browser/download/model/download_manager_tab_helper_unittest.mm
@@ -130,6 +130,12 @@
     return !!tab_helper()->content_analysis_info_;
   }
 
+  void MaybeMoveDownloadToDownloadsDirectory(
+      base::WeakPtr<web::DownloadTask> task,
+      bool should_proceed) {
+    tab_helper()->MaybeMoveDownloadToDownloadsDirectory(task, should_proceed);
+  }
+
   // Creates a fake download task associated with `web_state_`.
   std::unique_ptr<web::FakeDownloadTask> CreateFakeDownloadTask(
       const GURL& original_url,
@@ -538,3 +544,71 @@
 
   EXPECT_FALSE(tab_helper()->IsScannerProcessing());
 }
+
+// Tests that a scan completion callback bound to a previous download is ignored
+// once that download has been replaced by a new one.
+TEST_F(DownloadManagerTabHelperTest,
+       StaleScanCallbackIgnoredAfterDownloadReplaced) {
+  web_state_->WasShown();
+
+  // Set the first download as the current task.
+  std::unique_ptr<web::FakeDownloadTask> first_task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  first_task->SetDone(true);
+  base::WeakPtr<web::DownloadTask> first_task_weak = first_task->GetWeakPtr();
+  tab_helper()->SetCurrentDownload(std::move(first_task));
+
+  // Replace the first download with a second one.
+  std::unique_ptr<web::FakeDownloadTask> second_task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  web::FakeDownloadTask* second_task_ptr = second_task.get();
+  second_task_ptr->SetIdentifier(@"second_id");
+  second_task_ptr->SetGeneratedFileName(base::FilePath("second.txt"));
+  tab_helper()->SetCurrentDownload(std::move(second_task));
+  ASSERT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+  ASSERT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+
+  // Simulate the scan completion callback for the first download arriving with
+  // `shouldProceed` set to true. The second download must not be affected.
+  MaybeMoveDownloadToDownloadsDirectory(first_task_weak, true);
+  ASSERT_TRUE(base::test::RunUntil([&]() {
+    return tab_helper()->GetDownloadTaskFinalFilePath().empty() &&
+           tab_helper()->GetActiveDownloadTask() == second_task_ptr;
+  }));
+  EXPECT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+  EXPECT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+
+  // Simulate the scan completion callback for the first download arriving with
+  // `shouldProceed` set to false. The second download must not be cleaned up.
+  MaybeMoveDownloadToDownloadsDirectory(first_task_weak, false);
+  EXPECT_EQ(second_task_ptr, tab_helper()->GetActiveDownloadTask());
+}
+
+// Tests that a scan completion callback bound to a previous download is ignored
+// once the current download has been cleared.
+TEST_F(DownloadManagerTabHelperTest,
+       StaleScanCallbackIgnoredAfterDownloadCleared) {
+  web_state_->WasShown();
+
+  // Set a download as the current task.
+  std::unique_ptr<web::FakeDownloadTask> task =
+      CreateFakeDownloadTask(GURL(kUrl), kMimeType);
+  task->SetDone(true);
+  base::WeakPtr<web::DownloadTask> task_weak = task->GetWeakPtr();
+  tab_helper()->SetCurrentDownload(std::move(task));
+
+  // Clear the current download.
+  tab_helper()->CleanupCurrentDownload();
+  ASSERT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+
+  // Simulate the scan completion callback arriving with `shouldProceed` set to
+  // true. It must be ignored and not cause crashes or unexpected state.
+  MaybeMoveDownloadToDownloadsDirectory(task_weak, true);
+  EXPECT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+  EXPECT_TRUE(tab_helper()->GetDownloadTaskFinalFilePath().empty());
+
+  // Simulate the scan completion callback arriving with `shouldProceed` set to
+  // false. It must be ignored.
+  MaybeMoveDownloadToDownloadsDirectory(task_weak, false);
+  EXPECT_EQ(nullptr, tab_helper()->GetActiveDownloadTask());
+}
Loading diff…

Original Bug Report

reported by [email protected]

Potential enterprise scanning bypass in DownloadManagerTabHelper via race condition

Flapjack, 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. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.

Overview: A logic error in DownloadManagerTabHelper on iOS allows an attacker to bypass enterprise content scanning. If a new download is initiated while a scan warning dialog for a previous download is open, the callback processes the new download instead. This allows the second file to be saved to the device without being fully scanned, bypassing enterprise policies.

Affected files:

  • ios/chrome/browser/download/model/download_manager_tab_helper.mm

Estimated timestamp from git blame: 2026-03-23

Summary

A potential security bypass vulnerability exists in the enterprise content scanning implementation for downloads on iOS. The DownloadManagerTabHelper fails to correctly associate asynchronous scan results with the specific download task that initiated the scan. By exploiting a race condition, an attacker can substitute a malicious payload while a warning dialog is open, causing the payload to be saved to the device without completing a scan.

Vulnerability Details

When a download completes and requires enterprise scanning, DownloadManagerTabHelper::ProcessCompleteDownloadTask initiates the scan. It provides a completion callback, MaybeMoveDownloadToDownloadsDirectory, which is bound to a WeakPtr of the DownloadManagerTabHelper instance.

// ios/chrome/browser/download/model/download_manager_tab_helper.mm
void DownloadManagerTabHelper::ProcessCompleteDownloadTask() {
  // ...
  auto files_request_handler_delegate = std::make_unique<
      enterprise_connectors::FilesRequestHandlerIOS>(
      profile, task_->GetResponsePath(),
      base::BindOnce(
          &enterprise_connectors::HandleScanDecision, web_state_->GetWeakPtr(),
          enterprise_connectors::TriggerType::kSavePrompt,
          base::BindOnce(
              &DownloadManagerTabHelper::MaybeMoveDownloadToDownloadsDirectory,
              weak_ptr_factory_.GetWeakPtr())));
  // ...
}

The issue is that this callback does not capture the specific DownloadTask instance. DownloadManagerTabHelper only manages one active task_ at a time. If a new download starts, DidCreateDownload calls CleanupCurrentDownload(), resetting the task_ and files_request_handler_, and then assigns the new task. However, the modal warning dialog shown for the first task remains open.

When the user eventually interacts with the dialog, MaybeMoveDownloadToDownloadsDirectory executes and operates entirely on the current task_ and files_request_handler_.

Potential Exploitation Steps

An attacker could potentially exploit this using the following steps:

  1. A malicious script initiates a download of File A, designed to trigger a WARNING verdict from the enterprise scanner.
  2. The user sees a modal warning dialog asking them to “Proceed” or “Cancel”.
  3. While the dialog is open, the script initiates a second download, File B (the actual malicious payload). File B replaces File A as the active task_ in the tab helper.
  4. File B is served from a fast server so its local download finishes quickly, and ProcessCompleteDownloadTask is called for File B, creating a new files_request_handler_ to start its scan.
  5. The user clicks “Proceed” on the original dialog for File A.
  6. The callback executes MaybeMoveDownloadToDownloadsDirectory(true).
  7. The code calls files_request_handler_->ReportWarningBypass(...). Since File B hasn’t reached a warning state yet, this returns early without crashing.
  8. The code resolves the file path using the current task_ (File B) and moves File B to the permanent user downloads directory.
  9. The callback calls files_request_handler_.reset(), which immediately cancels File B’s active scan.

The timing is important: File B must have finished its local download and entered the scanning phase (so files_request_handler_ is non-null) before the user clicks “Proceed”. If files_request_handler_ is null, the browser will crash at line 394 due to a null pointer dereference.

Proposed Fix

The completion callback provided to the scanner should be bound with a specific identifier (e.g., the DownloadTask pointer or a unique ID) representing the task that was originally scanned.

MaybeMoveDownloadToDownloadsDirectory should be updated to verify that the current task_ matches the task associated with the callback. If they do not match (meaning the download was replaced), the callback should abort the operation instead of moving the new, unscanned file.

Evaluated with Chrome root at commit: 2155cb00003ec35716a76ed3246eae995f87b7ff


Results so far have been promising, but there can be wrong deductions. Feel free to adjust as follows:

  • If you are familiar with the severity guidelines, you may adjust the severity.
  • If this is a false positive, and there’s no work to be done, please close as WAI.
  • If there is work to do here but not a vulnerability, please change the issue type to Task/Bug/FR.

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.

View on issue tracker