Skip to content

8341625: Improve ZipFile validation of the END header #21384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
eirbjo wants to merge 7 commits into from
Closed

8341625: Improve ZipFile validation of the END header #21384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ba81e6
Improve ZipFile validation of Zip64 END headers
eirbjo Oct 7, 2024
caf67b3
Test comment should be specific that this requires Zip64
eirbjo Oct 7, 2024
8e8cb81
No need to check MAX_ENTRIES, since MAX_ENTRIES * 46 would exceed the…
eirbjo Oct 7, 2024
7f2e408
Use a ZIP64 test vector which may be reproduced using InfoZIP
eirbjo Oct 7, 2024
ccfa56e
Remove trailing whitespace
eirbjo Oct 7, 2024
11a275d
Convert EndOfCenValidation to JUnit 5
eirbjo Oct 8, 2024
0f0801c
Reject "negative" 'total entries' values
eirbjo Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/java.base/share/classes/java/util/zip/ZipFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1608,7 +1608,7 @@ private final int readAt(byte[] buf, int off, int len, long pos)


private static class End {
int centot; // 4 bytes
long centot; // 4 bytes
long cenlen; // 4 bytes
long cenoff; // 4 bytes
long endpos; // 4 bytes
Expand Down Expand Up @@ -1703,7 +1703,7 @@ private End findEND() throws IOException {
// 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;
Expand Down Expand Up @@ -1739,11 +1739,14 @@ private void initCEN(int knownTotal) throws IOException {
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;
Expand Down
160 changes: 146 additions & 14 deletions test/jdk/java/util/zip/ZipFile/EndOfCenValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,12 +45,13 @@
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
Expand All @@ -70,14 +73,16 @@ 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)";
// Expected message when CEN offset is too large
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;
Expand All @@ -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();
}
Expand All @@ -95,7 +100,7 @@ public void setup() throws IOException {
* 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);
Expand All @@ -113,11 +118,11 @@ public void shouldRejectTooLargeCenSize() throws IOException {

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());
}

/**
Expand All @@ -133,11 +138,11 @@ public void shouldRejectInvalidCenSize() throws IOException {

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());
}

/**
Expand All @@ -153,11 +158,138 @@ public void shouldRejectInvalidCenOffset() throws IOException {

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("""
Copy link
Contributor

@LanceAndersen LanceAndersen Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be beneficial to future readers to provide the steps used to create the hex string in the format you have provided.

Copy link
Contributor Author

@eirbjo eirbjo Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be beneficial to future readers to provide the steps used to create the hex string in the format you have provided.

The test vector was crafted using a ZIP test library not available in OpenJDK.

I have replaced that with a test vector which is easily reproduced using InfoZIP. Take a look at the updated test.

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());
}

/**
Expand Down