From fba1a36fd912963a838bcf992f898bc5e9370b63 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 21 Sep 2016 14:58:11 -0700 Subject: [PATCH 1/3] Fix out of bound access in libziparchive The boundary check of an invalid EOCD record may succeed due to the overflow of uint32_t. Fix the check and add a unit test. Test: Open the crash.apk and libziparchive reports the offset error as expected. Bug: 31251826 Merged-In: I1d8092a19b73886a671bc9d291cfc27d65e3d236 Change-Id: I1d8092a19b73886a671bc9d291cfc27d65e3d236 (cherry picked from commit ae8180c06dee228cd1378c56afa6020ae98d8a24) --- libziparchive/testdata/crash.apk | Bin 0 -> 154 bytes libziparchive/zip_archive_test.cc | 7 +++++++ 2 files changed, 7 insertions(+) create mode 100644 libziparchive/testdata/crash.apk diff --git a/libziparchive/testdata/crash.apk b/libziparchive/testdata/crash.apk new file mode 100644 index 0000000000000000000000000000000000000000..d6dd52dd7ffc766ab17284b3a864bd5f1abb752d GIT binary patch literal 154 zcmWIWW@h1H0D*v&GM>SI0@Im*Y!GH-kYO+k4dG;9KK|%XI0% w0p5&Ea?H52OMpxT8pFV_r4hse8paAS49%bbZ&o&t0!AQgYG`Pv2Lc8L03~=HQ2+n{ literal 0 HcmV?d00001 diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index 52099c3b5..7653872fa 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -40,6 +40,7 @@ static const std::string kMissingZip = "missing.zip"; static const std::string kValidZip = "valid.zip"; static const std::string kLargeZip = "large.zip"; static const std::string kBadCrcZip = "bad_crc.zip"; +static const std::string kCrashApk = "crash.apk"; static const std::string kUpdateZip = "dummy-update.zip"; static const std::vector kATxtContents { @@ -89,6 +90,12 @@ TEST(ziparchive, Open) { CloseArchive(handle); } +TEST(ziparchive, OutOfBound) { + ZipArchiveHandle handle; + ASSERT_EQ(-8, OpenArchiveWrapper(kCrashApk, &handle)); + CloseArchive(handle); +} + TEST(ziparchive, OpenMissing) { ZipArchiveHandle handle; ASSERT_NE(0, OpenArchiveWrapper(kMissingZip, &handle)); From 9e020e2d117ac389387743c22a30f049c39970c7 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Mon, 10 Oct 2016 12:11:30 -0700 Subject: [PATCH 2/3] Check filename memory bound when parsing ziparchive Add a check to ensure the filename boundary doesn't exceed the mapped memory region. Also add the corresponding unit test. Bug: 28802225 Test: New unit test passes. Merged-In: Ibf543a7da3d7898952e9eb332c84cdfc67cf5aa4 Change-Id: Ibf543a7da3d7898952e9eb332c84cdfc67cf5aa4 (cherry picked from commit bcc4431f240d1ee5bc89c6ab41542e096e07c48c) --- libziparchive/testdata/bad_filename.zip | Bin 0 -> 4119 bytes libziparchive/zip_archive.cc | 10 ++++++++++ libziparchive/zip_archive_test.cc | 3 +++ 3 files changed, 13 insertions(+) create mode 100644 libziparchive/testdata/bad_filename.zip diff --git a/libziparchive/testdata/bad_filename.zip b/libziparchive/testdata/bad_filename.zip new file mode 100644 index 0000000000000000000000000000000000000000..294eaf562a2350980296bedd5a1066b16fe9a365 GIT binary patch literal 4119 zcmeI#u?+wq2t-jZ(bk3m+{ecKmq93qGno9NxT{}>oUP5oq~ejcYgI3T00IagfB*sr ZAbhash_table_size = RoundUpPower2(1 + (num_entries * 4) / 3); archive->hash_table = reinterpret_cast(calloc(archive->hash_table_size, sizeof(ZipString))); + if (archive->hash_table == nullptr) { + ALOGW("Zip: unable to allocate the %u-entry hash_table, entry size: %zu", + archive->hash_table_size, sizeof(ZipString)); + return -1; + } /* * Walk through the central directory, adding entries to the hash @@ -348,6 +353,11 @@ static int32_t ParseZipArchive(ZipArchive* archive) { const uint16_t comment_length = cdr->comment_length; const uint8_t* file_name = ptr + sizeof(CentralDirectoryRecord); + if (file_name + file_name_length > cd_end) { + ALOGW("Zip: file name boundary exceeds the central directory range, file_name_length: " + "%" PRIx16 ", cd_length: %zu", file_name_length, cd_length); + return -1; + } /* check that file name is valid UTF-8 and doesn't contain NUL (U+0000) characters */ if (!IsValidEntryName(file_name, file_name_length)) { return -1; diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index 7653872fa..47f4bbd57 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -41,6 +41,7 @@ static const std::string kValidZip = "valid.zip"; static const std::string kLargeZip = "large.zip"; static const std::string kBadCrcZip = "bad_crc.zip"; static const std::string kCrashApk = "crash.apk"; +static const std::string kBadFilenameZip = "bad_filename.zip"; static const std::string kUpdateZip = "dummy-update.zip"; static const std::vector kATxtContents { @@ -86,7 +87,9 @@ static void SetZipString(ZipString* zip_str, const std::string& str) { TEST(ziparchive, Open) { ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); + CloseArchive(handle); + ASSERT_EQ(-1, OpenArchiveWrapper(kBadFilenameZip, &handle)); CloseArchive(handle); } From 0fda1cf6330942c3671348f905b9009868850a77 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 5 Apr 2017 14:46:27 -0700 Subject: [PATCH 3/3] Fix out of bound read in libziparchive We should check the boundary of central directory before checking its signature. Swap the order of these two checks. Bug: 36392138 Test: libziparchive doesn't read the signature after boundary check fails. Merged-In: Ie89f709bb2d1ccb647116fb7ccb1e23c943e5ab8 Change-Id: Ie89f709bb2d1ccb647116fb7ccb1e23c943e5ab8 (cherry picked from commit 74464a1361562d4042a67c5d66bfcf396ee7e59c) --- libziparchive/zip_archive.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index 0e8536df7..a310a0777 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -329,6 +329,14 @@ static int32_t ParseZipArchive(ZipArchive* archive) { const uint8_t* const cd_end = cd_ptr + cd_length; const uint8_t* ptr = cd_ptr; for (uint16_t i = 0; i < num_entries; i++) { + if (ptr > cd_end - sizeof(CentralDirectoryRecord)) { + ALOGW("Zip: ran off the end (at %" PRIu16 ")", i); +#if defined(__ANDROID__) + android_errorWriteLog(0x534e4554, "36392138"); +#endif + return -1; + } + const CentralDirectoryRecord* cdr = reinterpret_cast(ptr); if (cdr->record_signature != CentralDirectoryRecord::kSignature) { @@ -336,11 +344,6 @@ static int32_t ParseZipArchive(ZipArchive* archive) { return -1; } - if (ptr + sizeof(CentralDirectoryRecord) > cd_end) { - ALOGW("Zip: ran off the end (at %" PRIu16 ")", i); - return -1; - } - const off64_t local_header_offset = cdr->local_file_header_offset; if (local_header_offset >= archive->directory_offset) { ALOGW("Zip: bad LFH offset %" PRId64 " at entry %" PRIu16,