Chrome · Safebrowsing › Safe_browsing
CVE-2026-87535
Logic Error in Safebrowsing (Safe_browsing)
Overview
Medium
Severity
6.5
CVSS
No
Exploited ITW
Fixed
Fix Status
Background
- DMG analyzer
- Safe Browsing’s macOS component that walks the entries of a downloaded
.dmgdisk image and copies interesting files into a temp file so they can be inspected and reported. - `archived_binary`
- the repeated field in the download-protection request (
ClientDownloadRequest) that lists the files found inside an archive; entries absent from it are never seen by Safe Browsing’s backend. - `UpdateResultsForEntry`
- the shared routine each archive analyzer calls per entry; per the patch’s own comment it “handles nested archives vs plain files itself,” i.e. it, not the caller, is responsible for classifying an entry.
- `DownloadFileType_InspectionType`
- an enum (
ZIP,RAR,DMG,SEVEN_ZIP, etc.) returned byGetFileTypedescribing how a file extension should be treated.
Root Cause Analysis
In the DMG extraction loop (in DMGAnalyzer::ResumeExtraction per the commit message; the @@ header only shows @@ -213,28 +213,21 @@), a non-Mach-O entry was gated behind a file_type check: it was copied to temp_file_ and passed to UpdateResultsForEntry only when GetFileType(base::FilePath(path)) returned DownloadFileType::ZIP, RAR, DMG, or SEVEN_ZIP. Every other non-Mach-O entry — the removed code’s comment and the commit message cite .pkg, .command, and .tar — fell through and was silently dropped, so it never reached archived_binary. This violated the invariant, upheld by the ZIP/RAR/7z analyzers, that every non-Mach-O entry is forwarded to UpdateResultsForEntry, which itself decides how to classify it.
The fix simply deletes the if (file_type == ... ) wrapper so the copy-to-temp-file and UpdateResultsForEntry call now run for all non-Mach-O entries. Because the omitted files were never reported, this is an information-loss/omission defect in Safe Browsing rather than a memory-safety bug; the diff shows only a filtering condition being removed, not a new sink being reached.
Key insight
The core mistake was letting the caller pre-filter entries by extension-derived
DownloadFileType_InspectionType when classification is properly UpdateResultsForEntry’s job, causing every non-archive, non-Mach-O file to be dropped; removing the check restores parity with the other archive analyzers and forwards all entries.Attack Path
- Craft a DMG with a hidden payload
An attacker packages a malicious but non-Mach-O, non-nested-archive file (e.g. a
.pkginstaller, a.commandscript, or a.tar) inside a.dmgdisk image and gets a victim to download it. - Extension misses the allow-list
During analysis,
GetFileTypereturns anDownloadFileType_InspectionTypethat is notZIP/RAR/DMG/SEVEN_ZIP, so the pre-patch guard skipsCopyStreamToFileandUpdateResultsForEntryfor that entry. - Entry omitted from the report
The file never gets added to
archived_binary, so theClientDownloadRequestsent to Safe Browsing omits it entirely. - Detection is evaded Because Safe Browsing’s backend never sees the payload’s digest/metadata, it cannot flag or block the download on that basis (inferred; the diff proves the omission but not a specific backend verdict change).
Impact Assessment
An attacker gains the ability to smuggle a class of files (non-Mach-O, non-nested-archive entries such as
.pkg, .command, .tar) inside a .dmg past Safe Browsing’s download-protection reporting on macOS, since those entries were never placed in archived_binary. This is an information loss/omission in Safe Browsing, matching the advisory’s stated impact and LogicError class; the precondition is that the victim downloads an attacker-supplied DMG containing such an entry. The vendor advisory rates this medium severity and does not report in-the-wild exploitation; NVD scores it CVSS 6.5. The advisory does not report exploitation in the wild or a severity rating, so none is asserted here.Changed Functions
| Function | Change | Notes |
|---|---|---|
forchrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc |
modified | |
TESTchrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc |
modified |
Files Changed
chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.ccchrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.ccchrome/utility/safe_browsing/mac/dmg_analyzer_unittest.ccchrome/utility/safe_browsing/mac/dmg_analyzer.cc
Audit Directions
- Caller-side pre-filtering before a shared classifierLook for other archive analyzers or extraction loops that gate entries on
GetFileType/DownloadFileType_InspectionTypebefore calling a shared per-entry routine likeUpdateResultsForEntry, since that routine is designed to classify entries itself and any upstream filter can silently drop data. - Silent `continue`/skip on the non-matching branchAudit loops where entries that fail an allow-list fall through with no reporting; a missing entry in a security telemetry field (e.g.
archived_binary) is an omission bug that produces no error and no test failure absent an explicit count check. - Divergence between per-format analyzersCompare the DMG, ZIP, RAR, and 7z analyzers for behavioral parity; a rule (like “forward every non-Mach-O/non-nested entry”) enforced in some analyzers but not others is a strong signal of an information-loss gap.
Patch
From 04f80b813454196b720af0b1c3fefa963c643fef Mon Sep 17 00:00:00 2001 From: Tiffany Song <[email protected]> Date: Wed, 29 Jul 2026 10:43:05 -0700 Subject: [PATCH] safe_browsing: record all non-Mach-O DMG entries DMGAnalyzer::ResumeExtraction only forwarded a non-Mach-O entry to UpdateResultsForEntry when its extension mapped to one of the nested-archive inspection types (ZIP/RAR/DMG/7z), so files like .pkg, .command and .tar inside a DMG were dropped and never appeared in archived_binary. The ZIP/RAR/7z analyzers already forward every entry; UpdateResultsForEntry handles nested archives vs plain files itself. Remove the inspection-type check so every non-Mach-O entry is copied to the temp file and reported, matching the other archive analyzers. Relax the SandboxedDMGAnalyzer and DownloadProtectionService DMG end-to-end tests, which previously asserted that the mach_o_in_dmg.dmg fixture produced exactly two archived_binary entries, to tolerate the additional non-Mach-O entries now reported. Add DMGAnalyzer unit tests covering .pkg and .tar entries. Internal review: https://chrome-internal-review.googlesource.com/c/chrome/experimental/chromium/src/+/9604128 Bug: 520572550 Change-Id: I547f9cf2a9d18319084efbc471aead8dcf230833 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8163316 Reviewed-by: Yaw Frempong <[email protected]> Commit-Queue: Tiffany Song <[email protected]> Cr-Commit-Position: refs/heads/main@{#1670393} --- diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer.cc index 894f4f8..06187d7 100644 --- a/chrome/utility/safe_browsing/mac/dmg_analyzer.cc +++ b/chrome/utility/safe_browsing/mac/dmg_analyzer.cc @@ -213,28 +213,21 @@ } else { // Get a new `stream` because it was moved from in previous branches. stream = iterator_->GetReadStream(); - DownloadFileType_InspectionType file_type = - GetFileType(base::FilePath(path)); - if (file_type == DownloadFileType::ZIP || - file_type == DownloadFileType::RAR || - file_type == DownloadFileType::DMG || - file_type == DownloadFileType::SEVEN_ZIP) { - if (!CopyStreamToFile(*stream, temp_file_)) { - continue; - } + if (!CopyStreamToFile(*stream, temp_file_)) { + continue; + } - if (!temp_file_.IsValid()) { - continue; - } + if (!temp_file_.IsValid()) { + continue; + } - // TODO(crbug.com/40871873): Support file length here. - if (!UpdateResultsForEntry( - temp_file_.Duplicate(), GetRootPath().Append(path), - /*file_length=*/0, - /*is_encrypted=*/false, /*is_directory=*/false, - /*contents_valid=*/true)) { - return false; - } + // TODO(crbug.com/40871873): Support file length here. + if (!UpdateResultsForEntry(temp_file_.Duplicate(), + GetRootPath().Append(path), + /*file_length=*/0, + /*is_encrypted=*/false, /*is_directory=*/false, + /*contents_valid=*/true)) { + return false; } } } diff --git a/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc index 8b96419c..20c3d138 100644 --- a/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc +++ b/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc @@ -2460,21 +2460,26 @@ EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, request->download_type()); - ASSERT_EQ(2, request->archived_binary().size()); - for (const auto& binary : request->archived_binary()) { + EXPECT_GT(request->archived_binary_size(), 0); + bool found_mach_o = false; + for (int i = 0; i < request->archived_binary_size(); ++i) { + const auto& binary = request->archived_binary(i); EXPECT_FALSE(binary.file_path().empty()); - EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, - binary.download_type()); ASSERT_TRUE(binary.has_digests()); EXPECT_TRUE(binary.digests().has_sha256()); EXPECT_TRUE(binary.has_length()); - ASSERT_TRUE(binary.has_image_headers()); - ASSERT_FALSE(binary.image_headers().mach_o_headers().empty()); + if (binary.image_headers().mach_o_headers().empty()) { + continue; + } + found_mach_o = true; + EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, + binary.download_type()); EXPECT_FALSE( binary.image_headers().mach_o_headers().Get(0).mach_header().empty()); EXPECT_FALSE( binary.image_headers().mach_o_headers().Get(0).load_commands().empty()); } + EXPECT_TRUE(found_mach_o); ASSERT_EQ(1, request->detached_code_signature().size()); EXPECT_FALSE(request->detached_code_signature().Get(0).file_name().empty()); diff --git a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc index fdc298d..4b1628c 100644 --- a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc +++ b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc @@ -98,7 +98,6 @@ EXPECT_TRUE(results.success); EXPECT_TRUE(results.has_executable); - EXPECT_EQ(2, results.archived_binary.size()); bool got_executable = false, got_dylib = false; for (const auto& binary : results.archived_binary) { @@ -107,11 +106,11 @@ safe_browsing::ClientDownloadRequest_MachOHeaders>& headers = binary.image_headers().mach_o_headers(); - EXPECT_EQ(safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, - binary.download_type()); - if (file_name.find("executablefat") != std::string::npos) { got_executable = true; + EXPECT_EQ( + safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, + binary.download_type()); ASSERT_EQ(2, headers.size()); const safe_browsing::ClientDownloadRequest_MachOHeaders& arch32 = @@ -133,6 +132,9 @@ actual_sha256); } else if (file_name.find("lib64.dylib") != std::string::npos) { got_dylib = true; + EXPECT_EQ( + safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, + binary.download_type()); ASSERT_EQ(1, headers.size()); const safe_browsing::ClientDownloadRequest_MachOHeaders& arch = @@ -146,8 +148,6 @@ EXPECT_EQ( "2012CE4987B0FA4A5D285DF7E810560E841CFAB3054BC19E1AAB345F862A6C4E", actual_sha256); - } else { - ADD_FAILURE() << "Unexpected result file " << binary.file_path(); } } @@ -173,7 +173,6 @@ EXPECT_TRUE(results.success); EXPECT_TRUE(results.has_executable); - EXPECT_EQ(2, results.archived_binary.size()); bool got_executable = false, got_dylib = false; for (const auto& binary : results.archived_binary) { @@ -182,11 +181,11 @@ safe_browsing::ClientDownloadRequest_MachOHeaders>& headers = binary.image_headers().mach_o_headers(); - EXPECT_EQ(safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, - binary.download_type()); - if (file_name.find("executablefat") != std::string::npos) { got_executable = true; + EXPECT_EQ( + safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, + binary.download_type()); ASSERT_EQ(2, headers.size()); const safe_browsing::ClientDownloadRequest_MachOHeaders& arch32 = @@ -208,6 +207,9 @@ actual_sha256); } else if (file_name.find("lib64.dylib") != std::string::npos) { got_dylib = true; + EXPECT_EQ( + safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE, + binary.download_type()); ASSERT_EQ(1, headers.size()); const safe_browsing::ClientDownloadRequest_MachOHeaders& arch = @@ -221,8 +223,6 @@ EXPECT_EQ( "2012CE4987B0FA4A5D285DF7E810560E841CFAB3054BC19E1AAB345F862A6C4E", actual_sha256); - } else { - ADD_FAILURE() << "Unexpected result file " << binary.file_path(); } } diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc index a6b17936..1444a51 100644 --- a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc +++ b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc @@ -312,6 +312,76 @@ EXPECT_EQ(inner, written_bytes); } +// Non-Mach-O entries with extensions that are checked binaries (e.g. .pkg) +// should be recorded in `archived_binary` so they are included in the download +// protection request, matching the behavior of the other archive analyzers. +TEST(DMGAnalyzerTest, ContainedPkgRecorded) { + base::test::TaskEnvironment task_environment; + DMGAnalyzer analyzer_; + base::FilePath temp_path; + base::File temp_file; + base::CreateTemporaryFile(&temp_path); + temp_file.Initialize( + temp_path, (base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ | + base::File::FLAG_WRITE | base::File::FLAG_WIN_TEMPORARY | + base::File::FLAG_DELETE_ON_CLOSE)); + + MockDMGIterator::FileList file_list{ + {"DMG/Install.pkg", {'x', 'a', 'r', '!', 0x00, 0x1c, 0x00, 0x01}}, + }; + + std::unique_ptr<MockDMGIterator> iterator = + std::make_unique<MockDMGIterator>(true, file_list); + safe_browsing::ArchiveAnalyzerResults results; + base::RunLoop run_loop; + analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results, + std::move(temp_file), + run_loop.QuitClosure()); + run_loop.Run(); + + EXPECT_TRUE(results.success); + EXPECT_TRUE(results.has_executable); + ASSERT_EQ(1, results.archived_binary.size()); + EXPECT_EQ("DMG/Install.pkg", results.archived_binary.Get(0).file_path()); + EXPECT_TRUE(results.archived_binary.Get(0).is_executable()); + EXPECT_TRUE(results.archived_binary.Get(0).has_digests()); +} + +// Non-Mach-O entries with extensions that are archives (e.g. .tar) should be +// recorded in `archived_binary`, matching the behavior of the other archive +// analyzers. +TEST(DMGAnalyzerTest, ContainedTarRecorded) { + base::test::TaskEnvironment task_environment; + DMGAnalyzer analyzer_; + base::FilePath temp_path; + base::File temp_file; + base::CreateTemporaryFile(&temp_path); + temp_file.Initialize( + temp_path, (base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ | + base::File::FLAG_WRITE | base::File::FLAG_WIN_TEMPORARY | + base::File::FLAG_DELETE_ON_CLOSE)); + + MockDMGIterator::FileList file_list{ + {"DMG/payload.tar", {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}}, + }; + + std::unique_ptr<MockDMGIterator> iterator = + std::make_unique<MockDMGIterator>(true, file_list); + safe_browsing::ArchiveAnalyzerResults results; + base::RunLoop run_loop; + analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results, + std::move(temp_file), + run_loop.QuitClosure()); + run_loop.Run(); + + EXPECT_TRUE(results.success); + EXPECT_TRUE(results.has_archive); + ASSERT_EQ(1, results.archived_binary.size()); + EXPECT_EQ("DMG/payload.tar", results.archived_binary.Get(0).file_path()); + EXPECT_TRUE(results.archived_binary.Get(0).is_archive()); + EXPECT_TRUE(results.archived_binary.Get(0).has_digests()); +} + } // namespace } // namespace dmg } // namespace safe_browsing
Loading diff…
Regression Test / PoC
shipped with the fix
diff --git a/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc
index 8b96419c..20c3d138 100644
--- a/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc
+++ b/chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc
@@ -2460,21 +2460,26 @@
EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
request->download_type());
- ASSERT_EQ(2, request->archived_binary().size());
- for (const auto& binary : request->archived_binary()) {
+ EXPECT_GT(request->archived_binary_size(), 0);
+ bool found_mach_o = false;
+ for (int i = 0; i < request->archived_binary_size(); ++i) {
+ const auto& binary = request->archived_binary(i);
EXPECT_FALSE(binary.file_path().empty());
- EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
- binary.download_type());
ASSERT_TRUE(binary.has_digests());
EXPECT_TRUE(binary.digests().has_sha256());
EXPECT_TRUE(binary.has_length());
- ASSERT_TRUE(binary.has_image_headers());
- ASSERT_FALSE(binary.image_headers().mach_o_headers().empty());
+ if (binary.image_headers().mach_o_headers().empty()) {
+ continue;
+ }
+ found_mach_o = true;
+ EXPECT_EQ(ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
+ binary.download_type());
EXPECT_FALSE(
binary.image_headers().mach_o_headers().Get(0).mach_header().empty());
EXPECT_FALSE(
binary.image_headers().mach_o_headers().Get(0).load_commands().empty());
}
+ EXPECT_TRUE(found_mach_o);
ASSERT_EQ(1, request->detached_code_signature().size());
EXPECT_FALSE(request->detached_code_signature().Get(0).file_name().empty());
diff --git a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc
index fdc298d..4b1628c 100644
--- a/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc
+++ b/chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac_unittest.cc
@@ -98,7 +98,6 @@
EXPECT_TRUE(results.success);
EXPECT_TRUE(results.has_executable);
- EXPECT_EQ(2, results.archived_binary.size());
bool got_executable = false, got_dylib = false;
for (const auto& binary : results.archived_binary) {
@@ -107,11 +106,11 @@
safe_browsing::ClientDownloadRequest_MachOHeaders>& headers =
binary.image_headers().mach_o_headers();
- EXPECT_EQ(safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
- binary.download_type());
-
if (file_name.find("executablefat") != std::string::npos) {
got_executable = true;
+ EXPECT_EQ(
+ safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
+ binary.download_type());
ASSERT_EQ(2, headers.size());
const safe_browsing::ClientDownloadRequest_MachOHeaders& arch32 =
@@ -133,6 +132,9 @@
actual_sha256);
} else if (file_name.find("lib64.dylib") != std::string::npos) {
got_dylib = true;
+ EXPECT_EQ(
+ safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
+ binary.download_type());
ASSERT_EQ(1, headers.size());
const safe_browsing::ClientDownloadRequest_MachOHeaders& arch =
@@ -146,8 +148,6 @@
EXPECT_EQ(
"2012CE4987B0FA4A5D285DF7E810560E841CFAB3054BC19E1AAB345F862A6C4E",
actual_sha256);
- } else {
- ADD_FAILURE() << "Unexpected result file " << binary.file_path();
}
}
@@ -173,7 +173,6 @@
EXPECT_TRUE(results.success);
EXPECT_TRUE(results.has_executable);
- EXPECT_EQ(2, results.archived_binary.size());
bool got_executable = false, got_dylib = false;
for (const auto& binary : results.archived_binary) {
@@ -182,11 +181,11 @@
safe_browsing::ClientDownloadRequest_MachOHeaders>& headers =
binary.image_headers().mach_o_headers();
- EXPECT_EQ(safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
- binary.download_type());
-
if (file_name.find("executablefat") != std::string::npos) {
got_executable = true;
+ EXPECT_EQ(
+ safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
+ binary.download_type());
ASSERT_EQ(2, headers.size());
const safe_browsing::ClientDownloadRequest_MachOHeaders& arch32 =
@@ -208,6 +207,9 @@
actual_sha256);
} else if (file_name.find("lib64.dylib") != std::string::npos) {
got_dylib = true;
+ EXPECT_EQ(
+ safe_browsing::ClientDownloadRequest_DownloadType_MAC_EXECUTABLE,
+ binary.download_type());
ASSERT_EQ(1, headers.size());
const safe_browsing::ClientDownloadRequest_MachOHeaders& arch =
@@ -221,8 +223,6 @@
EXPECT_EQ(
"2012CE4987B0FA4A5D285DF7E810560E841CFAB3054BC19E1AAB345F862A6C4E",
actual_sha256);
- } else {
- ADD_FAILURE() << "Unexpected result file " << binary.file_path();
}
}
diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
index a6b17936..1444a51 100644
--- a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
+++ b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
@@ -312,6 +312,76 @@
EXPECT_EQ(inner, written_bytes);
}
+// Non-Mach-O entries with extensions that are checked binaries (e.g. .pkg)
+// should be recorded in `archived_binary` so they are included in the download
+// protection request, matching the behavior of the other archive analyzers.
+TEST(DMGAnalyzerTest, ContainedPkgRecorded) {
+ base::test::TaskEnvironment task_environment;
+ DMGAnalyzer analyzer_;
+ base::FilePath temp_path;
+ base::File temp_file;
+ base::CreateTemporaryFile(&temp_path);
+ temp_file.Initialize(
+ temp_path, (base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ |
+ base::File::FLAG_WRITE | base::File::FLAG_WIN_TEMPORARY |
+ base::File::FLAG_DELETE_ON_CLOSE));
+
+ MockDMGIterator::FileList file_list{
+ {"DMG/Install.pkg", {'x', 'a', 'r', '!', 0x00, 0x1c, 0x00, 0x01}},
+ };
+
+ std::unique_ptr<MockDMGIterator> iterator =
+ std::make_unique<MockDMGIterator>(true, file_list);
+ safe_browsing::ArchiveAnalyzerResults results;
+ base::RunLoop run_loop;
+ analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results,
+ std::move(temp_file),
+ run_loop.QuitClosure());
+ run_loop.Run();
+
+ EXPECT_TRUE(results.success);
+ EXPECT_TRUE(results.has_executable);
+ ASSERT_EQ(1, results.archived_binary.size());
+ EXPECT_EQ("DMG/Install.pkg", results.archived_binary.Get(0).file_path());
+ EXPECT_TRUE(results.archived_binary.Get(0).is_executable());
+ EXPECT_TRUE(results.archived_binary.Get(0).has_digests());
+}
+
+// Non-Mach-O entries with extensions that are archives (e.g. .tar) should be
+// recorded in `archived_binary`, matching the behavior of the other archive
+// analyzers.
+TEST(DMGAnalyzerTest, ContainedTarRecorded) {
+ base::test::TaskEnvironment task_environment;
+ DMGAnalyzer analyzer_;
+ base::FilePath temp_path;
+ base::File temp_file;
+ base::CreateTemporaryFile(&temp_path);
+ temp_file.Initialize(
+ temp_path, (base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ |
+ base::File::FLAG_WRITE | base::File::FLAG_WIN_TEMPORARY |
+ base::File::FLAG_DELETE_ON_CLOSE));
+
+ MockDMGIterator::FileList file_list{
+ {"DMG/payload.tar", {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}},
+ };
+
+ std::unique_ptr<MockDMGIterator> iterator =
+ std::make_unique<MockDMGIterator>(true, file_list);
+ safe_browsing::ArchiveAnalyzerResults results;
+ base::RunLoop run_loop;
+ analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results,
+ std::move(temp_file),
+ run_loop.QuitClosure());
+ run_loop.Run();
+
+ EXPECT_TRUE(results.success);
+ EXPECT_TRUE(results.has_archive);
+ ASSERT_EQ(1, results.archived_binary.size());
+ EXPECT_EQ("DMG/payload.tar", results.archived_binary.Get(0).file_path());
+ EXPECT_TRUE(results.archived_binary.Get(0).is_archive());
+ EXPECT_TRUE(results.archived_binary.Get(0).has_digests());
+}
+
} // namespace
} // namespace dmg
} // namespace safe_browsing
Loading diff…
Original Bug Report
reported by [email protected]
Potential Safe Browsing nested-content reporting bypass in DMGAnalyzer for checked binaries
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. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.
Overview: The DMGAnalyzer potentially silently drops nested non-Mach-O entries whose file types are not ZIP, RAR, DMG, or 7z (such as .pkg, .command, and .tar files), failing to record them as archived binaries. While sibling analyzers (ZIP, RAR, 7z) unconditionally record these entries, DMGAnalyzer lacks a fallthrough execution path for them. This causes a potential detection asymmetry where malicious payloads packaged inside a DMG can bypass Safe Browsing telemetry.
Affected files:
chrome/utility/safe_browsing/mac/dmg_analyzer.cc
Estimated timestamp from git blame: 2023-04-26
Root Cause
In chrome/utility/safe_browsing/mac/dmg_analyzer.cc (lines 200-226), DMGAnalyzer::ResumeExtraction() is responsible for iterating over and analyzing files within a DMG. However, non-Mach-O entries are handled in an else block that only processes files of four specific inspection types (ZIP, RAR, DMG, or SEVEN_ZIP):
} else {
// Get a new `stream` because it was read from in previous branches.
stream = iterator_->GetReadStream();
DownloadFileType_InspectionType file_type =
GetFileType(base::FilePath(path));
if (file_type == DownloadFileType::ZIP ||
file_type == DownloadFileType::RAR ||
file_type == DownloadFileType::DMG ||
file_type == DownloadFileType::SEVEN_ZIP) {
if (!CopyStreamToFile(*stream, temp_file_)) {
continue;
}
...
if (!UpdateResultsForEntry(temp_file_.Duplicate(), ...)) {
return false;
}
}
// No else: non-{ZIP, RAR, DMG, 7z} and non-Mach-O files are silently dropped.
}
By comparison, sibling analyzers (ZIP, RAR, 7z) call UpdateResultsForEntry() unconditionally for every entry in the archive.
UpdateResultsForEntry() subsequently calls UpdateArchiveAnalyzerResultsWithFile(), which checks if the file is a checked binary (IsCheckedBinaryFile()) or an archive (IsArchiveFile()). If so, it computes its SHA256 and appends it to results->archived_binary so that it is included in the Safe Browsing telemetry request.
Because the DMGAnalyzer does not invoke UpdateResultsForEntry() for other file types, common macOS payloads such as flat-package installer files (.pkg, .mpkg), executable scripts (.command), or compressed archives (.tar, .gz, .bz2) inside a DMG are silently dropped and never recorded.
Potential Trigger Path
Note: The following steps are a potential vector, as our tooling cannot execute code.
- An attacker packages a malicious installer file
Install.pkginside a DMG volume (evil.dmg) and hosts it on a server. - A user downloads
evil.dmgon macOS using Google Chrome. - Chrome triggers the out-of-process
SandboxedDMGAnalyzerwithin the utility process. DMGAnalyzer::ResumeExtraction()iterates over the DMG’s files. ForInstall.pkg, the parser determines it is not a Mach-O file (since its magic bytes arexar!), entering theelseblock.- The analyzer calls
GetFileType("Install.pkg"), which evaluates toDownloadFileType::NONEbecause.pkghas no specialinspection_typeindownload_file_types.asciipb. - The
ifcheck on lines 205-208 evaluates to false, so the file is skipped. - The browser process receives the results with an empty
archived_binarylist. The Safe Browsing backend receives a telemetry ping with no record ofInstall.pkgor its hash, allowing the download to potentially bypass blocklist and reputation checks.
Suggested Fix
Modify the check in chrome/utility/safe_browsing/mac/dmg_analyzer.cc to also extract and process the file if it is a checked binary or archive file:
} else {
stream = iterator_->GetReadStream();
base::FilePath file_path(path);
DownloadFileType_InspectionType file_type = GetFileType(file_path);
bool should_analyze =
file_type == DownloadFileType::ZIP ||
file_type == DownloadFileType::RAR ||
file_type == DownloadFileType::DMG ||
file_type == DownloadFileType::SEVEN_ZIP ||
safe_browsing::FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file_path) ||
safe_browsing::FileTypePolicies::GetInstance()->IsArchiveFile(file_path);
if (should_analyze) {
if (!CopyStreamToFile(*stream, temp_file_)) {
continue;
}
if (!temp_file_.IsValid()) {
continue;
}
if (!UpdateResultsForEntry(
temp_file_.Duplicate(), GetRootPath().Append(path),
/*file_length=*/0,
/*is_encrypted=*/false, /*is_directory=*/false,
/*contents_valid=*/true)) {
return false;
}
}
}
Evaluated with Chrome root at commit: e9507a33bb4148ee071aaaf8a7e9ad68770359bf
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.
References
On This Page