diff --git a/libziparchive/testdata/bad_filename.zip b/libziparchive/testdata/bad_filename.zip new file mode 100644 index 000000000..294eaf562 Binary files /dev/null and b/libziparchive/testdata/bad_filename.zip differ diff --git a/libziparchive/testdata/crash.apk b/libziparchive/testdata/crash.apk new file mode 100644 index 000000000..d6dd52dd7 Binary files /dev/null and b/libziparchive/testdata/crash.apk differ diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index efe10966e..a310a0777 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -316,6 +316,11 @@ static int32_t ParseZipArchive(ZipArchive* archive) { archive->hash_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 @@ -324,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) { @@ -331,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, @@ -348,6 +356,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 52099c3b5..47f4bbd57 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -40,6 +40,8 @@ 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 kBadFilenameZip = "bad_filename.zip"; static const std::string kUpdateZip = "dummy-update.zip"; static const std::vector kATxtContents { @@ -85,7 +87,15 @@ 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); +} + +TEST(ziparchive, OutOfBound) { + ZipArchiveHandle handle; + ASSERT_EQ(-8, OpenArchiveWrapper(kCrashApk, &handle)); CloseArchive(handle); }