diff --git a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc index a13a158535f83..58956d32b8ff4 100644 --- a/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc +++ b/chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.cc @@ -200,6 +200,80 @@ TEST(DMGAnalyzerTest, InvalidDetachedCodeSignature) { EXPECT_EQ(0, results.detached_code_signatures.size()); } +// ----------------------------------------------------------------------------- +// POC: Nested archives inside a DMG bypass Safe Browsing inspection. +// +// DMGAnalyzer::OnGetTempFile() never assigns the received temp file to +// |temp_file_|, so when ResumeExtraction() encounters a nested .zip/.rar/.7z/ +// .dmg entry it tries to copy the stream into an *invalid* base::File and +// silently `continue`s. The nested archive's contents are therefore never +// added to the analyzer results -> malware packed as DMG->nested.zip->payload +// is never scanned. +// +// Additionally, line 216 (`return !UpdateResultsForEntry(...)`) inverts the +// contract documented in archive_analyzer.h:72-74. That branch is currently +// dead because of the temp-file bug above; fixing only the temp-file bug would +// unmask a premature finished_analysis_callback_ (and a CHECK crash on the +// already-moved OnceCallback at archive_analyzer.cc:170). +// +// This test feeds the analyzer a DMG containing a single nested ZIP that holds +// a Windows executable. With correct behaviour the executable would appear in +// |archived_binary| (as it does for the Zip/Rar/7z analyzers). The test +// asserts that expected behaviour and therefore FAILS on current trunk, +// proving the scan bypass. +// ----------------------------------------------------------------------------- +TEST(DMGAnalyzerTest, POC_NestedArchiveIsScanned) { + base::test::TaskEnvironment task_environment; + + // A minimal valid ZIP archive containing a single stored file `payload.exe` + // whose body is the two ASCII bytes "MZ" (DOS/PE header magic). + static const uint8_t kInnerZip[] = { + 0x50, 0x4b, 0x03, 0x04, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x21, 0x00, 0x5d, 0x9b, 0xb0, 0x8f, 0x02, 0x00, 0x00, 0x00, 0x02, 0x00, + 0x00, 0x00, 0x0b, 0x00, 0x00, 0x00, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, + 0x64, 0x2e, 0x65, 0x78, 0x65, 0x4d, 0x5a, 0x50, 0x4b, 0x01, 0x02, 0x14, + 0x03, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x00, 0x5d, + 0x9b, 0xb0, 0x8f, 0x02, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x0b, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, + 0x01, 0x00, 0x00, 0x00, 0x00, 0x70, 0x61, 0x79, 0x6c, 0x6f, 0x61, 0x64, + 0x2e, 0x65, 0x78, 0x65, 0x50, 0x4b, 0x05, 0x06, 0x00, 0x00, 0x00, 0x00, + 0x01, 0x00, 0x01, 0x00, 0x39, 0x00, 0x00, 0x00, 0x2b, 0x00, 0x00, 0x00, + 0x00, 0x00}; + + MockDMGIterator::FileList file_list{ + {"DMG/inner.zip", + std::vector(std::begin(kInnerZip), std::end(kInnerZip))}, + }; + + 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)); + ASSERT_TRUE(temp_file.IsValid()); + + DMGAnalyzer analyzer; + std::unique_ptr iterator = + std::make_unique(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(); + + // Expected (correct) behaviour: the nested archive's payload is reported. + // Actual behaviour on trunk: the nested .zip is silently skipped because + // (a) temp_file_ is never assigned in OnGetTempFile() so CopyStreamToFile + // fails and the loop `continue`s, and + // (b) even if (a) were fixed, the inverted `!UpdateResultsForEntry` at + // :216 would prematurely end extraction. + EXPECT_FALSE(results.archived_binary.empty()) + << "BUG: nested archive inside DMG was not inspected"; +} + } // namespace } // namespace dmg } // namespace safe_browsing