From 8efe1e8b1b126421aef6998f64f01dd466db07d3 Mon Sep 17 00:00:00 2001 From: Paulo Casanova Date: Tue, 28 Nov 2017 18:37:47 +0000 Subject: Improve overlapping detection in ZFile. ZFile now reports when files overlap in a zip. It previously failed with a VerifyException and now, at least in cases where the local header can still be read correctly (for example, a file contained inside another file), it reports a nice error that allows users to understand what the problem is. Bug: 69835362 Change-Id: Ib34df31f97805f1d5ec9ba917dce7374ea6a7d22 Test: included --- .../java/com/android/apkzlib/zip/FileUseMap.java | 37 ++++++++++++++++ src/main/java/com/android/apkzlib/zip/ZFile.java | 47 ++++++++++++++++++++- .../java/com/android/apkzlib/zip/ZFileTest.java | 36 ++++++++++++++++ .../testData/packaging/entry-outside-file.zip | Bin 0 -> 364 bytes .../resources/testData/packaging/overlapping.zip | Bin 0 -> 502 bytes .../resources/testData/packaging/overlapping2.zip | Bin 0 -> 502 bytes 6 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 src/test/resources/testData/packaging/entry-outside-file.zip create mode 100644 src/test/resources/testData/packaging/overlapping.zip create mode 100644 src/test/resources/testData/packaging/overlapping2.zip diff --git a/src/main/java/com/android/apkzlib/zip/FileUseMap.java b/src/main/java/com/android/apkzlib/zip/FileUseMap.java index a72a956..8a76878 100644 --- a/src/main/java/com/android/apkzlib/zip/FileUseMap.java +++ b/src/main/java/com/android/apkzlib/zip/FileUseMap.java @@ -538,6 +538,43 @@ class FileUseMap { return map.lower(entry); } + /** + * Obtains the entry that is located after the one provided. + * + * @param entry the map entry to get the next one for; must belong to the map + * @return the entry after the provided one, {@code null} if {@code entry} is the last entry in + * the map + */ + @Nullable + FileUseMapEntry after(@Nonnull FileUseMapEntry entry) { + Preconditions.checkNotNull(entry, "entry == null"); + + return map.higher(entry); + } + + /** + * Obtains the entry at the given offset. + * + * @param offset the offset to look for + * @return the entry found or {@code null} if there is no entry (not even a free one) at the + * given offset + */ + @Nullable + FileUseMapEntry at(long offset) { + Preconditions.checkArgument(offset >= 0, "offset < 0"); + Preconditions.checkArgument(offset < size, "offset >= size"); + + FileUseMapEntry entry = map.floor(FileUseMapEntry.makeFree(offset, offset + 1)); + if (entry == null) { + return null; + } + + Verify.verify(entry.getStart() <= offset); + Verify.verify(entry.getEnd() > offset); + + return entry; + } + @Override public String toString() { StringJoiner j = new StringJoiner(", "); diff --git a/src/main/java/com/android/apkzlib/zip/ZFile.java b/src/main/java/com/android/apkzlib/zip/ZFile.java index 1bdb410..9034f4c 100644 --- a/src/main/java/com/android/apkzlib/zip/ZFile.java +++ b/src/main/java/com/android/apkzlib/zip/ZFile.java @@ -597,7 +597,7 @@ public class ZFile implements Closeable { readCentralDirectory(); /* - * Compute where the last file ends. We will need this to compute thee extra offset. + * Go over all files and create the usage map, verifying there is no overlap in the files. */ long entryEndOffset; long directoryStartOffset; @@ -625,6 +625,51 @@ public class ZFile implements Closeable { * file. */ + Verify.verify(start >= 0, "start < 0"); + Verify.verify(end < map.size(), "end >= map.size()"); + + FileUseMapEntry found = map.at(start); + Verify.verifyNotNull(found); + + // We've got a problem if the found entry is not free or is a free entry but + // doesn't cover the whole file. + if (!found.isFree() || found.getEnd() < end) { + if (found.isFree()) { + found = map.after(found); + Verify.verify(found != null && !found.isFree()); + } + + Object foundEntry = found.getStore(); + Verify.verify(foundEntry != null); + + // Obtains a custom description of an entry. + IOExceptionFunction describe = + e -> + String.format( + "'%s' (offset: %d, size: %d)", + e.getCentralDirectoryHeader().getName(), + e.getCentralDirectoryHeader().getOffset(), + e.getInFileSize()); + + String overlappingEntryDescription; + if (foundEntry instanceof StoredEntry) { + StoredEntry foundStored = (StoredEntry) foundEntry; + overlappingEntryDescription = describe.apply((StoredEntry) foundEntry); + } else { + overlappingEntryDescription = + "Central Directory / EOCD: " + + found.getStart() + + " - " + + found.getEnd(); + } + + throw new IOException( + "Cannot read entry " + + describe.apply(entry) + + " because it overlaps with " + + overlappingEntryDescription); + } + FileUseMapEntry mapEntry = map.add(start, end, entry); entries.put(entry.getCentralDirectoryHeader().getName(), mapEntry); diff --git a/src/test/java/com/android/apkzlib/zip/ZFileTest.java b/src/test/java/com/android/apkzlib/zip/ZFileTest.java index bce0286..b7f2979 100644 --- a/src/test/java/com/android/apkzlib/zip/ZFileTest.java +++ b/src/test/java/com/android/apkzlib/zip/ZFileTest.java @@ -33,6 +33,7 @@ import com.android.apkzlib.zip.utils.CloseableByteSource; import com.android.apkzlib.zip.utils.RandomAccessFileUtils; import com.google.common.base.Charsets; import com.google.common.base.Strings; +import com.google.common.base.Throwables; import com.google.common.hash.Hashing; import com.google.common.io.ByteStreams; import com.google.common.io.Closer; @@ -1782,4 +1783,39 @@ public class ZFileTest { assertArrayEquals(comment, zf.getEocdComment()); } } + + @Test + public void overlappingZipEntries() throws Exception { + File myZip = ZipTestUtils.cloneRsrc("overlapping.zip", mTemporaryFolder); + try (ZFile zf = new ZFile(myZip)) { + fail(); + } catch (IOException e) { + assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/bbb")); + assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/ddd")); + assertFalse(Throwables.getStackTraceAsString(e).contains("Central Directory")); + } + } + + @Test + public void overlappingZipEntryWithCentralDirectory() throws Exception { + File myZip = ZipTestUtils.cloneRsrc("overlapping2.zip", mTemporaryFolder); + try (ZFile zf = new ZFile(myZip)) { + fail(); + } catch (IOException e) { + assertFalse(Throwables.getStackTraceAsString(e).contains("overlapping/bbb")); + assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/ddd")); + assertTrue(Throwables.getStackTraceAsString(e).contains("Central Directory")); + } + } + + @Test + public void readFileWithOffsetBeyondFileEnd() throws Exception { + File myZip = ZipTestUtils.cloneRsrc("entry-outside-file.zip", mTemporaryFolder); + try (ZFile zf = new ZFile(myZip)) { + fail(); + } catch (IOException e) { + assertTrue(Throwables.getStackTraceAsString(e).contains("entry-outside-file/foo")); + assertTrue(Throwables.getStackTraceAsString(e).contains("EOF")); + } + } } diff --git a/src/test/resources/testData/packaging/entry-outside-file.zip b/src/test/resources/testData/packaging/entry-outside-file.zip new file mode 100644 index 0000000..ffd6be9 Binary files /dev/null and b/src/test/resources/testData/packaging/entry-outside-file.zip differ diff --git a/src/test/resources/testData/packaging/overlapping.zip b/src/test/resources/testData/packaging/overlapping.zip new file mode 100644 index 0000000..7f6144c Binary files /dev/null and b/src/test/resources/testData/packaging/overlapping.zip differ diff --git a/src/test/resources/testData/packaging/overlapping2.zip b/src/test/resources/testData/packaging/overlapping2.zip new file mode 100644 index 0000000..eecefa9 Binary files /dev/null and b/src/test/resources/testData/packaging/overlapping2.zip differ -- cgit v1.2.3