From 9538f4194f6e5eff1bd59f2396ed9d05b1a8d801 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Degros?= Date: Tue, 15 Feb 2022 22:08:31 +0000 Subject: [zip] Add struct UnzipOptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added struct UnzipOptions to pass optional arguments to the Unzip functions (including the decryption password, if needed.) Removed function UnzipWithFilterCallback(). Use Unzip() + UnzipOptions instead. Renamed function UnzipWithFilterAndWriters() to simply Unzip(), and pass to it an UnzipOptions struct too. Added tests: ZipTest.UnzipEncryptedWithRightPassword ZipTest.UnzipEncryptedWithWrongPassword ZipTest.UnzipEncryptedWithNoPassword BUG=chromium:1296838, chromium:953256 TEST=autoninja -C out/Default zlib_unittests && out/Default/zlib_unittests TEST=autoninja -C out/Default components_unittests && out/Default/components_unittests --gtest_filter='UnzipTest.*' Change-Id: I8a29f57f2cec3007e419a94dd6a4d8ec83c8a59c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3463278 Reviewed-by: Noel Gordon Commit-Queue: François Degros Cr-Commit-Position: refs/heads/main@{#971432} NOKEYCHECK=True GitOrigin-RevId: 68766ecf2214aca9bd122c00ece1f7cfaba168e5 --- google/zip.cc | 44 ++++++--------- google/zip.h | 50 +++++++++-------- google/zip_unittest.cc | 148 ++++++++++++++++++++++++++++++------------------- 3 files changed, 135 insertions(+), 107 deletions(-) diff --git a/google/zip.cc b/google/zip.cc index feedc22..f48306a 100644 --- a/google/zip.cc +++ b/google/zip.cc @@ -26,10 +26,6 @@ bool IsHiddenFile(const base::FilePath& file_path) { return file_path.BaseName().value()[0] == '.'; } -bool ExcludeNoFilesFilter(const base::FilePath& file_path) { - return true; -} - // Creates a directory at |extract_dir|/|entry_path|, including any parents. bool CreateDirectory(const base::FilePath& extract_dir, const base::FilePath& entry_path) { @@ -170,34 +166,28 @@ bool Zip(const ZipParams& params) { return zip_writer->Close(); } -bool Unzip(const base::FilePath& src_file, const base::FilePath& dest_dir) { - return UnzipWithFilterCallback( - src_file, dest_dir, base::BindRepeating(&ExcludeNoFilesFilter), true); -} - -bool UnzipWithFilterCallback(const base::FilePath& src_file, - const base::FilePath& dest_dir, - FilterCallback filter_cb, - bool log_skipped_files) { +bool Unzip(const base::FilePath& src_file, + const base::FilePath& dest_dir, + UnzipOptions options) { base::File file(src_file, base::File::FLAG_OPEN | base::File::FLAG_READ); if (!file.IsValid()) { DLOG(WARNING) << "Cannot open '" << src_file << "'"; return false; } - return UnzipWithFilterAndWriters( - file.GetPlatformFile(), - base::BindRepeating(&CreateFilePathWriterDelegate, dest_dir), - base::BindRepeating(&CreateDirectory, dest_dir), std::move(filter_cb), - log_skipped_files); + return Unzip(file.GetPlatformFile(), + base::BindRepeating(&CreateFilePathWriterDelegate, dest_dir), + base::BindRepeating(&CreateDirectory, dest_dir), + std::move(options)); } -bool UnzipWithFilterAndWriters(const base::PlatformFile& src_file, - WriterFactory writer_factory, - DirectoryCreator directory_creator, - FilterCallback filter_cb, - bool log_skipped_files) { +bool Unzip(const base::PlatformFile& src_file, + WriterFactory writer_factory, + DirectoryCreator directory_creator, + UnzipOptions options) { ZipReader reader; + reader.SetPassword(std::move(options.password)); + if (!reader.OpenFromPlatformFile(src_file)) { DLOG(WARNING) << "Cannot open ZIP from file handle " << src_file; return false; @@ -209,8 +199,8 @@ bool UnzipWithFilterAndWriters(const base::PlatformFile& src_file, return false; } - if (filter_cb && !filter_cb.Run(entry->path)) { - DLOG_IF(WARNING, log_skipped_files) + if (options.filter && !options.filter.Run(entry->path)) { + DLOG_IF(WARNING, options.log_skipped_files) << "Skipped ZIP entry " << entry->path; continue; } @@ -237,11 +227,11 @@ bool UnzipWithFilterAndWriters(const base::PlatformFile& src_file, bool ZipWithFilterCallback(const base::FilePath& src_dir, const base::FilePath& dest_file, - FilterCallback filter_cb) { + FilterCallback filter) { DCHECK(base::DirectoryExists(src_dir)); return Zip({.src_dir = src_dir, .dest_file = dest_file, - .filter_callback = std::move(filter_cb)}); + .filter_callback = std::move(filter)}); } bool Zip(const base::FilePath& src_dir, diff --git a/google/zip.h b/google/zip.h index ecaecb1..a13dbca 100644 --- a/google/zip.h +++ b/google/zip.h @@ -170,33 +170,37 @@ bool ZipFiles(const base::FilePath& src_dir, int dest_fd); #endif // defined(OS_POSIX) -// Unzip the contents of zip_file into dest_dir. -// For each file in zip_file, include it only if the callback |filter_cb| -// returns true. Otherwise omit it. -// If |log_skipped_files| is true, files skipped during extraction are printed -// to debug log. -bool UnzipWithFilterCallback(const base::FilePath& zip_file, - const base::FilePath& dest_dir, - FilterCallback filter_cb, - bool log_skipped_files); - -// Unzip the contents of zip_file, using the writers provided by writer_factory. -// For each file in zip_file, include it only if the callback |filter_cb| -// returns true. Otherwise omit it. -// If |log_skipped_files| is true, files skipped during extraction are printed -// to debug log. +// Options of the Unzip function, with valid default values. +struct UnzipOptions { + // Only extract the entries for which |filter_cb| returns true. If no filter + // is provided, everything gets extracted. + FilterCallback filter; + + // Password to decrypt the encrypted files. + std::string password; + + // If |log_skipped_files| is true, files skipped during extraction are printed + // to debug log. + bool log_skipped_files = true; +}; + typedef base::RepeatingCallback( const base::FilePath&)> WriterFactory; + typedef base::RepeatingCallback DirectoryCreator; -bool UnzipWithFilterAndWriters(const base::PlatformFile& zip_file, - WriterFactory writer_factory, - DirectoryCreator directory_creator, - FilterCallback filter_cb, - bool log_skipped_files); - -// Unzip the contents of zip_file into dest_dir. -bool Unzip(const base::FilePath& zip_file, const base::FilePath& dest_dir); + +// Unzips the contents of |zip_file|, using the writers provided by +// |writer_factory|. +bool Unzip(const base::PlatformFile& zip_file, + WriterFactory writer_factory, + DirectoryCreator directory_creator, + UnzipOptions options = {}); + +// Unzips the contents of |zip_file| into |dest_dir|. +bool Unzip(const base::FilePath& zip_file, + const base::FilePath& dest_dir, + UnzipOptions options = {}); } // namespace zip diff --git a/google/zip_unittest.cc b/google/zip_unittest.cc index a8fdfc6..e383dbf 100644 --- a/google/zip_unittest.cc +++ b/google/zip_unittest.cc @@ -192,32 +192,28 @@ class ZipTest : public PlatformTest { virtual void TearDown() { PlatformTest::TearDown(); } - bool GetTestDataDirectory(base::FilePath* path) { - bool success = base::PathService::Get(base::DIR_SOURCE_ROOT, path); + static base::FilePath GetDataDirectory() { + base::FilePath path; + bool success = base::PathService::Get(base::DIR_SOURCE_ROOT, &path); EXPECT_TRUE(success); - if (!success) - return false; - for (const base::StringPiece s : - {"third_party", "zlib", "google", "test", "data"}) { - *path = path->AppendASCII(s); - } - return true; + return std::move(path) + .AppendASCII("third_party") + .AppendASCII("zlib") + .AppendASCII("google") + .AppendASCII("test") + .AppendASCII("data"); } void TestUnzipFile(const base::FilePath::StringType& filename, bool expect_hidden_files) { - base::FilePath test_dir; - ASSERT_TRUE(GetTestDataDirectory(&test_dir)); - TestUnzipFile(test_dir.Append(filename), expect_hidden_files); + TestUnzipFile(GetDataDirectory().Append(filename), expect_hidden_files); } void TestUnzipFile(const base::FilePath& path, bool expect_hidden_files) { ASSERT_TRUE(base::PathExists(path)) << "no file " << path; ASSERT_TRUE(zip::Unzip(path, test_dir_)); - base::FilePath original_dir; - ASSERT_TRUE(GetTestDataDirectory(&original_dir)); - original_dir = original_dir.AppendASCII("test"); + base::FilePath original_dir = GetDataDirectory().AppendASCII("test"); base::FileEnumerator files( test_dir_, true, @@ -329,9 +325,7 @@ TEST_F(ZipTest, UnzipUncompressed) { } TEST_F(ZipTest, UnzipEvil) { - base::FilePath path; - ASSERT_TRUE(GetTestDataDirectory(&path)); - path = path.AppendASCII("evil.zip"); + base::FilePath path = GetDataDirectory().AppendASCII("evil.zip"); // Unzip the zip file into a sub directory of test_dir_ so evil.zip // won't create a persistent file outside test_dir_ in case of a // failure. @@ -344,10 +338,9 @@ TEST_F(ZipTest, UnzipEvil) { } TEST_F(ZipTest, UnzipEvil2) { - base::FilePath path; - ASSERT_TRUE(GetTestDataDirectory(&path)); // The ZIP file contains a file with invalid UTF-8 in its file name. - path = path.AppendASCII("evil_via_invalid_utf8.zip"); + base::FilePath path = + GetDataDirectory().AppendASCII("evil_via_invalid_utf8.zip"); // See the comment at UnzipEvil() for why we do this. base::FilePath output_dir = test_dir_.AppendASCII("out"); ASSERT_TRUE(zip::Unzip(path, output_dir)); @@ -360,10 +353,8 @@ TEST_F(ZipTest, UnzipWithFilter) { auto filter = base::BindRepeating([](const base::FilePath& path) { return path.BaseName().MaybeAsASCII() == "foo.txt"; }); - base::FilePath path; - ASSERT_TRUE(GetTestDataDirectory(&path)); - ASSERT_TRUE(zip::UnzipWithFilterCallback(path.AppendASCII("test.zip"), - test_dir_, filter, false)); + ASSERT_TRUE(zip::Unzip(GetDataDirectory().AppendASCII("test.zip"), test_dir_, + {.filter = std::move(filter)})); // Only foo.txt should have been extracted. The following paths should not // be extracted: // foo/ @@ -392,9 +383,71 @@ TEST_F(ZipTest, UnzipWithFilter) { ASSERT_EQ(0, extracted_count); } +TEST_F(ZipTest, UnzipEncryptedWithRightPassword) { + // TODO(crbug.com/1296838) Also check the AES-encrypted files. + auto filter = base::BindRepeating([](const base::FilePath& path) { + return !base::StartsWith(path.MaybeAsASCII(), "Encrypted AES"); + }); + + ASSERT_TRUE(zip::Unzip( + GetDataDirectory().AppendASCII("Different Encryptions.zip"), test_dir_, + {.filter = std::move(filter), .password = "password"})); + + std::string contents; + ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("ClearText.txt"), + &contents)); + EXPECT_EQ("This is not encrypted.\n", contents); + + ASSERT_TRUE(base::ReadFileToString( + test_dir_.AppendASCII("Encrypted ZipCrypto.txt"), &contents)); + EXPECT_EQ("This is encrypted with ZipCrypto.\n", contents); +} + +TEST_F(ZipTest, UnzipEncryptedWithWrongPassword) { + // TODO(crbug.com/1296838) Also check the AES-encrypted files. + auto filter = base::BindRepeating([](const base::FilePath& path) { + return !base::StartsWith(path.MaybeAsASCII(), "Encrypted AES"); + }); + + ASSERT_FALSE(zip::Unzip( + GetDataDirectory().AppendASCII("Different Encryptions.zip"), test_dir_, + {.filter = std::move(filter), .password = "wrong"})); + + std::string contents; + ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("ClearText.txt"), + &contents)); + EXPECT_EQ("This is not encrypted.\n", contents); + + // This extracted file contains rubbish data. + ASSERT_TRUE(base::ReadFileToString( + test_dir_.AppendASCII("Encrypted ZipCrypto.txt"), &contents)); + EXPECT_NE("", contents); + EXPECT_NE("This is encrypted with ZipCrypto.\n", contents); +} + +TEST_F(ZipTest, UnzipEncryptedWithNoPassword) { + // TODO(crbug.com/1296838) Also check the AES-encrypted files. + auto filter = base::BindRepeating([](const base::FilePath& path) { + return !base::StartsWith(path.MaybeAsASCII(), "Encrypted AES"); + }); + + ASSERT_FALSE( + zip::Unzip(GetDataDirectory().AppendASCII("Different Encryptions.zip"), + test_dir_, {.filter = std::move(filter)})); + + std::string contents; + ASSERT_TRUE(base::ReadFileToString(test_dir_.AppendASCII("ClearText.txt"), + &contents)); + EXPECT_EQ("This is not encrypted.\n", contents); + + // This extracted file contains rubbish data. + ASSERT_TRUE(base::ReadFileToString( + test_dir_.AppendASCII("Encrypted ZipCrypto.txt"), &contents)); + EXPECT_NE("", contents); + EXPECT_NE("This is encrypted with ZipCrypto.\n", contents); +} + TEST_F(ZipTest, UnzipWithDelegates) { - auto filter = - base::BindRepeating([](const base::FilePath& path) { return true; }); auto dir_creator = base::BindRepeating( [](const base::FilePath& extract_dir, const base::FilePath& entry_path) { return base::CreateDirectory(extract_dir.Append(entry_path)); @@ -407,12 +460,10 @@ TEST_F(ZipTest, UnzipWithDelegates) { extract_dir.Append(entry_path)); }, test_dir_); - base::FilePath path; - ASSERT_TRUE(GetTestDataDirectory(&path)); - base::File file(path.AppendASCII("test.zip"), + + base::File file(GetDataDirectory().AppendASCII("test.zip"), base::File::Flags::FLAG_OPEN | base::File::Flags::FLAG_READ); - ASSERT_TRUE(zip::UnzipWithFilterAndWriters(file.GetPlatformFile(), writer, - dir_creator, filter, false)); + ASSERT_TRUE(zip::Unzip(file.GetPlatformFile(), writer, dir_creator)); base::FilePath dir = test_dir_; base::FilePath dir_foo = dir.AppendASCII("foo"); base::FilePath dir_foo_bar = dir_foo.AppendASCII("bar"); @@ -426,9 +477,7 @@ TEST_F(ZipTest, UnzipWithDelegates) { } TEST_F(ZipTest, Zip) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -439,9 +488,7 @@ TEST_F(ZipTest, Zip) { } TEST_F(ZipTest, ZipIgnoreHidden) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -452,9 +499,7 @@ TEST_F(ZipTest, ZipIgnoreHidden) { } TEST_F(ZipTest, ZipNonASCIIDir) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -492,9 +537,7 @@ TEST_F(ZipTest, ZipTimeStamp) { #if defined(OS_POSIX) TEST_F(ZipTest, ZipFiles) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -519,15 +562,12 @@ TEST_F(ZipTest, ZipFiles) { #endif // defined(OS_POSIX) TEST_F(ZipTest, UnzipFilesWithIncorrectSize) { - base::FilePath test_data_folder; - ASSERT_TRUE(GetTestDataDirectory(&test_data_folder)); - // test_mismatch_size.zip contains files with names from 0.txt to 7.txt with // sizes from 0 to 7 bytes respectively, but the metadata in the zip file says // the uncompressed size is 3 bytes. The ZipReader and minizip code needs to // be clever enough to get all the data out. base::FilePath test_zip_file = - test_data_folder.AppendASCII("test_mismatch_size.zip"); + GetDataDirectory().AppendASCII("test_mismatch_size.zip"); base::ScopedTempDir scoped_temp_dir; ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDir()); @@ -574,9 +614,7 @@ TEST_F(ZipTest, ZipWithFileAccessor) { // Tests progress reporting while zipping files. TEST_F(ZipTest, ZipProgress) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -613,9 +651,7 @@ TEST_F(ZipTest, ZipProgress) { // Tests throttling of progress reporting while zipping files. TEST_F(ZipTest, ZipProgressPeriod) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); @@ -654,9 +690,7 @@ TEST_F(ZipTest, ZipProgressPeriod) { // Tests cancellation while zipping files. TEST_F(ZipTest, ZipCancel) { - base::FilePath src_dir; - ASSERT_TRUE(GetTestDataDirectory(&src_dir)); - src_dir = src_dir.AppendASCII("test"); + base::FilePath src_dir = GetDataDirectory().AppendASCII("test"); base::ScopedTempDir temp_dir; ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); -- cgit v1.2.3