commit 173aba03f72f92dd1f51e117d57185e2face3ee3
author Tianjie <xunchang@google.com> Sat Mar 28 18:28:43 2020 -0700
committer Tianjie <xunchang@google.com> Sat Mar 28 21:31:56 2020 -0700
tree ad8f96f7c9738075c9d6da34c88e3619f0539d05
parent 1e413c8a9d9131eb83ad24d2d9753943e778d870

Fix integrity check when parsing zip64 eocd

The old check has some missing cases and may lead to read OOB.

Bug: 152433916
Test: unit tests pass
Change-Id: I3e8705b9c2db081228c5f9bd191c133668376ff2

libziparchive/zip_archive.cc

diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 9812026..849b68c 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -162,8 +162,10 @@
   }
 
   const int64_t zip64EocdOffset = zip64EocdLocator.zip64_eocd_offset;
-  if (zip64EocdOffset > locatorOffset - sizeof(Zip64EocdRecord)) {
-    ALOGW("Zip: %s: Bad zip64 eocd offset %" PRIu64, debugFileName, zip64EocdOffset);
+  if (locatorOffset <= sizeof(Zip64EocdRecord) ||
+      zip64EocdOffset > locatorOffset - sizeof(Zip64EocdRecord)) {
+    ALOGW("Zip: %s: Bad zip64 eocd offset %" PRId64 ", eocd locator offset %" PRId64, debugFileName,
+          zip64EocdOffset, locatorOffset);
     return kInvalidOffset;
   }
 
@@ -171,7 +173,7 @@
   if (!archive->mapped_zip.ReadAtOffset(reinterpret_cast<uint8_t*>(&zip64EocdRecord),
                                         sizeof(Zip64EocdRecord), zip64EocdOffset)) {
     ALOGW("Zip: %s: read %zu from offset %" PRId64 " failed %s", debugFileName,
-          sizeof(Zip64EocdLocator), static_cast<int64_t>(zip64EocdOffset), debugFileName);
+          sizeof(Zip64EocdLocator), zip64EocdOffset, debugFileName);
     return kIoError;
   }
 
@@ -181,7 +183,8 @@
     return kInvalidFile;
   }
 
-  if (zip64EocdRecord.cd_start_offset > zip64EocdOffset - zip64EocdRecord.cd_size) {
+  if (zip64EocdOffset <= zip64EocdRecord.cd_size ||
+      zip64EocdRecord.cd_start_offset > zip64EocdOffset - zip64EocdRecord.cd_size) {
     ALOGW("Zip: %s: Bad offset for zip64 central directory. cd offset %" PRIu64 ", cd size %" PRIu64
           ", zip64 eocd offset %" PRIu64,
           debugFileName, zip64EocdRecord.cd_start_offset, zip64EocdRecord.cd_size, zip64EocdOffset);

libziparchive/zip_archive_test.cc

diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc
index fbabf96..69be3df 100644
--- a/libziparchive/zip_archive_test.cc
+++ b/libziparchive/zip_archive_test.cc
@@ -1256,3 +1256,17 @@
 
   CloseArchive(handle);
 }
+
+TEST_F(Zip64ParseTest, zip64EocdWrongLocatorOffset) {
+  AddEntry("a.txt", std::vector<uint8_t>(1, 'a'), true, true, true);
+  ConstructEocd();
+  zip_content_.resize(20, 'a');
+  std::copy(zip64_eocd_locator_.begin(), zip64_eocd_locator_.end(),
+            std::back_inserter(zip_content_));
+  std::copy(eocd_record_.begin(), eocd_record_.end(), std::back_inserter(zip_content_));
+
+  ZipArchiveHandle handle;
+  ASSERT_NE(
+      0, OpenArchiveFromMemory(zip_content_.data(), zip_content_.size(), "debug_zip64", &handle));
+  CloseArchive(handle);
+}