From 188ca7ff69b6c8966c9f676e8ac9e10ddf2282bf Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 9 Sep 2026 08:50:12 -0400 Subject: [PATCH] fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, UTF-8 entry names Three zip container conformance fixes found while auditing the TDF zip container against PKWARE APPNOTE.TXT. 1. ZipWriter only set the zip64 flag on the end of central directory record when the entry count exceeded 0xFF or the central directory offset/size exceeded 0xFFFF. Those masks do not match the field widths: the entry count is 2 bytes and the offset and size are 4 bytes each. Archives with between 256 and 65534 entries were needlessly promoted to zip64, and the offset/size checks now go through needsZip64 so they honor the same 2 GiB ceiling as the per-entry fields. 2. ZipReader treated a short read while scanning backwards for the end of central directory signature as a signature match, so a truncated archive could fall out of the scan loop and parse whatever followed as an end of central directory record. It now only breaks on a real match and throws InvalidZipException otherwise, and rejects an archive too small to hold the zip64 locator it claims to have. 3. ZipWriter computed the central directory filename length from String.length() rather than from the UTF-8 encoded byte count. The value was assigned to a field that write() never read, so the bytes on the wire were already correct, but the dead field is removed, the name is encoded once instead of twice, and a name too long for the 2 byte length field is now rejected instead of silently truncated. --- .../io/opentdf/platform/sdk/ZipReader.java | 99 +++++++-- .../io/opentdf/platform/sdk/ZipWriter.java | 62 ++++-- .../opentdf/platform/sdk/ZipReaderTest.java | 205 +++++++++++++++++ .../opentdf/platform/sdk/ZipWriterTest.java | 210 +++++++++++++++++- 4 files changed, 543 insertions(+), 33 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index 576ba5de..634ba989 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -25,23 +25,38 @@ public class ZipReader { public static final int END_OF_CENTRAL_DIRECTORY_SIZE = 22; public static final int ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE = 20; + /** + * Fills {@code buf} from the channel. {@link SeekableByteChannel#read} may return fewer bytes + * than asked for while more are still available, so a single read cannot tell "the archive + * ends here" from "that read came up short" — and taking the second for the first rejects a + * perfectly good archive. Only a read that reports no progress at all is an end of file. + * + * @return false at end of file, in which case {@code buf} holds nothing worth reading + */ + private boolean fill(ByteBuffer buf) throws IOException { + buf.clear(); + while (buf.hasRemaining()) { + if (this.zipChannel.read(buf) <= 0) { + return false; + } + } + buf.flip(); + return true; + } + final ByteBuffer longBuf = ByteBuffer.allocate(Long.BYTES).order(ByteOrder.LITTLE_ENDIAN); private long readLong() throws IOException { - longBuf.clear(); - if (this.zipChannel.read(longBuf) != 8) { + if (!fill(longBuf)) { throw new InvalidZipException("Expected long value"); } - longBuf.flip(); return longBuf.getLong(); } final ByteBuffer intBuf = ByteBuffer.allocate(Integer.BYTES).order(ByteOrder.LITTLE_ENDIAN); private Integer readInteger() throws IOException { - intBuf.clear(); - if (this.zipChannel.read(intBuf) != 4) { + if (!fill(intBuf)) { return null; } - intBuf.flip(); return intBuf.getInt(); } private int readInt() throws IOException { @@ -64,11 +79,9 @@ private long readUnsignedInt() throws IOException { final ByteBuffer shortBuf = ByteBuffer.allocate(Short.BYTES).order(ByteOrder.LITTLE_ENDIAN); private short readShort() throws IOException { - shortBuf.clear(); - if (this.zipChannel.read(shortBuf) != 2) { + if (!fill(shortBuf)) { throw new InvalidZipException("Expected short value"); } - shortBuf.flip(); return shortBuf.getShort(); } @@ -102,23 +115,60 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { private static final int ZIP64_MAGIC_SHORT = 0xFFFF; private static final int ZIP64_EXTID= 0x0001; + /** + * The most a comment can push the end of central directory record back from the end of the + * archive, and so how far back the scan for it has to look. The record is followed by nothing + * but its own comment, whose length lives in a 2-byte field. + */ + private static final long MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE = 0xFFFF; + + /** + * Positions the channel at an offset that came out of the archive itself. The zip64 records + * carry offsets as 64-bit values, so a corrupt archive can point anywhere: unchecked, a + * negative one escapes as an {@link IllegalArgumentException} from the channel rather than as + * a zip error, and one past the end lands somewhere plausible and fails later with a + * complaint about whatever happened to be there. + */ + private void seekWithinArchive(String what, long offset) throws IOException { + if (offset < 0 || offset >= zipChannel.size()) { + throw new InvalidZipException(what + " points to offset " + offset + + ", which is outside this " + zipChannel.size() + " byte archive"); + } + zipChannel.position(offset); + } + CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { long eoCDRStart = zipChannel.size() - END_OF_CENTRAL_DIRECTORY_SIZE; // 22 is the minimum size of the EOCDR - - while (eoCDRStart >= 0) { + // a comment is the only thing that can sit between the record and the end of the archive, + // so there is no reason to look back any further than the longest possible one. an + // unbounded scan walks the whole archive a byte at a time doing a positioned four byte + // read per byte — tens of seconds per hundred MiB against a file — before it can report + // that the archive is not a zip, and it gives a stray signature deep inside a payload a + // chance to be mistaken for the record + long earliestPossibleStart = Math.max(0, zipChannel.size() + - (END_OF_CENTRAL_DIRECTORY_SIZE + MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE)); + + boolean found = false; + while (eoCDRStart >= earliestPossibleStart) { zipChannel.position(eoCDRStart); Integer signature = readInteger(); - if (signature == null || signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE) { + // readInteger reports an end of file as null, which the bounds of this scan rule out: + // every offset it probes has a whole record behind it. keep the two cases apart + // anyway, so that an end of file can never be taken for a match + if (signature != null && signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE) { if (logger.isDebugEnabled()) { logger.debug("Found end of central directory signature at {}", zipChannel.position() - Integer.BYTES); } + found = true; break; } eoCDRStart--; } - if (eoCDRStart < 0) { - throw new InvalidZipException("Didn't find the end of central directory"); + if (!found) { + throw new InvalidZipException("Didn't find the end of central directory in the last " + + (zipChannel.size() - earliestPossibleStart) + " bytes of this " + + zipChannel.size() + " byte archive"); } short diskNumber = readShort(); @@ -128,7 +178,7 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { int totalNumEntries = readUnsignedShort(); long sizeOfCentralDirectory = readUnsignedInt(); long offsetToStartOfCentralDirectory = readUnsignedInt(); - int commentLength = readUnsignedShort(); + readUnsignedShort(); // comment length; nothing here reads it, but the field is there // any one of these fields may carry the sentinel that sends its real value to the zip64 // end of central directory record; an archive can need zip64 for its entry count alone @@ -140,7 +190,15 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { return new CentralDirectoryRecord(totalNumEntries, offsetToStartOfCentralDirectory); } - long zip64CentralDirectoryLocatorStart = zipChannel.size() - (ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE + END_OF_CENTRAL_DIRECTORY_SIZE + commentLength); + // the locator sits immediately before the record we found, so it is measured from there + // rather than from the end of the archive. the two agree only when nothing follows the + // record and its comment length is honest; measuring from the end lands at the wrong + // offset for anything else and blames the locator for it + long zip64CentralDirectoryLocatorStart = eoCDRStart - ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE; + if (zip64CentralDirectoryLocatorStart < 0) { + throw new InvalidZipException( + "Archive is too small to hold the zip64 end of central directory locator it claims to have"); + } zipChannel.position(zip64CentralDirectoryLocatorStart); return extractZIP64CentralDirectoryInfo(); } @@ -156,10 +214,13 @@ private CentralDirectoryRecord extractZIP64CentralDirectoryInfo() throws IOExcep long offsetToEndOfCentralDirectory = readLong(); int totalNumberOfDisks = readInt(); - zipChannel.position(offsetToEndOfCentralDirectory); + seekWithinArchive("the zip64 end of central directory locator", offsetToEndOfCentralDirectory); int sig = readInt(); if (sig != ZIP_64_END_OF_CENTRAL_DIRECTORY_SIGNATURE) { - throw new InvalidZipException("Invalid"); + throw new InvalidZipException("Invalid zip64 end of central directory signature at offset " + + offsetToEndOfCentralDirectory + ": expected 0x" + + Integer.toHexString(ZIP_64_END_OF_CENTRAL_DIRECTORY_SIGNATURE) + + " but found 0x" + Integer.toHexString(sig)); } long sizeOfEndOfCentralDirectoryRecord = readLong(); short versionMadeBy = readShort(); @@ -341,7 +402,7 @@ public Entry readCentralDirectoryFileHeader() throws IOException { public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); - zipChannel.position(centralDirectoryRecord.offsetToStart); + seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); for (int i = 0; i < centralDirectoryRecord.numEntries; i++) { entries.add(readCentralDirectoryFileHeader()); } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java index 65357573..dc416b8f 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java @@ -30,6 +30,24 @@ public class ZipWriter { * {@link ZipReader}. */ static final long MAX_NON_ZIP64_VALUE = Integer.MAX_VALUE; + + /** + * The largest entry count we will write into the 2-byte end of central directory field. + * {@code 0xFFFF} in that field is the sentinel that sends the real count to the zip64 end of + * central directory record, so the format's own limit is {@code 0xFFFE}. + *

+ * We stop at {@link Short#MAX_VALUE} instead, for the reason spelled out on + * {@link #MAX_NON_ZIP64_VALUE}: the field is unsigned on the wire, but a reader that widens it + * with a signed read sees any count above {@code 0x7FFF} as negative and then finds no entries + * at all — which is what versions of this SDK that predate the unsigned reads in + * {@link ZipReader} do. Switching to ZIP64 at 32,767 entries costs one zip64 end of central + * directory record and locator for the whole archive. + */ + private static final long MAX_NON_ZIP64_ENTRY_COUNT = Short.MAX_VALUE; + + /** The largest name we can describe in the 2-byte filename length field. */ + private static final int MAX_FILENAME_LENGTH = 0xFFFF; + private static final long ZIP_64_END_OF_CD_RECORD_SIZE = 56; private static final int ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE = 28; @@ -81,16 +99,30 @@ static void checkFitsInCentralDirectory(String name, long offset, long size) { } } + /** + * Encodes an entry name the way it is written to the archive. Zip filename lengths are + * counted in bytes, so anything derived from {@link String#length()} — which counts UTF-16 + * code units — desyncs the central directory for a non-ASCII name. + */ + private static byte[] encodeFilename(String name) { + var bytes = name.getBytes(StandardCharsets.UTF_8); + if (bytes.length > MAX_FILENAME_LENGTH) { + throw new SDKException("zip entry name is " + bytes.length + + " bytes when encoded as UTF-8, which does not fit in the " + + MAX_FILENAME_LENGTH + " byte filename length field"); + } + return bytes; + } + public OutputStream stream(String name) throws IOException { var startPosition = out.position; long fileTime, fileDate; fileTime = fileDate = getTimeDateUnMSDosFormat(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; localFileHeader.crc32 = 0; localFileHeader.generalPurposeBitFlag = (1 << 3) | (1 << 11); // we are using the data descriptor and we are using UTF-8 localFileHeader.compressedSize = ZIP_64_MAGIC_VAL; @@ -170,12 +202,12 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream checkFitsInCentralDirectory(fileInfo.filename, fileInfo.offset, fileInfo.size); } + var nameBytes = encodeFilename(fileInfo.filename); CDFileHeader cdFileHeader = new CDFileHeader(); cdFileHeader.generalPurposeBitFlag = fileInfo.flag; cdFileHeader.lastModifiedTime = fileInfo.fileTime; cdFileHeader.lastModifiedDate = fileInfo.fileDate; cdFileHeader.crc32 = (int) fileInfo.crc; - cdFileHeader.filenameLength = (short) fileInfo.filename.length(); cdFileHeader.extraFieldLength = 0; cdFileHeader.compressedSize = (int) fileInfo.size; cdFileHeader.uncompressedSize = (int) fileInfo.size; @@ -188,7 +220,7 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream cdFileHeader.extraFieldLength = ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE; } - cdFileHeader.write(out, fileInfo.filename.getBytes(StandardCharsets.UTF_8)); + cdFileHeader.write(out, nameBytes); if (fileInfo.isZip64) { Zip64GlobalExtendedInfoExtraField zip64ExtendedInfoExtraField = new Zip64GlobalExtendedInfoExtraField(); @@ -208,18 +240,17 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o crc.update(data); var crcValue = crc.getValue(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; localFileHeader.generalPurposeBitFlag = 0; localFileHeader.crc32 = (int) crcValue; localFileHeader.compressedSize = data.length; localFileHeader.uncompressedSize = data.length; localFileHeader.extraFieldLength = 0; - localFileHeader.write(out, name.getBytes(StandardCharsets.UTF_8)); + localFileHeader.write(out, nameBytes); out.write(data); @@ -238,10 +269,15 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o private void writeEndOfCentralDirectory(boolean hasZip64Entry, long numEntries, long startOfCentralDirectory, long sizeOfCentralDirectory, CountingOutputStream out) throws IOException { + // each of these corresponds to a field in the end of central directory record that has to + // carry a sentinel — and send its real value to the zip64 record — once the value stops + // fitting. the entry count is 2 bytes and uses its own fixed limit; the offset and size + // are 4 bytes each and go through the configured threshold, which is MAX_NON_ZIP64_VALUE + // outside of tests. both limits stop short of what the format allows, so that readers + // which widen these fields with a signed read never see a negative value var isZip64 = hasZip64Entry - || (numEntries & ~0xFF) != 0 - || (startOfCentralDirectory & ~0xFFFF) != 0 - || (sizeOfCentralDirectory & ~0xFFFF) != 0; + || numEntries > MAX_NON_ZIP64_ENTRY_COUNT + || needsZip64(startOfCentralDirectory, sizeOfCentralDirectory); if (isZip64) { var endPosition = out.position; @@ -322,7 +358,6 @@ private static class LocalFileHeader { int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength = 0; void write(OutputStream out, byte[] filename) throws IOException { @@ -337,7 +372,8 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); - buffer.putShort(filenameLength); + // the length of the encoded bytes, never String.length() + buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.put(filename); @@ -376,7 +412,6 @@ private static class CDFileHeader { int crc32; int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength; final short fileCommentLength = 0; final short diskNumberStart = 0; @@ -397,6 +432,7 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); + // the length of the encoded bytes, never String.length() buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.putShort(fileCommentLength); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java index fa2014bd..fc7be580 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -16,6 +16,7 @@ import java.nio.ByteOrder; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; import java.util.Random; @@ -23,6 +24,7 @@ import java.util.stream.IntStream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class ZipReaderTest { @@ -258,6 +260,209 @@ private static void assertReadsEveryEntry(byte[] archive) throws IOException { } } + /** + * An archive missing the trailing records it needs is not a zip, and has to say so rather + * than fail somewhere downstream. These all reach the same rejection, by different routes: + * the scan finds no signature within its window. + */ + @Test + public void testTruncatedArchiveIsRejected() throws IOException { + var archive = zip64Archive(); + + // the trailing end of central directory record and its locator are gone + var withoutTrailingRecords = + Arrays.copyOf(archive, archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE)); + assertThatThrownBy(() -> readArchive(withoutTrailingRecords)) + .isInstanceOf(InvalidZipException.class); + + // only the last few bytes of the record are gone + var clippedRecord = Arrays.copyOf(archive, archive.length - 4); + assertThatThrownBy(() -> readArchive(clippedRecord)) + .isInstanceOf(InvalidZipException.class); + + // cut down to less than a single end of central directory record + var shorterThanARecord = Arrays.copyOf(archive, EOCD_SIZE - 1); + assertThatThrownBy(() -> readArchive(shorterThanARecord)) + .isInstanceOf(InvalidZipException.class); + + assertThatThrownBy(() -> readArchive(new byte[0])) + .isInstanceOf(InvalidZipException.class); + } + + /** + * The end of central directory record claims a zip64 locator that the archive is too short to + * hold. Reaching for it has to be a zip error rather than an out of range seek. + */ + @Test + public void testArchiveTooShortForTheZip64LocatorIsRejected() throws IOException { + var archive = zip64Archive(); + // drop everything before the trailing records, leaving the end of central directory (which + // still carries its sentinels) with nothing in front of it + var truncated = Arrays.copyOfRange(archive, archive.length - EOCD_SIZE, archive.length); + + assertThatThrownBy(() -> readArchive(truncated)).isInstanceOf(InvalidZipException.class); + } + + private static void readArchive(byte[] archive) throws IOException { + try (var channel = new SeekableInMemoryByteChannel(archive)) { + new ZipReader(channel); + } + } + + /** + * A channel may satisfy a read with fewer bytes than were asked for while more are still + * available. Every multi-byte field the reader parses has to cope with that, or a valid + * archive read through such a channel is rejected as corrupt — and {@code loadTDF} takes a + * caller-supplied channel, so this is reachable from the public API. + */ + @Test + public void testArchiveReadThroughAChannelThatReturnsShortReads() throws IOException { + var archive = zip64Archive(); + + for (int maxRead = 1; maxRead <= 8; maxRead++) { + try (var channel = new ShortReadChannel(new SeekableInMemoryByteChannel(archive), maxRead)) { + var reader = new ZipReader(channel); + assertThat(reader.getEntries()) + .withFailMessage("reading %d byte(s) at a time lost entries", maxRead) + .hasSize(3); + assertThat(readEntry(reader, "0.payload")) + .withFailMessage("reading %d byte(s) at a time corrupted the payload", maxRead) + .isEqualTo(PAYLOAD); + } + } + } + + /** + * Trailing bytes push the end of central directory record back from the end of the archive. + * Everything the reader derives from it — the zip64 locator above all — has to be measured + * from where the record actually is, not from the end of the file. + */ + @Test + public void testZip64ArchiveWithTrailingDataStillReads() throws IOException { + var archive = zip64Archive(); + var padded = Arrays.copyOf(archive, archive.length + 100); + + assertReadsEveryEntry(padded); + } + + /** + * The record can only be pushed back by its own comment, whose length field caps it at 65,535 + * bytes, so the scan stops there. Beyond that limit an archive is not one we can read, and + * saying so immediately is the point: an unbounded scan reads its way back through the whole + * file a byte at a time before reaching the same conclusion. + */ + @Test + public void testEndOfCentralDirectoryPushedBeyondTheCommentLimitIsRejected() throws IOException { + var archive = zip64Archive(); + var padded = Arrays.copyOf(archive, archive.length + 0xFFFF + 1); + + assertThatThrownBy(() -> readArchive(padded)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + + /** + * The zip64 locator's pointer to the zip64 end of central directory record is a 64-bit value + * taken straight from the archive, so a corrupt one can point anywhere. Following it has to + * be a zip error rather than an out of range seek. + */ + @Test + public void testZip64LocatorPointingOutsideTheArchiveIsRejected() throws IOException { + // the locator sits between the zip64 end of central directory record and the trailing + // end of central directory record; its pointer is 8 bytes in, after the signature and + // the disk number + int pointer = zip64Archive().length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE) + 8; + + for (long badOffset : new long[] { -1L, Long.MIN_VALUE, 1L << 40 }) { + var archive = zip64Archive(); + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putLong(pointer, badOffset); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an offset of %d should be rejected as a zip error", badOffset) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + } + + /** + * The same for the central directory offset the zip64 record carries, which is the other + * 64-bit offset the reader seeks to. + */ + @Test + public void testZip64CentralDirectoryOffsetOutsideTheArchiveIsRejected() throws IOException { + var archive = zip64Archive(); + int zip64Eocd = archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE + ZIP64_EOCD_SIZE); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt(zip64Eocd)).isEqualTo(ZIP64_EOCD_SIGNATURE); + buf.putLong(zip64Eocd + 48, -1L); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + + /** Hands back at most {@code maxRead} bytes per read, as a channel is permitted to do. */ + private static final class ShortReadChannel implements SeekableByteChannel { + private final SeekableByteChannel delegate; + private final int maxRead; + + ShortReadChannel(SeekableByteChannel delegate, int maxRead) { + this.delegate = delegate; + this.maxRead = maxRead; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + if (!dst.hasRemaining()) { + return 0; + } + int limit = dst.limit(); + dst.limit(dst.position() + Math.min(maxRead, dst.remaining())); + try { + return delegate.read(dst); + } finally { + dst.limit(limit); + } + } + + @Override + public int write(ByteBuffer src) throws IOException { + return delegate.write(src); + } + + @Override + public long position() throws IOException { + return delegate.position(); + } + + @Override + public SeekableByteChannel position(long newPosition) throws IOException { + delegate.position(newPosition); + return this; + } + + @Override + public long size() throws IOException { + return delegate.size(); + } + + @Override + public SeekableByteChannel truncate(long size) throws IOException { + delegate.truncate(size); + return this; + } + + @Override + public boolean isOpen() { + return delegate.isOpen(); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + } + private static String readEntry(ZipReader reader, String name) throws IOException { var entry = reader.getEntries().stream() .filter(e -> e.getName().equals(name)) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java index d1d287b2..4bb36aa3 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java @@ -15,6 +15,8 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.nio.channels.FileChannel; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; @@ -22,9 +24,9 @@ import java.util.Random; import java.util.zip.CRC32; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.assertj.core.api.AssertionsForClassTypes.assertThat; public class ZipWriterTest { @Test @@ -157,6 +159,132 @@ public void rejectsAnOutOfRangeZip64Threshold() { assertThatThrownBy(() -> new ZipWriter(out, 1L << 32)).isInstanceOf(IllegalArgumentException.class); } + /** + * The entry count in the end of central directory record is a 2-byte field. An archive can + * need zip64 for its entry count alone while every one of its entries, and its whole central + * directory, stays comfortably inside 32 bits. + *

+ * The threshold is {@link Short#MAX_VALUE} rather than the {@code 0xFFFE} the format allows, + * so that a reader widening the field with a signed read never sees a negative count. Going + * over it has to produce a real zip64 record, not a count of {@code -32768}. + */ + @Test + public void entryCountAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + var justFits = archiveOfEmptyEntries(Short.MAX_VALUE, ZipWriter.MAX_NON_ZIP64_VALUE); + assertThat(endOfCentralDirectory(justFits).totalEntries).isEqualTo(Short.MAX_VALUE); + assertThat(containsZip64EndOfCentralDirectory(justFits)) + .withFailMessage("an archive whose entry count still fits should not be zip64") + .isFalse(); + // the non-zip64 side of the boundary has to read back too, not just look right + try (var chan = new SeekableInMemoryByteChannel(justFits)) { + assertThat(new ZipReader(chan).getEntries()).hasSize(Short.MAX_VALUE); + } + + var overflows = archiveOfEmptyEntries(Short.MAX_VALUE + 1, ZipWriter.MAX_NON_ZIP64_VALUE); + var eocd = endOfCentralDirectory(overflows); + assertThat(eocd.totalEntries).isEqualTo(0xFFFF); + assertThat(eocd.entriesOnThisDisk).isEqualTo(0xFFFF); + assertThat(containsZip64EndOfCentralDirectory(overflows)) + .withFailMessage("the entry count no longer fits, so the archive has to be zip64") + .isTrue(); + + // and the real count survives, which it only can if it went into the zip64 record + try (var chan = new SeekableInMemoryByteChannel(overflows)) { + assertThat(new ZipReader(chan).getEntries()).hasSize(Short.MAX_VALUE + 1); + } + } + + /** + * The central directory offset is a 4-byte field. Held to the same 2 GiB ceiling as the + * per-entry fields, so the lowered threshold drives it here. + */ + @Test + public void centralDirectoryOffsetAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // one entry, small enough to stay non-zip64 itself, whose data pushes the start of the + // central directory past the threshold while the directory stays under it + var belowThreshold = archiveOfOneEntry(50, 100); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + var archive = archiveOfOneEntry(100, 100); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.offsetOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory past the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "big.bin"); + } + + /** + * The central directory size is also a 4-byte field. An empty entry costs 46 bytes in the + * directory but only 30 before it, so a pile of them makes the directory outgrow its own + * offset and this sentinel fires while the offset one does not. + */ + @Test + public void centralDirectorySizeAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // 5 entries: the directory starts at 180 and is 260 bytes long, both under the threshold + var belowThreshold = archiveOfEmptyEntries(5, 400); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + // 10 entries: the directory still starts at 360 but is now 520 bytes long + var archive = archiveOfEmptyEntries(10, 400); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.sizeOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory larger than the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "e00000"); + } + + /** + * Zip counts filename lengths in bytes. Deriving one from {@link String#length()}, which + * counts UTF-16 code units, desyncs the central directory by the difference for any entry + * name that isn't pure ASCII. + */ + @Test + public void filenameLengthIsMeasuredInUtf8Bytes() throws IOException { + // a surrogate pair (2 code units, 4 bytes) and a BMP character (1 code unit, 3 bytes), + // so String.length() is 7 where the encoded form is 11 bytes + var name = "🔒両.txt"; + var nameBytes = name.getBytes(StandardCharsets.UTF_8); + assertThat(name.length()).isNotEqualTo(nameBytes.length); + + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + writer.data(name, "contents".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + // the sole local file header starts at 0, and its filename length is at offset 26 + assertThat(readUnsignedShort(archive, 26)).isEqualTo(nameBytes.length); + // the central directory file header keeps its filename length at offset 28 + var centralDirectory = (int) endOfCentralDirectory(archive).offsetOfCentralDirectory; + assertThat(readUnsignedShort(archive, centralDirectory + 28)).isEqualTo(nameBytes.length); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(readEntry(new ZipReader(chan), name)).isEqualTo("contents"); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(getDataStream(z, z.getEntry(name)).toString(StandardCharsets.UTF_8)) + .isEqualTo("contents"); + } + } + + @Test + public void rejectsAnEntryNameTooLongToDescribe() { + var name = "両".repeat(30_000); // 3 bytes each, so well past the 0xFFFF byte field + var writer = new ZipWriter(new ByteArrayOutputStream()); + assertThatThrownBy(() -> writer.data(name, new byte[0])) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + } + @Test @Disabled("this takes a long time and shouldn't run on build machines") public void testWritingLargeFile() throws IOException { @@ -282,6 +410,86 @@ private static boolean containsZip64EndOfCentralDirectory(byte[] archive) { return false; } + private static final long ZIP64_SENTINEL = 0xFFFFFFFFL; + private static final int END_OF_CENTRAL_DIRECTORY_SIZE = 22; + private static final int END_OF_CENTRAL_DIRECTORY_SIGNATURE = 0x06054b50; + + /** An archive of {@code count} empty entries, whose names are all the same length. */ + private static byte[] archiveOfEmptyEntries(int count, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + var empty = new byte[0]; + for (int i = 0; i < count; i++) { + writer.data(String.format("e%05d", i), empty); + } + writer.finish(); + return out.toByteArray(); + } + + /** An archive of one entry named {@code big.bin} carrying {@code dataSize} bytes. */ + private static byte[] archiveOfOneEntry(int dataSize, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + writer.data("big.bin", new byte[dataSize]); + writer.finish(); + return out.toByteArray(); + } + + /** + * Shows that it was an end of central directory field, and not an entry, that made the + * archive zip64: no entry carries a zip64 extra field, and the whole thing still reads. + */ + private static void assertOnlyTheEndOfCentralDirectoryIsZip64(byte[] archive, String anEntryName) + throws IOException { + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(zip64ExtraField(z, anEntryName)) + .withFailMessage("no entry should be zip64 here, only the end of central directory") + .isNull(); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(new ZipReader(chan).getEntries()) + .withFailMessage("the archive should still be readable") + .isNotEmpty(); + } + } + + private static int readUnsignedShort(byte[] archive, int position) { + return ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).getShort(position) & 0xFFFF; + } + + /** The fields of the trailing end of central directory record. We never write a comment. */ + private static final class EndOfCentralDirectory { + final int entriesOnThisDisk; + final int totalEntries; + final long sizeOfCentralDirectory; + final long offsetOfCentralDirectory; + + EndOfCentralDirectory(int entriesOnThisDisk, int totalEntries, long sizeOfCentralDirectory, + long offsetOfCentralDirectory) { + this.entriesOnThisDisk = entriesOnThisDisk; + this.totalEntries = totalEntries; + this.sizeOfCentralDirectory = sizeOfCentralDirectory; + this.offsetOfCentralDirectory = offsetOfCentralDirectory; + } + } + + private static EndOfCentralDirectory endOfCentralDirectory(byte[] archive) { + var buf = ByteBuffer + .wrap(archive, archive.length - END_OF_CENTRAL_DIRECTORY_SIZE, END_OF_CENTRAL_DIRECTORY_SIZE) + .order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt()) + .withFailMessage("the archive doesn't end in an end of central directory record") + .isEqualTo(END_OF_CENTRAL_DIRECTORY_SIGNATURE); + buf.getShort(); // disk number + buf.getShort(); // disk the central directory starts on + return new EndOfCentralDirectory( + buf.getShort() & 0xFFFF, + buf.getShort() & 0xFFFF, + buf.getInt() & 0xFFFFFFFFL, + buf.getInt() & 0xFFFFFFFFL); + } + private static long crcOfWholeFile(File file) throws IOException { var crc = new CRC32(); var buf = new byte[1 << 16];