From ae8180c06dee228cd1378c56afa6020ae98d8a24 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 21 Sep 2016 14:58:11 -0700 Subject: [PATCH] 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 Change-Id: I1d8092a19b73886a671bc9d291cfc27d65e3d236 --- libziparchive/testdata/crash.apk | Bin 0 -> 154 bytes libziparchive/zip_archive.cc | 7 ++++++- libziparchive/zip_archive_test.cc | 7 +++++++ 3 files changed, 13 insertions(+), 1 deletion(-) 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.cc b/libziparchive/zip_archive.cc index 4649a75b2..45f6f0f65 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -270,9 +270,14 @@ static int32_t MapCentralDirectory0(int fd, const char* debug_file_name, * Grab the CD offset and size, and the number of entries in the * archive and verify that they look reasonable. */ - if (eocd->cd_start_offset + eocd->cd_size > eocd_offset) { + if (static_cast(eocd->cd_start_offset) + eocd->cd_size > eocd_offset) { ALOGW("Zip: bad offsets (dir %" PRIu32 ", size %" PRIu32 ", eocd %" PRId64 ")", eocd->cd_start_offset, eocd->cd_size, static_cast(eocd_offset)); +#if defined(__ANDROID__) + if (eocd->cd_start_offset + eocd->cd_size <= eocd_offset) { + android_errorWriteLog(0x534e4554, "31251826"); + } +#endif return kInvalidOffset; } if (eocd->num_records == 0) { diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index 6aee1bbdf..42eb5eece 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -36,6 +36,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::vector kATxtContents { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', @@ -84,6 +85,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));