From 950e3a7587ed3269aab0c3b6625b9cc9149d34d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eirik=20Bj=C3=B8rsn=C3=B8s?= Date: Wed, 9 Oct 2024 15:56:38 +0000 Subject: [PATCH] 8341625: Improve ZipFile validation of the END header Reviewed-by: lancea --- .../share/classes/java/util/zip/ZipFile.java | 9 +- .../util/zip/ZipFile/EndOfCenValidation.java | 160 ++++++++++++++++-- 2 files changed, 152 insertions(+), 17 deletions(-) diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java b/src/java.base/share/classes/java/util/zip/ZipFile.java index 00d100b21a8..21b9593c017 100644 --- a/src/java.base/share/classes/java/util/zip/ZipFile.java +++ b/src/java.base/share/classes/java/util/zip/ZipFile.java @@ -1605,7 +1605,7 @@ public class ZipFile implements ZipConstants, Closeable { private static class End { - int centot; // 4 bytes + long centot; // 4 bytes long cenlen; // 4 bytes long cenoff; // 4 bytes long endpos; // 4 bytes @@ -1697,7 +1697,7 @@ public class ZipFile implements ZipConstants, Closeable { // to use the end64 values end.cenlen = cenlen64; end.cenoff = cenoff64; - end.centot = (int)centot64; // assume total < 2g + end.centot = centot64; end.endpos = end64pos; } catch (IOException x) {} // no ZIP64 loc/end return end; @@ -1733,11 +1733,14 @@ public class ZipFile implements ZipConstants, Closeable { if (end.cenlen > MAX_CEN_SIZE) { zerror("invalid END header (central directory size too large)"); } + if (end.centot < 0 || end.centot > end.cenlen / CENHDR) { + zerror("invalid END header (total entries count too large)"); + } cen = this.cen = new byte[(int)end.cenlen]; if (readFullyAt(cen, 0, cen.length, cenpos) != end.cenlen) { zerror("read CEN tables failed"); } - this.total = end.centot; + this.total = Math.toIntExact(end.centot); } else { cen = this.cen; this.total = knownTotal; diff --git a/test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java b/test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java index ec6542768c5..7adcfb9c128 100644 --- a/test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java +++ b/test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java @@ -25,13 +25,15 @@ * @bug 8272746 * @modules java.base/jdk.internal.util * @summary Verify that ZipFile rejects files with CEN sizes exceeding the implementation limit - * @run testng/othervm EndOfCenValidation + * @run junit/othervm EndOfCenValidation */ import jdk.internal.util.ArraysSupport; -import org.testng.annotations.AfterTest; -import org.testng.annotations.BeforeTest; -import org.testng.annotations.Test; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import java.io.*; import java.nio.ByteBuffer; @@ -43,12 +45,13 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.Arrays; import java.util.EnumSet; +import java.util.HexFormat; import java.util.zip.ZipEntry; import java.util.zip.ZipException; import java.util.zip.ZipFile; import java.util.zip.ZipOutputStream; -import static org.testng.Assert.*; +import static org.junit.jupiter.api.Assertions.*; /** * This test augments {@link TestTooManyEntries}. It creates sparse ZIPs where @@ -70,7 +73,7 @@ public class EndOfCenValidation { private static final int ENDSIZ = ZipFile.ENDSIZ; // Offset of CEN size field within ENDHDR private static final int ENDOFF = ZipFile.ENDOFF; // Offset of CEN offset field within ENDHDR // Maximum allowed CEN size allowed by ZipFile - private static int MAX_CEN_SIZE = ArraysSupport.SOFT_MAX_ARRAY_LENGTH; + private static final int MAX_CEN_SIZE = ArraysSupport.SOFT_MAX_ARRAY_LENGTH; // Expected message when CEN size does not match file size private static final String INVALID_CEN_BAD_SIZE = "invalid END header (bad central directory size)"; @@ -78,6 +81,8 @@ public class EndOfCenValidation { private static final String INVALID_CEN_BAD_OFFSET = "invalid END header (bad central directory offset)"; // Expected message when CEN size is too large private static final String INVALID_CEN_SIZE_TOO_LARGE = "invalid END header (central directory size too large)"; + // Expected message when total entry count is too large + private static final String INVALID_BAD_ENTRY_COUNT = "invalid END header (total entries count too large)"; // A valid ZIP file, used as a template private byte[] zipBytes; @@ -86,7 +91,7 @@ public class EndOfCenValidation { * Create a valid ZIP file, used as a template * @throws IOException if an error occurs */ - @BeforeTest + @BeforeEach public void setup() throws IOException { zipBytes = templateZip(); } @@ -95,7 +100,7 @@ public class EndOfCenValidation { * Delete big files after test, in case the file system did not support sparse files. * @throws IOException if an error occurs */ - @AfterTest + @AfterEach public void cleanup() throws IOException { Files.deleteIfExists(CEN_TOO_LARGE_ZIP); Files.deleteIfExists(INVALID_CEN_SIZE); @@ -113,11 +118,11 @@ public class EndOfCenValidation { Path zip = zipWithModifiedEndRecord(size, true, 0, CEN_TOO_LARGE_ZIP); - ZipException ex = expectThrows(ZipException.class, () -> { + ZipException ex = assertThrows(ZipException.class, () -> { new ZipFile(zip.toFile()); }); - assertEquals(ex.getMessage(), INVALID_CEN_SIZE_TOO_LARGE); + assertEquals(INVALID_CEN_SIZE_TOO_LARGE, ex.getMessage()); } /** @@ -133,11 +138,11 @@ public class EndOfCenValidation { Path zip = zipWithModifiedEndRecord(size, false, 0, INVALID_CEN_SIZE); - ZipException ex = expectThrows(ZipException.class, () -> { + ZipException ex = assertThrows(ZipException.class, () -> { new ZipFile(zip.toFile()); }); - assertEquals(ex.getMessage(), INVALID_CEN_BAD_SIZE); + assertEquals(INVALID_CEN_BAD_SIZE, ex.getMessage()); } /** @@ -153,11 +158,138 @@ public class EndOfCenValidation { Path zip = zipWithModifiedEndRecord(size, true, 100, BAD_CEN_OFFSET_ZIP); - ZipException ex = expectThrows(ZipException.class, () -> { + ZipException ex = assertThrows(ZipException.class, () -> { new ZipFile(zip.toFile()); }); - assertEquals(ex.getMessage(), INVALID_CEN_BAD_OFFSET); + assertEquals(INVALID_CEN_BAD_OFFSET, ex.getMessage()); + } + + /** + * Validate that a 'Zip64 End of Central Directory' record (the END header) + * where the value of the 'total entries' field is larger than what fits + * in the CEN size is rejected. + * + * @throws IOException if an error occurs + */ + @ParameterizedTest + @ValueSource(longs = { + -1, // Negative + Long.MIN_VALUE, // Very negative + 0x3B / 3L - 1, // Cannot fit in test ZIP's CEN + MAX_CEN_SIZE / 3 + 1, // Too large to allocate int[] entries array + Long.MAX_VALUE // Unreasonably large + }) + public void shouldRejectBadTotalEntries(long totalEntries) throws IOException { + /** + * A small ZIP using the ZIP64 format. + * + * ZIP created using: "echo -n hello | zip zip64.zip -" + * Hex encoded using: "cat zip64.zip | xxd -ps" + * + * The file has the following structure: + * + * 0000 LOCAL HEADER #1 04034B50 + * 0004 Extract Zip Spec 2D '4.5' + * 0005 Extract OS 00 'MS-DOS' + * 0006 General Purpose Flag 0000 + * 0008 Compression Method 0000 'Stored' + * 000A Last Mod Time 5947AB78 'Mon Oct 7 21:27:48 2024' + * 000E CRC 363A3020 + * 0012 Compressed Length FFFFFFFF + * 0016 Uncompressed Length FFFFFFFF + * 001A Filename Length 0001 + * 001C Extra Length 0014 + * 001E Filename '-' + * 001F Extra ID #0001 0001 'ZIP64' + * 0021 Length 0010 + * 0023 Uncompressed Size 0000000000000006 + * 002B Compressed Size 0000000000000006 + * 0033 PAYLOAD hello. + * + * 0039 CENTRAL HEADER #1 02014B50 + * 003D Created Zip Spec 1E '3.0' + * 003E Created OS 03 'Unix' + * 003F Extract Zip Spec 2D '4.5' + * 0040 Extract OS 00 'MS-DOS' + * 0041 General Purpose Flag 0000 + * 0043 Compression Method 0000 'Stored' + * 0045 Last Mod Time 5947AB78 'Mon Oct 7 21:27:48 2024' + * 0049 CRC 363A3020 + * 004D Compressed Length 00000006 + * 0051 Uncompressed Length FFFFFFFF + * 0055 Filename Length 0001 + * 0057 Extra Length 000C + * 0059 Comment Length 0000 + * 005B Disk Start 0000 + * 005D Int File Attributes 0001 + * [Bit 0] 1 Text Data + * 005F Ext File Attributes 11B00000 + * 0063 Local Header Offset 00000000 + * 0067 Filename '-' + * 0068 Extra ID #0001 0001 'ZIP64' + * 006A Length 0008 + * 006C Uncompressed Size 0000000000000006 + * + * 0074 ZIP64 END CENTRAL DIR 06064B50 + * RECORD + * 0078 Size of record 000000000000002C + * 0080 Created Zip Spec 1E '3.0' + * 0081 Created OS 03 'Unix' + * 0082 Extract Zip Spec 2D '4.5' + * 0083 Extract OS 00 'MS-DOS' + * 0084 Number of this disk 00000000 + * 0088 Central Dir Disk no 00000000 + * 008C Entries in this disk 0000000000000001 + * 0094 Total Entries 0000000000000001 + * 009C Size of Central Dir 000000000000003B + * 00A4 Offset to Central dir 0000000000000039 + * + * 00AC ZIP64 END CENTRAL DIR 07064B50 + * LOCATOR + * 00B0 Central Dir Disk no 00000000 + * 00B4 Offset to Central dir 0000000000000074 + * 00BC Total no of Disks 00000001 + * + * 00C0 END CENTRAL HEADER 06054B50 + * 00C4 Number of this disk 0000 + * 00C6 Central Dir Disk no 0000 + * 00C8 Entries in this disk 0001 + * 00CA Total Entries 0001 + * 00CC Size of Central Dir 0000003B + * 00D0 Offset to Central Dir FFFFFFFF + * 00D4 Comment Length 0000 + */ + + byte[] zipBytes = HexFormat.of().parseHex(""" + 504b03042d000000000078ab475920303a36ffffffffffffffff01001400 + 2d010010000600000000000000060000000000000068656c6c6f0a504b01 + 021e032d000000000078ab475920303a3606000000ffffffff01000c0000 + 00000001000000b011000000002d010008000600000000000000504b0606 + 2c000000000000001e032d00000000000000000001000000000000000100 + 0000000000003b000000000000003900000000000000504b060700000000 + 740000000000000001000000504b050600000000010001003b000000ffff + ffff0000 + """.replaceAll("\n","")); + + // Buffer to manipulate the above ZIP + ByteBuffer buf = ByteBuffer.wrap(zipBytes).order(ByteOrder.LITTLE_ENDIAN); + // Offset of the 'total entries' in the 'ZIP64 END CENTRAL DIR' record + // Update ZIP64 entry count to a value which cannot possibly fit in the small CEN + buf.putLong(0x94, totalEntries); + // The corresponding END field needs the ZIP64 magic value + buf.putShort(0xCA, (short) 0xFFFF); + // Write the ZIP to disk + Path zipFile = Path.of("bad-entry-count.zip"); + Files.write(zipFile, zipBytes); + + // Verify that the END header is rejected + ZipException ex = assertThrows(ZipException.class, () -> { + try (var zf = new ZipFile(zipFile.toFile())) { + } + }); + + assertEquals(INVALID_BAD_ENTRY_COUNT, ex.getMessage()); } /**