From d1ba38f7c96e74901779089fea6d09b0c7c2521d Mon Sep 17 00:00:00 2001 From: Kelvin Zhang Date: Thu, 17 Sep 2020 11:32:29 -0400 Subject: [PATCH] Check for overflow before allocating memory fore decompression. On 32bit devices, an ZipEntry64 may have size > 2^32, we should check for such cases before attempting to allocate memory. Test: mm -j Change-Id: I0f916ef4b2a692f167719a74bd6ff2e887c6c2ce --- applypatch/imgdiff.cpp | 34 ++++++++++++++++++++++++++++++---- install/install.cpp | 8 +++++++- install/verifier.cpp | 6 ++++++ install/wipe_device.cpp | 7 ++++++- updater/install.cpp | 6 ++++++ updater/target_files.cpp | 7 +++++++ updater/updater.cpp | 7 ++++++- 7 files changed, 68 insertions(+), 7 deletions(-) diff --git a/applypatch/imgdiff.cpp b/applypatch/imgdiff.cpp index d0cb687c..376c511c 100644 --- a/applypatch/imgdiff.cpp +++ b/applypatch/imgdiff.cpp @@ -712,8 +712,14 @@ bool ZipModeImage::InitializeChunks(const std::string& filename, ZipArchiveHandl // Add the end of zip file (mainly central directory) as a normal chunk. size_t entries_end = 0; if (!temp_entries.empty()) { - entries_end = static_cast(temp_entries.back().second.offset + - temp_entries.back().second.compressed_length); + CHECK_GE(temp_entries.back().second.offset, 0); + if (__builtin_add_overflow(temp_entries.back().second.offset, + temp_entries.back().second.compressed_length, &entries_end)) { + LOG(ERROR) << "`entries_end` overflows on entry with offset " + << temp_entries.back().second.offset << " and compressed_length " + << temp_entries.back().second.compressed_length; + return false; + } } CHECK_LT(entries_end, file_content_.size()); chunks_.emplace_back(CHUNK_NORMAL, entries_end, &file_content_, @@ -735,8 +741,16 @@ bool ZipModeImage::InitializeChunks(const std::string& filename, ZipArchiveHandl LOG(ERROR) << "Failed to add " << entry_name << " to target chunks"; return false; } - - pos += temp_entries[nextentry].second.compressed_length; + if (temp_entries[nextentry].second.compressed_length > std::numeric_limits::max()) { + LOG(ERROR) << "Entry " << name << " compressed size exceeds size of address space. " + << entry.compressed_length; + return false; + } + if (__builtin_add_overflow(pos, temp_entries[nextentry].second.compressed_length, &pos)) { + LOG(ERROR) << "`pos` overflows after adding " + << temp_entries[nextentry].second.compressed_length; + return false; + } ++nextentry; continue; } @@ -758,6 +772,12 @@ bool ZipModeImage::InitializeChunks(const std::string& filename, ZipArchiveHandl bool ZipModeImage::AddZipEntryToChunks(ZipArchiveHandle handle, const std::string& entry_name, ZipEntry64* entry) { + if (entry->compressed_length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to add " << entry_name + << " because's compressed size exceeds size of address space. " + << entry->compressed_length; + return false; + } size_t compressed_len = entry->compressed_length; if (compressed_len == 0) return true; @@ -775,6 +795,12 @@ bool ZipModeImage::AddZipEntryToChunks(ZipArchiveHandle handle, const std::strin } } else if (entry->method == kCompressDeflated) { size_t uncompressed_len = entry->uncompressed_length; + if (uncompressed_len > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to add " << entry_name + << " because's compressed size exceeds size of address space. " + << uncompressed_len; + return false; + } std::vector uncompressed_data(uncompressed_len); int ret = ExtractToMemory(handle, entry, uncompressed_data.data(), uncompressed_len); if (ret != 0) { diff --git a/install/install.cpp b/install/install.cpp index 75337320..1b220cb3 100644 --- a/install/install.cpp +++ b/install/install.cpp @@ -246,7 +246,13 @@ bool SetUpAbUpdateCommands(const std::string& package, ZipArchiveHandle zip, int LOG(ERROR) << "Failed to find " << AB_OTA_PAYLOAD_PROPERTIES; return false; } - uint32_t properties_entry_length = properties_entry.uncompressed_length; + auto properties_entry_length = properties_entry.uncompressed_length; + if (properties_entry_length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to extract " << AB_OTA_PAYLOAD_PROPERTIES + << " because's uncompressed size exceeds size of address space. " + << properties_entry_length; + return false; + } std::vector payload_properties(properties_entry_length); int32_t err = ExtractToMemory(zip, &properties_entry, payload_properties.data(), properties_entry_length); diff --git a/install/verifier.cpp b/install/verifier.cpp index d8bc53f6..3f026013 100644 --- a/install/verifier.cpp +++ b/install/verifier.cpp @@ -323,6 +323,12 @@ static std::vector IterateZipEntriesAndSearchForKeys(const ZipArchi std::string_view name; ZipEntry64 entry; while ((iter_status = Next(cookie, &entry, &name)) == 0) { + if (entry.uncompressed_length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to extract " << name + << " because's uncompressed size exceeds size of address space. " + << entry.uncompressed_length; + return {}; + } std::vector pem_content(entry.uncompressed_length); if (int32_t extract_status = ExtractToMemory(handle, &entry, pem_content.data(), pem_content.size()); diff --git a/install/wipe_device.cpp b/install/wipe_device.cpp index 0f896c43..915c87b4 100644 --- a/install/wipe_device.cpp +++ b/install/wipe_device.cpp @@ -51,7 +51,12 @@ std::vector GetWipePartitionList(Package* wipe_package) { std::string partition_list_content; ZipEntry64 entry; if (FindEntry(zip, RECOVERY_WIPE_ENTRY_NAME, &entry) == 0) { - uint32_t length = entry.uncompressed_length; + auto length = entry.uncompressed_length; + if (length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to extract " << RECOVERY_WIPE_ENTRY_NAME + << " because's uncompressed size exceeds size of address space. " << length; + return {}; + } partition_list_content = std::string(length, '\0'); if (auto err = ExtractToMemory( zip, &entry, reinterpret_cast(partition_list_content.data()), length); diff --git a/updater/install.cpp b/updater/install.cpp index cfa4d9d8..29596504 100644 --- a/updater/install.cpp +++ b/updater/install.cpp @@ -35,6 +35,7 @@ #include #include +#include #include #include #include @@ -172,6 +173,11 @@ Value* PackageExtractFileFn(const char* name, State* state, } std::string buffer; + if (entry.uncompressed_length > std::numeric_limits::max()) { + return ErrorAbort(state, kPackageExtractFileFailure, + "%s(): Entry `%s` Uncompressed size exceeds size of address space.", name, + zip_path.c_str()); + } buffer.resize(entry.uncompressed_length); int32_t ret = diff --git a/updater/target_files.cpp b/updater/target_files.cpp index 95192329..207146f5 100644 --- a/updater/target_files.cpp +++ b/updater/target_files.cpp @@ -137,6 +137,13 @@ bool TargetFile::ReadEntryToString(const std::string_view name, std::string* con return true; } + if (entry.uncompressed_length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to extract " << name + << " because's uncompressed size exceeds size of address space. " + << entry.uncompressed_length; + return false; + } + content->resize(entry.uncompressed_length); if (auto extract_err = ExtractToMemory( handle_, &entry, reinterpret_cast(&content->at(0)), entry.uncompressed_length); diff --git a/updater/updater.cpp b/updater/updater.cpp index 73ca0e53..c5267346 100644 --- a/updater/updater.cpp +++ b/updater/updater.cpp @@ -170,7 +170,12 @@ bool Updater::ReadEntryToString(ZipArchiveHandle za, const std::string& entry_na << " in the package: " << ErrorCodeString(find_err); return false; } - + if (entry.uncompressed_length > std::numeric_limits::max()) { + LOG(ERROR) << "Failed to extract " << entry_name + << " because's uncompressed size exceeds size of address space. " + << entry.uncompressed_length; + return false; + } content->resize(entry.uncompressed_length); int extract_err = ExtractToMemory(za, &entry, reinterpret_cast(&content->at(0)), entry.uncompressed_length);