From d942e1c61a0ad15aa4b745eb087a390a00e2ebae Mon Sep 17 00:00:00 2001
From: Andrew Paseltiner <[email protected]>
Date: Mon, 04 May 2026 06:11:21 -0700
Subject: [PATCH] Fix logic errors in DMGAnalyzer skipping nested archives

This CL fixes two interconnected logic bugs in DMGAnalyzer that caused
nested archives within DMG files to bypass Safe Browsing analysis:

1. In DMGAnalyzer::OnGetTempFile, the temp_file parameter was not being
   assigned to the temp_file_ member.
2. In DMGAnalyzer::ResumeExtraction, the return value of
   UpdateResultsForEntry was being inverted, causing the analyzer to
   incorrectly signal completion when it should have paused for nested
   analysis.

Included is a regression test DMGAnalyzerTest.NestedArchive that
verifies the fix by ensuring nested archives are correctly detected and
processed.

Fixed: 495840862
Change-Id: I7cfb0a6fe0e69bcb75d037a6258202cd80f65837
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7808264
Commit-Queue: Andrew Paseltiner <[email protected]>
Reviewed-by: Xinghui Lu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1624598}
---

diff --git a/chrome/utility/safe_browsing/archive_analyzer.cc b/chrome/utility/safe_browsing/archive_analyzer.cc
index 074c463..7aaf094 100644
--- a/chrome/utility/safe_browsing/archive_analyzer.cc
+++ b/chrome/utility/safe_browsing/archive_analyzer.cc
@@ -76,6 +76,11 @@
   finished_analysis_callback_ = std::move(callback);
 }
 
+void ArchiveAnalyzer::SetGetTempFileCallbackForTesting(
+    GetTempFileCallback callback) {
+  get_temp_file_callback_ = std::move(callback);
+}
+
 void ArchiveAnalyzer::SetAnalysisDelegate(
     std::unique_ptr<ArchiveAnalysisDelegate> analysis_delegate) {
   CHECK(analysis_delegate);
diff --git a/chrome/utility/safe_browsing/archive_analyzer.h b/chrome/utility/safe_browsing/archive_analyzer.h
index bc9a966..6f657e5 100644
--- a/chrome/utility/safe_browsing/archive_analyzer.h
+++ b/chrome/utility/safe_browsing/archive_analyzer.h
@@ -45,6 +45,7 @@
 
   void SetResultsForTesting(ArchiveAnalyzerResults* results);
   void SetFinishedCallbackForTesting(FinishedAnalysisCallback callback);
+  void SetGetTempFileCallbackForTesting(GetTempFileCallback callback);
 
   void SetAnalysisDelegate(
       std::unique_ptr<ArchiveAnalysisDelegate> analysis_delegate);
diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer.cc
index cabf1eff..30c5b10 100644
--- a/chrome/utility/safe_browsing/mac/dmg_analyzer.cc
+++ b/chrome/utility/safe_browsing/mac/dmg_analyzer.cc
@@ -213,11 +213,13 @@
         }
 
         // TODO(crbug.com/40871873): Support file length here.
-        return !UpdateResultsForEntry(
-            temp_file_.Duplicate(), GetRootPath().Append(path),
-            /*file_length=*/0,
-            /*is_encrypted=*/false, /*is_directory=*/false,
-            /*contents_valid=*/true);
+        if (!UpdateResultsForEntry(
+                temp_file_.Duplicate(), GetRootPath().Append(path),
+                /*file_length=*/0,
+                /*is_encrypted=*/false, /*is_directory=*/false,
+                /*contents_valid=*/true)) {
+          return false;
+        }
       }
     }
   }
@@ -231,6 +233,8 @@
     return;
   }
 
+  temp_file_ = std::move(temp_file);
+
   if (!iterator_->Open()) {
     InitComplete(ArchiveAnalysisResult::kUnknown);
     return;
diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
index a13a158..1e0127c 100644
--- a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
+++ b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
@@ -200,6 +200,59 @@
   EXPECT_EQ(0, results.detached_code_signatures.size());
 }
 
+// Regression test for crbug.com/495840862. This verifies that nested archives
+// are correctly detected and processed by ensuring the analyzer saves the
+// temporary file and correctly pauses extraction for nested analysis.
+// Note: We expect results.success to be false because the nested archive
+// provided in the test is intentionally invalid (it has no 7z header), which
+// causes the nested analyzer (and thus the overall analysis) to report failure.
+TEST(DMGAnalyzerTest, NestedArchive) {
+  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{
+      {"Nested.7z", {0x01, 0x02, 0x03, 0x04}},
+  };
+
+  std::unique_ptr<MockDMGIterator> iterator =
+      std::make_unique<MockDMGIterator>(true, file_list);
+  safe_browsing::ArchiveAnalyzerResults results;
+  base::RunLoop run_loop;
+
+  analyzer_.SetGetTempFileCallbackForTesting(
+      base::BindRepeating([](base::OnceCallback<void(base::File)> callback) {
+        base::FilePath path;
+        base::CreateTemporaryFile(&path);
+        base::File file(path, base::File::FLAG_CREATE_ALWAYS |
+                                  base::File::FLAG_READ |
+                                  base::File::FLAG_WRITE |
+                                  base::File::FLAG_DELETE_ON_CLOSE);
+        std::move(callback).Run(std::move(file));
+      }));
+
+  analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results,
+                                     std::move(temp_file),
+                                     run_loop.QuitClosure());
+  run_loop.Run();
+
+  // The analysis as a whole fails because the nested 7z is invalid.
+  EXPECT_FALSE(results.success);
+  // However, we verify the logic fix by checking that the archive was detected.
+  // If the logic fix were missing, has_archive would be false because the
+  // Nested.7z would have been skipped entirely.
+  EXPECT_TRUE(results.has_archive);
+  ASSERT_EQ(1u, results.archived_archive_filenames.size());
+  EXPECT_EQ(FILE_PATH_LITERAL("Nested.7z"),
+            results.archived_archive_filenames[0].value());
+}
+
 }  // namespace
 }  // namespace dmg
 }  // namespace safe_browsing

diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
index a13a158..1e0127c 100644
--- a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
+++ b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc
@@ -200,6 +200,59 @@
   EXPECT_EQ(0, results.detached_code_signatures.size());
 }
 
+// Regression test for crbug.com/495840862. This verifies that nested archives
+// are correctly detected and processed by ensuring the analyzer saves the
+// temporary file and correctly pauses extraction for nested analysis.
+// Note: We expect results.success to be false because the nested archive
+// provided in the test is intentionally invalid (it has no 7z header), which
+// causes the nested analyzer (and thus the overall analysis) to report failure.
+TEST(DMGAnalyzerTest, NestedArchive) {
+  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{
+      {"Nested.7z", {0x01, 0x02, 0x03, 0x04}},
+  };
+
+  std::unique_ptr<MockDMGIterator> iterator =
+      std::make_unique<MockDMGIterator>(true, file_list);
+  safe_browsing::ArchiveAnalyzerResults results;
+  base::RunLoop run_loop;
+
+  analyzer_.SetGetTempFileCallbackForTesting(
+      base::BindRepeating([](base::OnceCallback<void(base::File)> callback) {
+        base::FilePath path;
+        base::CreateTemporaryFile(&path);
+        base::File file(path, base::File::FLAG_CREATE_ALWAYS |
+                                  base::File::FLAG_READ |
+                                  base::File::FLAG_WRITE |
+                                  base::File::FLAG_DELETE_ON_CLOSE);
+        std::move(callback).Run(std::move(file));
+      }));
+
+  analyzer_.AnalyzeDMGFileForTesting(std::move(iterator), &results,
+                                     std::move(temp_file),
+                                     run_loop.QuitClosure());
+  run_loop.Run();
+
+  // The analysis as a whole fails because the nested 7z is invalid.
+  EXPECT_FALSE(results.success);
+  // However, we verify the logic fix by checking that the archive was detected.
+  // If the logic fix were missing, has_archive would be false because the
+  // Nested.7z would have been skipped entirely.
+  EXPECT_TRUE(results.has_archive);
+  ASSERT_EQ(1u, results.archived_archive_filenames.size());
+  EXPECT_EQ(FILE_PATH_LITERAL("Nested.7z"),
+            results.archived_archive_filenames[0].value());
+}
+
 }  // namespace
 }  // namespace dmg
 }  // namespace safe_browsing