CVE-2025-13634
Overview
Background
- Save Page As / `SaveFileManager`
- the browser-process machinery that writes a page and its parts to disk as
SaveFileitems. It runs on the download task runner and tracks each item bySaveItemId. - `SaveFileCreateInfo::SaveFileSource`
- records where a saved item’s bytes come from.
SAVE_FILE_FROM_NETitems are fetched by aSimpleURLLoaderHelper. Other sources, handled by theelsebranch, are started “manually” because their data does not come from a network loader; the diff’s comment says the data “will come from render process”. - Quarantine / source annotation
SaveFile::AnnotateWithSourceInformation, reached through aquarantine::mojom::Quarantineremote, tags a saved file with its source URL and referrer. This is Mark-of-the-Web style metadata that OS and enterprise (DLP) protections use. It completes inSaveFileManager::OnQuarantineComplete.- `SaveFinished` vs `SaveCompleted`
- after the patch,
SaveFinishedmeans “bytes are written, now quarantine”, andSaveCompleted(the oldSaveFinishedbody) means “report the result to the UI thread viaOnSaveCompleted”.
Root Cause Analysis
Before the patch, quarantine was attached only to the network fetch path. In the save-registration code, remote_quarantine, url, referrer.url and client_guid were bound into save_finished_cb (SaveFileManager::OnURLLoaderComplete) inside the if (save_source == SaveFileCreateInfo::SAVE_FILE_FROM_NET) branch. Only OnURLLoaderComplete called save_file->AnnotateWithSourceInformation. Items started in the else branch (// We manually start the save job.) carried no quarantine information. When they finished, they went straight to the old SaveFinished, which posted OnSaveFinished to the UI thread, so in this code path these files were written to disk without source annotation. The broken rule is that every file Save Page writes should be quarantined before it is reported as complete, whatever its SaveFileSource.
The fix builds a single quarantine_callback (SaveFileManager::QuarantineItem, with URLs blanked when IsOffTheRecord()) before the branch. It stores the callback in SaveFileCreateInfo::quarantine_callback on both branches; on the network branch the callback goes through SimpleURLLoaderHelper and is placed into info there. SaveFinished now runs SaveFile::RunQuarantineCallback() on success. QuarantineItem → OnQuarantineComplete → SaveCompleted then finishes the item, so all sources share one quarantine step.
SAVE_FILE_FROM_NET skipped source annotation. The fix stores the quarantine closure on each item’s SaveFileCreateInfo and runs it from the common SaveFinished path.Attack Path
- Victim saves an attacker page
a user saves an attacker-controlled page with Save Page As, for example
SAVE_PAGE_TYPE_AS_COMPLETE_HTMLas in the updatedSaveDlp-style browser test. - Item uses a non-network source
part of the save, such as content supplied by the renderer, goes through the
elsebranch, which before the patch got noremote_quarantinebinding. - File written without annotation
on completion, the old
SaveFinishedreported success directly, soAnnotateWithSourceInformationnever ran and the file had no source URL or referrer metadata. - Downstream protections skipped OS reputation and Mark-of-the-Web checks, or ChromeOS DLP source-URL rules, would treat the saved file as locally created rather than downloaded from the attacker’s origin (inferred; the patch only shows that annotation was missing).
Impact Assessment
(inferred) and depends on further user action.Changed Functions
| Function | Change | Notes |
|---|---|---|
ifcontent/browser/download/save_file_manager.cc |
modified | |
save_sourcecontent/browser/download/save_types.cc |
modified | |
forchrome/browser/download/save_page_browsertest.cc |
modified | |
IN_PROC_BROWSER_TEST_Fchrome/browser/download/save_page_browsertest.cc |
modified |
Files Changed
chrome/browser/download/save_page_browsertest.cccontent/browser/download/save_file.cccontent/browser/download/save_file.hcontent/browser/download/save_file_manager.cccontent/browser/download/save_file_manager.hcontent/browser/download/save_types.cccontent/browser/download/save_types.h
Audit Directions
- Security steps bound to one source branchlook for quarantine, Safe Browsing or DLP hooks attached to one producer (network loader) instead of the shared sink (file completion). Examples include other save and download sources, MHTML, PDF save,
FileSystemAccesswrites, and drag-out downloads. - Completion paths that skip post-processingfor each item state machine, check that every success path, including failure-then-retry and renderer-driven completion, goes through annotation before “complete” is reported to the UI.
- Off-the-record source dataconfirm that blanking
urlandreferrer.urlforIsOffTheRecord()still produces a quarantine mark, and that the source information is not dropped entirely.
Patch
diff --git a/content/browser/download/save_file.cc b/content/browser/download/save_file.cc
index 6eed86d5..89dfc43 100644
--- a/content/browser/download/save_file.cc
+++ b/content/browser/download/save_file.cc
@@ -91,4 +91,10 @@
return file_.DebugString();
}
+void SaveFile::RunQuarantineCallback() {
+ if (!info_->quarantine_callback.is_null()) {
+ std::move(info_->quarantine_callback).Run();
+ }
+}
+
} // namespace content
diff --git a/content/browser/download/save_file.h b/content/browser/download/save_file.h
index 77dde846..71497ef 100644
--- a/content/browser/download/save_file.h
+++ b/content/browser/download/save_file.h
@@ -56,6 +56,8 @@
}
const SaveFileCreateInfo& create_info() const { return *info_; }
+ void RunQuarantineCallback();
+
private:
download::BaseFile file_;
std::unique_ptr<SaveFileCreateInfo> info_;
diff --git a/content/browser/download/save_file_manager.cc b/content/browser/download/save_file_manager.cc
index 41f27959..5e1057c 100644
--- a/content/browser/download/save_file_manager.cc
+++ b/content/browser/download/save_file_manager.cc
@@ -71,11 +71,13 @@
const net::NetworkTrafficAnnotationTag& annotation_tag,
network::mojom::URLLoaderFactory* url_loader_factory,
SaveFileManager* save_file_manager,
+ base::OnceClosure quarantine_callback,
URLLoaderCompleteCallback on_complete_cb) {
return std::unique_ptr<SimpleURLLoaderHelper>(new SimpleURLLoaderHelper(
std::move(resource_request), save_item_id, save_package_id,
render_process_id, render_frame_routing_id, annotation_tag,
- url_loader_factory, save_file_manager, std::move(on_complete_cb)));
+ url_loader_factory, save_file_manager, std::move(quarantine_callback),
+ std::move(on_complete_cb)));
}
SimpleURLLoaderHelper(const SimpleURLLoaderHelper&) = delete;
@@ -93,10 +95,12 @@
const net::NetworkTrafficAnnotationTag& annotation_tag,
network::mojom::URLLoaderFactory* url_loader_factory,
SaveFileManager* save_file_manager,
+ base::OnceClosure quarantine_callback,
URLLoaderCompleteCallback on_complete_cb)
: save_file_manager_(save_file_manager),
save_item_id_(save_item_id),
save_package_id_(save_package_id),
+ quarantine_callback_(std::move(quarantine_callback)),
on_complete_cb_(std::move(on_complete_cb)) {
GURL url = resource_request->url;
url_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
@@ -124,6 +128,7 @@
auto info = std::make_unique<SaveFileCreateInfo>(
url, final_url, save_item_id_, save_package_id_, render_process_id,
render_frame_routing_id, content_disposition);
+ info->quarantine_callback = std::move(quarantine_callback_);
download::GetDownloadTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&SaveFileManager::StartSave,
save_file_manager_, std::move(info)));
@@ -155,6 +160,7 @@
SaveItemId save_item_id_;
SavePackageId save_package_id_;
std::unique_ptr<network::SimpleURLLoader> url_loader_;
+ base::OnceClosure quarantine_callback_;
URLLoaderCompleteCallback on_complete_cb_;
};
@@ -227,6 +233,12 @@
DCHECK(!base::Contains(packages_, save_item_id));
packages_[save_item_id] = save_package;
+ base::OnceClosure quarantine_callback = base::BindOnce(
+ &SaveFileManager::QuarantineItem, this, save_item_id, save_package->id(),
+ context->IsOffTheRecord() ? GURL() : url,
+ context->IsOffTheRecord() ? GURL() : referrer.url, client_guid,
+ std::move(remote_quarantine));
+
// Register a saving job.
if (save_source == SaveFileCreateInfo::SAVE_FILE_FROM_NET) {
DCHECK(url.is_valid());
@@ -317,22 +329,20 @@
}
base::OnceCallback<void(bool /*success*/)> save_finished_cb =
- base::BindOnce(&SaveFileManager::OnURLLoaderComplete, this,
- save_item_id, save_package->id(),
- context->IsOffTheRecord() ? GURL() : url,
- context->IsOffTheRecord() ? GURL() : referrer.url,
- client_guid, std::move(remote_quarantine));
-
+ base::BindOnce(&SaveFileManager::SaveFinished, this, save_item_id,
+ save_package->id());
url_loader_helpers_[save_item_id] =
SimpleURLLoaderHelper::CreateAndStartDownload(
std::move(request), save_item_id, save_package->id(),
render_process_host_id, render_frame_routing_id, traffic_annotation,
- factory, this, std::move(save_finished_cb));
+ factory, this, std::move(quarantine_callback),
+ std::move(save_finished_cb));
} else {
// We manually start the save job.
auto info = std::make_unique<SaveFileCreateInfo>(
file_full_path, url, save_item_id, save_package->id(),
render_process_host_id, render_frame_routing_id, save_source);
+ info->quarantine_callback = std::move(quarantine_callback);
// Since the data will come from render process, so we need to start
// this kind of save job by ourself.
@@ -382,20 +392,16 @@
base::BindOnce(&SaveFileManager::CancelSave, this, save_item_id));
}
-void SaveFileManager::OnURLLoaderComplete(
+void SaveFileManager::QuarantineItem(
SaveItemId save_item_id,
SavePackageId save_package_id,
const GURL& url,
const GURL& referrer_url,
const std::string& client_guid,
- mojo::PendingRemote<quarantine::mojom::Quarantine> remote_quarantine,
- bool is_success) {
+ mojo::PendingRemote<quarantine::mojom::Quarantine> remote_quarantine) {
DCHECK(download::GetDownloadTaskRunner()->RunsTasksInCurrentSequence());
SaveFile* save_file = LookupSaveFile(save_item_id);
- if (!is_success || !save_file) {
- SaveFinished(save_item_id, save_package_id, is_success);
- return;
- }
+ CHECK(save_file);
save_file->AnnotateWithSourceInformation(
client_guid, url, referrer_url, std::move(remote_quarantine),
@@ -408,8 +414,8 @@
SavePackageId save_package_id,
download::DownloadInterruptReason result) {
DCHECK(download::GetDownloadTaskRunner()->RunsTasksInCurrentSequence());
- SaveFinished(save_item_id, save_package_id,
- result == download::DOWNLOAD_INTERRUPT_REASON_NONE);
+ SaveCompleted(save_item_id, save_package_id,
+ result == download::DOWNLOAD_INTERRUPT_REASON_NONE);
}
// Notifications sent from the IO thread and run on the file thread:
@@ -457,11 +463,23 @@
}
}
-// The IO thread will call this when saving is completed or it got error when
-// fetching data. We forward the message to OnSaveFinished in UI thread.
void SaveFileManager::SaveFinished(SaveItemId save_item_id,
SavePackageId save_package_id,
bool is_success) {
+ DCHECK(download::GetDownloadTaskRunner()->RunsTasksInCurrentSequence());
+
+ SaveFile* save_file = LookupSaveFile(save_item_id);
+ if (!is_success || !save_file) {
+ SaveCompleted(save_item_id, save_package_id, is_success);
+ return;
+ }
+
+ save_file->RunQuarantineCallback();
+}
+
+void SaveFileManager::SaveCompleted(SaveItemId save_item_id,
+ SavePackageId save_package_id,
+ bool is_success) {
DVLOG(20) << __func__ << "() save_item_id = " << save_item_id
<< " save_package_id = " << save_package_id
<< " is_success = " << is_success;
@@ -481,12 +499,11 @@
}
GetUIThreadTaskRunner({})->PostTask(
- FROM_HERE, base::BindOnce(&SaveFileManager::OnSaveFinished, this,
+ FROM_HERE, base::BindOnce(&SaveFileManager::OnSaveCompleted, this,
save_item_id, bytes_so_far, is_success));
}
// Notifications sent from the file thread and run on the UI thread.
-
void SaveFileManager::OnStartSave(const SaveFileCreateInfo& info) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
SavePackage* save_package = GetSavePackageFromRenderIds(
@@ -512,9 +529,9 @@
SendCancelRequest(save_item_id);
}
-void SaveFileManager::OnSaveFinished(SaveItemId save_item_id,
- int64_t bytes_so_far,
- bool is_success) {
+void SaveFileManager::OnSaveCompleted(SaveItemId save_item_id,
+ int64_t bytes_so_far,
+ bool is_success) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
ClearURLLoader(save_item_id);
SavePackage* package = LookupPackage(save_item_id);
diff --git a/content/browser/download/save_file_manager.h b/content/browser/download/save_file_manager.h
index 9b6e7be2..8ea30f6 100644
--- a/content/browser/download/save_file_manager.h
+++ b/content/browser/download/save_file_manager.h
@@ -119,9 +119,18 @@
// Notifications sent from the IO thread and run on the file thread:
void StartSave(std::unique_ptr<SaveFileCreateInfo> info);
void UpdateSaveProgress(SaveItemId save_item_id, const std::string& data);
+
+ // Called on the download TaskRunner when all the data is saved or the
+ // download failed. The saved item will go through quarantine before it is
+ // considered comple.
void SaveFinished(SaveItemId save_item_id,
SavePackageId save_package_id,
bool is_success);
+ // The file saving is completed. We forward the message to OnSaveFinished in
+ // UI thread.
+ void SaveCompleted(SaveItemId save_item_id,
+ SavePackageId save_package_id,
+ bool is_success);
// Notifications sent from the UI thread and run on the file thread.
// Cancel a SaveFile instance which has specified save item id.
@@ -181,17 +190,17 @@
// Help function for sending notification of canceling specific request.
void SendCancelRequest(SaveItemId save_item_id);
- // Called on the file thread when the URLLoader completes saving a SaveItem.
- void OnURLLoaderComplete(
+ // Called on the download TaskRunner to start quarantine a saved file.
+ void QuarantineItem(
SaveItemId save_item_id,
SavePackageId save_package_id,
const GURL& url,
const GURL& referrer_url,
const std::string& client_guid,
- mojo::PendingRemote<quarantine::mojom::Quarantine> remote_quarantine,
- bool is_success);
+ mojo::PendingRemote<quarantine::mojom::Quarantine> remote_quarantine);
- // Called on the file thread when file quarantine finishes on a SaveItem.
+ // Called on the download TaskRunner when file quarantine finishes on a
+ // SaveItem.
void OnQuarantineComplete(SaveItemId save_item_id,
SavePackageId save_package_id,
download::DownloadInterruptReason result);
@@ -208,9 +217,9 @@
bool write_success);
// Update the SavePackage with the finish state, and remove the request
// tracking entries.
- void OnSaveFinished(SaveItemId save_item_id,
- int64_t bytes_so_far,
- bool is_success);
+ void OnSaveCompleted(SaveItemId save_item_id,
+ int64_t bytes_so_far,
+ bool is_success);
// Notifies SavePackage that the whole page saving job is finished.
void OnFinishSavePageJob(int render_process_id,
int render_frame_routing_id,
diff --git a/content/browser/download/save_types.cc b/content/browser/download/save_types.cc
index 0d93a8f..1fdcb7a8 100644
--- a/content/browser/download/save_types.cc
+++ b/content/browser/download/save_types.cc
@@ -39,8 +39,18 @@
content_disposition(content_disposition),
save_source(SaveFileCreateInfo::SAVE_FILE_FROM_NET) {}
-SaveFileCreateInfo::SaveFileCreateInfo(const SaveFileCreateInfo& other) =
- default;
+SaveFileCreateInfo::SaveFileCreateInfo(const SaveFileCreateInfo& other)
+ : path(other.path),
+ url(other.url),
+ final_url(other.final_url),
+ save_item_id(other.save_item_id),
+ save_package_id(other.save_package_id),
+ render_process_id(other.render_process_id),
+ render_frame_routing_id(other.render_frame_routing_id),
+ content_disposition(other.content_disposition),
+ save_source(other.save_source) {
+ // The quarantine_callback is intentionally not copied.
+}
SaveFileCreateInfo::~SaveFileCreateInfo() {}
diff --git a/content/browser/download/save_types.h b/content/browser/download/save_types.h
index da246fc..d1ce26c0 100644
--- a/content/browser/download/save_types.h
+++ b/content/browser/download/save_types.h
@@ -13,6 +13,7 @@
#include <vector>
#include "base/files/file_path.h"
+#include "base/functional/callback.h"
#include "base/types/id_type.h"
#include "url/gurl.h"
@@ -80,6 +81,8 @@
std::string content_disposition;
// Source type of saved file.
SaveFileSource save_source;
+ // Callback to run to quarantine the file;
+ base::OnceClosure quarantine_callback;
};
} // namespace content
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index a2385a2..e038e24 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -69,6 +69,7 @@
#include "ui/shell_dialogs/fake_select_file_dialog.h"
#if BUILDFLAG(IS_CHROMEOS)
+#include "base/test/test_future.h"
#include "chromeos/dbus/dlp/dlp_client.h"
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -1712,27 +1713,40 @@
chromeos::DlpClient::Shutdown();
chromeos::DlpClient::InitializeFake();
- base::test::RepeatingTestFuture<
- dlp::AddFilesRequest, base::OnceCallback<void(dlp::AddFilesResponse)>>
- add_file_cb;
- chromeos::DlpClient::Get()->GetTestInterface()->SetAddFilesMock(
- add_file_cb.GetCallback());
- url = NavigateToMockURL("a");
+ // Use page "b" which has subresources (1.png, 1.css). Total 3 files.
+ url = NavigateToMockURL("b");
- SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "a", 1, &dir,
+ SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "b", 3, &dir,
&full_file_name);
ASSERT_FALSE(HasFailure());
- auto request = std::get<0>(add_file_cb.Take());
- ASSERT_EQ(1, request.add_file_requests().size());
- EXPECT_EQ(full_file_name.value(), request.add_file_requests(0).file_path());
- EXPECT_EQ(request.add_file_requests(0).source_url(), url.spec());
+ // Asynchronously get the recorded requests from the fake client.
+ base::test::TestFuture<const dlp::GetDatabaseEntriesResponse> future;
+ chromeos::DlpClient::Get()->GetDatabaseEntries(future.GetCallback());
+ const auto& response = future.Get();
+ const auto& requests = response.files_entries();
- base::ScopedAllowBlockingForTesting allow_blocking;
- EXPECT_TRUE(base::PathExists(full_file_name));
- EXPECT_FALSE(base::PathExists(dir));
+ // There is a total of 6 requests, 3 for temporary files and 3 for final
+ // destination.
+ ASSERT_EQ(6, requests.size());
+
+ // The order of subresource saving is not guaranteed, so we use a set
+ // to verify the presence of each expected file path.
+ std::set<std::string> expected_paths;
+ expected_paths.insert(full_file_name.value());
+ expected_paths.insert(dir.AppendASCII("1.png").value());
+ expected_paths.insert(dir.AppendASCII("1.css").value());
+
+ std::set<std::string> actual_paths;
+ for (const auto& request : requests) {
+ actual_paths.insert(request.path());
+ }
+
+ for (const auto& expected_path : expected_paths) {
+ EXPECT_TRUE(base::Contains(actual_paths, expected_path));
+ }
}
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveMHTMLWithDlp) {
Regression Test / PoC
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index a2385a2..e038e24 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -69,6 +69,7 @@
#include "ui/shell_dialogs/fake_select_file_dialog.h"
#if BUILDFLAG(IS_CHROMEOS)
+#include "base/test/test_future.h"
#include "chromeos/dbus/dlp/dlp_client.h"
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -1712,27 +1713,40 @@
chromeos::DlpClient::Shutdown();
chromeos::DlpClient::InitializeFake();
- base::test::RepeatingTestFuture<
- dlp::AddFilesRequest, base::OnceCallback<void(dlp::AddFilesResponse)>>
- add_file_cb;
- chromeos::DlpClient::Get()->GetTestInterface()->SetAddFilesMock(
- add_file_cb.GetCallback());
- url = NavigateToMockURL("a");
+ // Use page "b" which has subresources (1.png, 1.css). Total 3 files.
+ url = NavigateToMockURL("b");
- SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "a", 1, &dir,
+ SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "b", 3, &dir,
&full_file_name);
ASSERT_FALSE(HasFailure());
- auto request = std::get<0>(add_file_cb.Take());
- ASSERT_EQ(1, request.add_file_requests().size());
- EXPECT_EQ(full_file_name.value(), request.add_file_requests(0).file_path());
- EXPECT_EQ(request.add_file_requests(0).source_url(), url.spec());
+ // Asynchronously get the recorded requests from the fake client.
+ base::test::TestFuture<const dlp::GetDatabaseEntriesResponse> future;
+ chromeos::DlpClient::Get()->GetDatabaseEntries(future.GetCallback());
+ const auto& response = future.Get();
+ const auto& requests = response.files_entries();
- base::ScopedAllowBlockingForTesting allow_blocking;
- EXPECT_TRUE(base::PathExists(full_file_name));
- EXPECT_FALSE(base::PathExists(dir));
+ // There is a total of 6 requests, 3 for temporary files and 3 for final
+ // destination.
+ ASSERT_EQ(6, requests.size());
+
+ // The order of subresource saving is not guaranteed, so we use a set
+ // to verify the presence of each expected file path.
+ std::set<std::string> expected_paths;
+ expected_paths.insert(full_file_name.value());
+ expected_paths.insert(dir.AppendASCII("1.png").value());
+ expected_paths.insert(dir.AppendASCII("1.css").value());
+
+ std::set<std::string> actual_paths;
+ for (const auto& request : requests) {
+ actual_paths.insert(request.path());
+ }
+
+ for (const auto& expected_path : expected_paths) {
+ EXPECT_TRUE(base::Contains(actual_paths, expected_path));
+ }
}
IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveMHTMLWithDlp) {
Original Bug Report
Zone.Identifier MotW is missing for files saved with Ctrl+S
Steps to reproduce the problem
- Visit e.g. https://bayden.com/echo.aspx
- Hit CTRL+S to save the file
- Save the file using “Web Page, Complete” format.
OBSERVE: File does not have a Mark-of-the-Web alternate data stream.
Problem Description
The lack of a Zone.identifier based MotW was documented by https://mrd0x.com/filefix-part-2/ in an attack chain that involves tricking the user into saving a HTA file.
This appears to be a regression since https://issues.chromium.org/issues/40082670#comment6
If the user chooses “Web Page, HTML Only” the Zone.Identifier MotW is attached correctly. It’s only “Web Page, Complete” that doesn’t attach one.
Summary
Zone.Identifier MotW is missing for files saved with Ctrl+S
Additional Data
Category: Security
Chrome Channel: Not sure
Regression: Yes \