From 173aba03f72f92dd1f51e117d57185e2face3ee3 Mon Sep 17 00:00:00 2001
From: Tianjie <xunchang@google.com>
Date: Sat, 28 Mar 2020 18:28:43 -0700
Subject: [PATCH] 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      | 11 +++++++----
 libziparchive/zip_archive_test.cc | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 9812026b4..849b68c1e 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -162,8 +162,10 @@ static ZipError FindCentralDirectoryInfoForZip64(const char* debugFileName, ZipA
   }
 
   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 @@ static ZipError FindCentralDirectoryInfoForZip64(const char* debugFileName, ZipA
   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 @@ static ZipError FindCentralDirectoryInfoForZip64(const char* debugFileName, ZipA
     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);
diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc
index fbabf9625..69be3dfdc 100644
--- a/libziparchive/zip_archive_test.cc
+++ b/libziparchive/zip_archive_test.cc
@@ -1256,3 +1256,17 @@ TEST_F(Zip64ParseTest, iterates) {
 
   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);
+}