From 87436e6d0ef330a2f68f4e8ba07c5c086fe8137d Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 9 Sep 2026 08:50:12 -0400 Subject: [PATCH 1/2] 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 | 210 ++++++-- .../io/opentdf/platform/sdk/ZipWriter.java | 84 +++- .../opentdf/platform/sdk/ZipReaderTest.java | 458 +++++++++++++++++- .../opentdf/platform/sdk/ZipWriterTest.java | 252 +++++++++- 4 files changed, 943 insertions(+), 61 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..1b3f577e 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -3,7 +3,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; @@ -25,23 +24,64 @@ 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; + /** + * How many reads in a row may come back empty before the channel is taken to have stalled. + */ + private static final int MAX_CONSECUTIVE_EMPTY_READS = 16; + + /** + * Reads at least one byte into {@code buf}, which must have room for one. Only {@code -1} + * from {@link SeekableByteChannel#read} is an end of file; {@code 0} just means nothing + * arrived this time, which a non-blocking channel is allowed to do. Empty reads are retried, + * but only {@value #MAX_CONSECUTIVE_EMPTY_READS} times in a row, so that a channel with + * nothing to give fails instead of spinning forever. + * + * @return the number of bytes read, which is always positive, or {@code -1} at end of file + * @throws IOException if the channel keeps returning nothing + */ + private int readSome(ByteBuffer buf) throws IOException { + for (int attempt = 0; attempt < MAX_CONSECUTIVE_EMPTY_READS; attempt++) { + int read = zipChannel.read(buf); + if (read != 0) { + return read; + } + } + throw new IOException("channel returned no data " + MAX_CONSECUTIVE_EMPTY_READS + + " times in a row at offset " + zipChannel.position()); + } + + /** + * 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. + * + * @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 (readSome(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 +104,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,25 +140,81 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { private static final int ZIP64_MAGIC_SHORT = 0xFFFF; private static final int ZIP64_EXTID= 0x0001; + /** + * The longest comment the end of central directory record can carry, since its length lives + * in a 2-byte field. This is how far back from the end of the archive the scan for the record + * looks. Trailing data after the record is tolerated, but only within this same budget, + * because nothing records how long it is. + */ + private static final long MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE = 0xFFFF; + + /** The fixed part of a central directory file header, before its variable length fields. */ + private static final int CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE = 46; + + /** + * 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 offset escapes as an {@link IllegalArgumentException} from the channel rather than + * as a zip error, and one past the end fails at the next read with a generic complaint that + * names neither the offset nor the field it came from. + */ + 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); + } + + /** + * Checks that the comment of a candidate end of central directory record ends inside the + * archive, as a real record's comment does. The backward scan can meet a stray signature + * inside the comment or in trailing data before it reaches the real record; this rules out + * most of them, along with a real record whose comment was cut short. Trailing data after + * the comment is allowed. + */ + private boolean commentEndsWithinArchive(long eoCDRStart) throws IOException { + zipChannel.position(eoCDRStart + END_OF_CENTRAL_DIRECTORY_SIZE - Short.BYTES); + int commentLength = readUnsignedShort(); + return eoCDRStart + END_OF_CENTRAL_DIRECTORY_SIZE + commentLength <= zipChannel.size(); + } + CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { long eoCDRStart = zipChannel.size() - END_OF_CENTRAL_DIRECTORY_SIZE; // 22 is the minimum size of the EOCDR - - while (eoCDRStart >= 0) { + // the record can only be pushed back from the end of the archive by its own comment, plus + // whatever trailing data fits in the same budget, so there is no reason to look back any + // further than the longest possible comment. 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) { - if (logger.isDebugEnabled()) { - logger.debug("Found end of central directory signature at {}", zipChannel.position() - Integer.BYTES); - } + if (signature == null) { + // every offset this scan probes has a whole record behind it, so an end of file + // here means the channel holds less than its size claims + throw new InvalidZipException("Archive ended at offset " + zipChannel.position() + + ", before its reported size of " + zipChannel.size() + " bytes"); + } + if (signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE && commentEndsWithinArchive(eoCDRStart)) { + logger.debug("Found end of central directory signature at {}", eoCDRStart); + 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"); } + zipChannel.position(eoCDRStart + Integer.BYTES); short diskNumber = readShort(); short centralDirectoryDiskNumber = readShort(); short numCDEntriesOnThisDisk = readShort(); @@ -128,7 +222,7 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { int totalNumEntries = readUnsignedShort(); long sizeOfCentralDirectory = readUnsignedInt(); long offsetToStartOfCentralDirectory = readUnsignedInt(); - int commentLength = readUnsignedShort(); + // the comment length comes next, and the scan has already checked it // 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,26 +234,39 @@ 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); - zipChannel.position(zip64CentralDirectoryLocatorStart); - return extractZIP64CentralDirectoryInfo(); + // 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 but a comment whose 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"); + } + return extractZIP64CentralDirectoryInfo(zip64CentralDirectoryLocatorStart); } - private CentralDirectoryRecord extractZIP64CentralDirectoryInfo() throws IOException { - // buffer's position at the start of the Central Directory + private CentralDirectoryRecord extractZIP64CentralDirectoryInfo(long locatorStart) throws IOException { + zipChannel.position(locatorStart); Integer signature = readInteger(); if (signature == null || signature != ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIGNATURE) { - throw new InvalidZipException("Invalid Zip64 End of Central Directory Record Signature"); + throw new InvalidZipException("Invalid zip64 end of central directory locator signature at offset " + + locatorStart + ": expected 0x" + + Integer.toHexString(ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIGNATURE) + + " but found " + (signature == null ? "the end of the archive" : "0x" + Integer.toHexString(signature))); } int centralDirectoryDiskNumber = readInt(); 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(); @@ -241,10 +348,8 @@ public int read() throws IOException { } setChannelPosition(); buf.clear(); - while (buf.hasRemaining()) { - if (zipChannel.read(buf) <= 0) { - return -1; - } + if (readSome(buf) < 0) { + throw truncated(); } offset += 1; return buf.array()[0] & 0xFF; @@ -254,6 +359,15 @@ private boolean doneReading() { return offset >= fileSize; } + /** + * The archive ended before this entry's data did. Reporting that as an ordinary + * end of stream would hand the caller a short entry with no sign anything is wrong. + */ + private InvalidZipException truncated() { + return new InvalidZipException("Archive ended " + offset + " bytes into the " + + fileSize + " byte entry [" + fileName + "]"); + } + private void setChannelPosition() throws IOException { var nextPosition = startPosition + offset; if (zipChannel.position() != nextPosition) { @@ -263,16 +377,22 @@ private void setChannelPosition() throws IOException { @Override public int read(byte[] b, int off, int len) throws IOException { + if (len == 0) { + return 0; + } if (doneReading()) { return -1; } setChannelPosition(); var lenToRead = (int)Math.min(len, fileSize - offset); // cast is always valid because len is an int var buf = ByteBuffer.wrap(b, off, lenToRead); - var nread = zipChannel.read(buf); - if (nread > 0) { - offset += nread; + // InputStream must not return 0 for a non-empty request, so this has to wait + // for at least one byte rather than pass an empty channel read through + var nread = readSome(buf); + if (nread < 0) { + throw truncated(); } + offset += nread; return nread; } }; @@ -300,11 +420,12 @@ public Entry readCentralDirectoryFileHeader() throws IOException { int externalFileAttributes = readInt(); long relativeOffsetOfLocalHeader = readUnsignedInt(); + long fileNameStart = zipChannel.position(); ByteBuffer fileName = ByteBuffer.allocate(fileNameLength); - while (fileName.hasRemaining()) { - if (zipChannel.read(fileName) <= 0) { - throw new EOFException("Unexpected EOF when reading filename of length: " + fileNameLength); - } + if (!fill(fileName)) { + throw new InvalidZipException("Archive ended inside the " + fileNameLength + + " byte filename of the central directory file header at offset " + + (fileNameStart - CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE)); } // Parse the extra field @@ -341,8 +462,17 @@ public Entry readCentralDirectoryFileHeader() throws IOException { public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); - zipChannel.position(centralDirectoryRecord.offsetToStart); - for (int i = 0; i < centralDirectoryRecord.numEntries; i++) { + seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); + // the zip64 count is a 64-bit value straight from the archive. a corrupt one that comes + // out negative would otherwise read as an empty archive, with no sign anything was wrong + long bytesAvailable = zipChannel.size() - centralDirectoryRecord.offsetToStart; + long mostEntriesThatFit = bytesAvailable / CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; + if (centralDirectoryRecord.numEntries < 0 || centralDirectoryRecord.numEntries > mostEntriesThatFit) { + throw new InvalidZipException("The central directory claims " + centralDirectoryRecord.numEntries + + " entries, but the " + bytesAvailable + " bytes from its start to the end of the archive" + + " can hold at most " + mostEntriesThatFit); + } + for (long 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..55cdd045 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,30 @@ 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 above 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 longest entry name, in UTF-8 bytes, we will write. The filename length field is 2 bytes + * and unsigned, so the format allows {@code 0xFFFF}. We stop at {@link Short#MAX_VALUE} for + * the same reason as {@link #MAX_NON_ZIP64_ENTRY_COUNT}: versions of this SDK that predate + * the unsigned reads in {@link ZipReader} read the length as signed, and a negative length + * fails there. + */ + static final int MAX_FILENAME_LENGTH = Short.MAX_VALUE; + 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 +105,40 @@ 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 [" + abbreviate(name) + "] is " + bytes.length + + " bytes when encoded as UTF-8, more than the " + MAX_FILENAME_LENGTH + + " bytes we write into the filename length field"); + } + return bytes; + } + + private static final int ABBREVIATED_NAME_CODE_POINTS = 32; + + /** The start of a name too long to put in an error message whole. */ + private static String abbreviate(String name) { + if (name.codePointCount(0, name.length()) <= ABBREVIATED_NAME_CODE_POINTS) { + return name; + } + return name.substring(0, name.offsetByCodePoints(0, ABBREVIATED_NAME_CODE_POINTS)) + "..."; + } + 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 +218,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 +236,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,24 +256,25 @@ 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; + // the name is UTF-8, and readers that go by the local header need to be told so just as + // much as those that go by the central directory + localFileHeader.generalPurposeBitFlag = (1 << 11); 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); var fileInfo = new FileInfo(); fileInfo.offset = startPosition; - fileInfo.flag = (1 << 11); + fileInfo.flag = (short) localFileHeader.generalPurposeBitFlag; fileInfo.size = data.length; fileInfo.crc = crcValue; fileInfo.filename = name; @@ -238,10 +287,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 +376,6 @@ private static class LocalFileHeader { int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength = 0; void write(OutputStream out, byte[] filename) throws IOException { @@ -337,7 +390,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 +430,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 +450,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..6c9cebba 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -16,13 +16,18 @@ import java.nio.ByteOrder; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Random; import java.util.stream.Collectors; import java.util.stream.IntStream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; public class ZipReaderTest { @@ -241,11 +246,7 @@ private static byte[] keepOnlyTheEntryCountSentinel(byte[] archive) { private static void assertReadsEveryEntry(byte[] archive) throws IOException { try (var channel = new SeekableInMemoryByteChannel(archive)) { - var reader = new ZipReader(channel); - assertThat(reader.getEntries()).hasSize(3); - assertThat(readEntry(reader, "small.txt")).isEqualTo("tiny"); - assertThat(readEntry(reader, "0.payload")).isEqualTo(PAYLOAD); - assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + assertReadsEveryEntry(channel); } // and an independent implementation, so this shows the patched archive is well formed @@ -258,6 +259,453 @@ private static void assertReadsEveryEntry(byte[] archive) throws IOException { } } + /** Our reader alone, for archives or channels an independent implementation can't take. */ + private static void assertReadsEveryEntry(SeekableByteChannel channel) throws IOException { + var reader = new ZipReader(channel); + assertThat(reader.getEntries()).hasSize(3); + assertThat(readEntry(reader, "small.txt")).isEqualTo("tiny"); + assertThat(readEntry(reader, "0.payload")).isEqualTo(PAYLOAD); + assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + } + + /** + * 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)); + // only the last few bytes of the record are gone + var clippedRecord = Arrays.copyOf(archive, archive.length - 4); + // cut down to less than a single end of central directory record + var shorterThanARecord = Arrays.copyOf(archive, EOCD_SIZE - 1); + + for (var truncated : List.of(withoutTrailingRecords, clippedRecord, shorterThanARecord, new byte[0])) { + assertThatThrownBy(() -> readArchive(truncated)) + .withFailMessage("a %d byte truncation should fail the scan", truncated.length) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + } + + /** + * 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) + .hasMessageContaining("too small to hold the zip64 end of central directory locator"); + } + + 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"); + } + + /** + * The largest comment the format allows pushes the end of central directory record as far + * back as it can go. That is the edge of the scan window, and the record still has to be + * found there — and the zip64 locator in front of it, measured from the record rather than + * from the end of the archive. + */ + @Test + public void testZip64ArchiveWithTheLongestPossibleCommentStillReads() throws IOException { + var comment = new byte[0xFFFF]; + Arrays.fill(comment, (byte) 'c'); + + assertReadsEveryEntry(withComment(zip64Archive(), comment)); + } + + /** + * A comment can contain anything, including the end of central directory signature. Scanning + * backwards meets that one first; it has to be passed over because the comment it would + * carry runs off the end of the archive, which the real record's does not. + */ + @Test + public void testSignatureInsideTheCommentIsNotMistakenForTheRecord() throws IOException { + var comment = new byte[64]; + Arrays.fill(comment, (byte) 0xFF); // so the decoy's own comment length reads as 0xFFFF + ByteBuffer.wrap(comment).order(ByteOrder.LITTLE_ENDIAN).putInt(0, EOCD_SIGNATURE); + + // commons-compress takes the decoy for the record, so this one is ours alone + try (var channel = new SeekableInMemoryByteChannel(withComment(zip64Archive(), comment))) { + assertReadsEveryEntry(channel); + } + } + + /** A record whose comment was cut short isn't the end of a whole archive. */ + @Test + public void testRecordWhoseCommentRunsPastTheEndIsRejected() throws IOException { + var withFullComment = withComment(zip64Archive(), new byte[10]); + var truncated = Arrays.copyOf(withFullComment, withFullComment.length - 5); + + assertThatThrownBy(() -> readArchive(truncated)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("Didn't find the end of central directory"); + } + + /** + * Gives the trailing end of central directory record a comment. Assumes, as is true of + * everything our writer produces, that the archive doesn't already have one. + */ + private static byte[] withComment(byte[] archive, byte[] comment) { + var result = Arrays.copyOf(archive, archive.length + comment.length); + ByteBuffer.wrap(result).order(ByteOrder.LITTLE_ENDIAN) + .putShort(archive.length - Short.BYTES, (short) comment.length); + System.arraycopy(comment, 0, result, archive.length, comment.length); + return result; + } + + /** + * A channel is allowed to return zero bytes without being at the end of the stream; only + * {@code -1} means that. An empty read has to be retried, not taken for the end of the + * archive. + */ + @Test + public void testArchiveReadThroughAChannelThatReturnsEmptyReads() throws IOException { + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 1)) { + assertReadsEveryEntry(channel); + } + } + + /** + * But a channel that never returns anything has to fail, rather than leave the reader + * spinning on it — whether it stops before the archive is opened or partway through an entry. + */ + @Test + public void testChannelThatStopsReturningDataFails() throws IOException { + assertTimeoutPreemptively(Duration.ofSeconds(10), () -> { + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { + channel.stalled = true; + assertThatThrownBy(() -> new ZipReader(channel)) + .isInstanceOf(IOException.class) + .hasMessageContaining("no data"); + } + + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { + var reader = new ZipReader(channel); + channel.stalled = true; + assertThatThrownBy(() -> readEntry(reader, "0.payload")) + .isInstanceOf(IOException.class) + .hasMessageContaining("no data"); + } + }); + } + + /** + * The zip64 entry count is a 64-bit value taken straight from the archive. One that is + * negative would read as an empty archive, and one too large for the central directory to + * hold would send the reader past its end; both have to be zip errors instead. + */ + @Test + public void testZip64EntryCountTheCentralDirectoryCannotHoldIsRejected() throws IOException { + for (long badCount : new long[] { -1L, Long.MIN_VALUE, 1_000_000L }) { + 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 + 32, badCount); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an entry count of %d should be rejected as a zip error", badCount) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("can hold at most"); + } + } + + /** The same bounds check, on the 32-bit offset of an archive that isn't zip64 at all. */ + @Test + public void testCentralDirectoryOffsetOutsideTheArchiveIsRejected() throws IOException { + int length = plainArchive().length; + for (long badOffset : new long[] { length, 0xFFFFFFFEL }) { + var archive = plainArchive(); + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN) + .putInt(archive.length - EOCD_SIZE + 16, (int) badOffset); + + assertThatThrownBy(() -> readArchive(archive)) + .withFailMessage("an offset of %d should be rejected as a zip error", badOffset) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("outside this"); + } + } + + /** + * An archive with no entries has nothing but its end of central directory record, and its + * central directory offset points right at it. That is still inside the archive. + */ + @Test + public void testEmptyArchiveReads() throws IOException { + var out = new ByteArrayOutputStream(); + new ZipWriter(out).finish(); + + try (var channel = new SeekableInMemoryByteChannel(out.toByteArray())) { + assertThat(new ZipReader(channel).getEntries()).isEmpty(); + } + } + + /** A central directory that ends partway through a filename is a zip error like any other. */ + @Test + public void testCentralDirectoryFilenameRunningPastTheEndIsRejected() throws IOException { + var archive = plainArchive(); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + int centralDirectory = buf.getInt(archive.length - EOCD_SIZE + 16); + buf.putShort(centralDirectory + 28, (short) 0x7000); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("byte filename"); + } + + /** + * An entry whose data runs past the end of the archive is truncated, and reading it has to + * say so rather than end the stream early and hand back a short entry as if it were whole. + */ + @Test + public void testEntryRunningPastTheEndOfTheArchiveFailsToRead() throws IOException { + var archive = plainArchive(); + var buf = ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN); + int centralDirectory = buf.getInt(archive.length - EOCD_SIZE + 16); + // small.txt comes first; claim it is far bigger than the archive + buf.putInt(centralDirectory + 24, 1_000_000); + + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(channel); + assertThatThrownBy(() -> readEntry(reader, "small.txt")) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("bytes into the 1000000 byte entry [small.txt]"); + + // and a byte at a time, which takes a different path through the stream + assertThatThrownBy(() -> { + try (var data = reader.getEntries().get(0).getData()) { + while (data.read() != -1) { + // drain + } + } + }).isInstanceOf(InvalidZipException.class) + .hasMessageContaining("byte entry [small.txt]"); + } + } + + /** The same three entries as {@link #zip64Archive()}, written without anything zip64. */ + private static byte[] plainArchive() throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + writer.data("small.txt", "tiny".getBytes(StandardCharsets.UTF_8)); + writer.data("0.payload", PAYLOAD.getBytes(StandardCharsets.UTF_8)); + writer.data("0.manifest.json", MANIFEST.getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + assertReadsEveryEntry(archive); + return archive; + } + + /** Hands back at most {@code maxRead} bytes per read, as a channel is permitted to do. */ + private static final class ShortReadChannel extends DelegatingChannel { + private final int maxRead; + + ShortReadChannel(SeekableByteChannel delegate, int maxRead) { + super(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); + } + } + } + + /** + * Returns nothing {@code emptyReadsBeforeEachRead} times before every read that does return + * data, as a non-blocking channel is permitted to do, and nothing at all once {@code stalled}. + */ + private static final class StallingChannel extends DelegatingChannel { + private final int emptyReadsBeforeEachRead; + private int emptyReadsSoFar; + volatile boolean stalled; + + StallingChannel(SeekableByteChannel delegate, int emptyReadsBeforeEachRead) { + super(delegate); + this.emptyReadsBeforeEachRead = emptyReadsBeforeEachRead; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + if (stalled) { + return 0; + } + if (emptyReadsSoFar < emptyReadsBeforeEachRead) { + emptyReadsSoFar++; + return 0; + } + emptyReadsSoFar = 0; + return delegate.read(dst); + } + } + + private static class DelegatingChannel implements SeekableByteChannel { + protected final SeekableByteChannel delegate; + + DelegatingChannel(SeekableByteChannel delegate) { + this.delegate = delegate; + } + + @Override + public int read(ByteBuffer dst) throws IOException { + return delegate.read(dst); + } + + @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..aa6c3150 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java @@ -15,16 +15,19 @@ 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; import java.nio.file.StandardOpenOption; +import java.util.Locale; 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 +160,172 @@ 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); + + // and both headers flag the name as UTF-8 (general purpose bit 11), so a reader that goes + // by either one decodes it the same way + assertThat(readUnsignedShort(archive, 6) & UTF8_FLAG) + .withFailMessage("the local file header doesn't flag the name as UTF-8") + .isEqualTo(UTF8_FLAG); + assertThat(readUnsignedShort(archive, centralDirectory + 8) & UTF8_FLAG) + .withFailMessage("the central directory file header doesn't flag the name as UTF-8") + .isEqualTo(UTF8_FLAG); + + 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"); + } + } + + /** + * The longest name we write is {@link ZipWriter#MAX_FILENAME_LENGTH} UTF-8 bytes. One byte + * more is rejected on both the byte array and the streaming path — the second is how a TDF + * writes its payload — and a rejected name leaves nothing behind, so the writer can carry on. + */ + @Test + public void rejectsAnEntryNameTooLongToDescribe() throws IOException { + // "両" is 3 bytes, so this lands exactly on the limit only once padded out with ASCII + var longest = "両".repeat(ZipWriter.MAX_FILENAME_LENGTH / 3) + + "x".repeat(ZipWriter.MAX_FILENAME_LENGTH % 3); + assertThat(longest.getBytes(StandardCharsets.UTF_8)).hasSize(ZipWriter.MAX_FILENAME_LENGTH); + var oneTooMany = longest + "x"; + + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + assertThatThrownBy(() -> writer.data(oneTooMany, new byte[0])) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + assertThatThrownBy(() -> writer.stream(oneTooMany)) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + + writer.data(longest, "contents".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(chan); + assertThat(reader.getEntries()) + .withFailMessage("a rejected name should not leave an entry behind") + .hasSize(1); + assertThat(readEntry(reader, longest)).isEqualTo("contents"); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(getDataStream(z, z.getEntry(longest)).toString(StandardCharsets.UTF_8)) + .isEqualTo("contents"); + } + } + @Test @Disabled("this takes a long time and shouldn't run on build machines") public void testWritingLargeFile() throws IOException { @@ -282,6 +451,87 @@ private static boolean containsZip64EndOfCentralDirectory(byte[] archive) { return false; } + private static final long ZIP64_SENTINEL = 0xFFFFFFFFL; + private static final int UTF8_FLAG = 1 << 11; + 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(Locale.ROOT, "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]; From 8e39dc8791c670752a2590de7f711788ce4b4bfa Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 14 Oct 2026 09:36:39 -0400 Subject: [PATCH 2/2] fix(sdk): tighten zip reader errors and comments, cover missed paths (DSPX-4589) Follow-ups from review of the zip64 change. - Correct the writer comment on end of central directory sentinels: once an archive is zip64, every field gets the sentinel, which is what lets readers before 0.19.0 find the zip64 record. - Say plainly that ZipReader needs a blocking channel, in the readSome Javadoc and in the error raised when a channel stalls. - Report the entry, offset and found value for a bad local file header signature. - Trim comments that described the old bug instead of the current code, and name the 0.19.0 release instead of "versions that predate". - Test a zero-length read, a missing or misdirected zip64 locator, a channel smaller than its reported size, a stall inside the entry stream, and a non-ASCII name written through stream(); check the 32,768-entry archive with commons-compress too. - Match error messages on the entry name and size, not their wording. --- .../io/opentdf/platform/sdk/ZipReader.java | 57 ++++---- .../io/opentdf/platform/sdk/ZipWriter.java | 36 +++-- .../opentdf/platform/sdk/ZipReaderTest.java | 127 +++++++++++++++--- .../opentdf/platform/sdk/ZipWriterTest.java | 43 +++++- 4 files changed, 192 insertions(+), 71 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 1b3f577e..da1a4a83 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -31,10 +31,9 @@ public class ZipReader { /** * Reads at least one byte into {@code buf}, which must have room for one. Only {@code -1} - * from {@link SeekableByteChannel#read} is an end of file; {@code 0} just means nothing - * arrived this time, which a non-blocking channel is allowed to do. Empty reads are retried, - * but only {@value #MAX_CONSECUTIVE_EMPTY_READS} times in a row, so that a channel with - * nothing to give fails instead of spinning forever. + * from {@link SeekableByteChannel#read} means end of file. An occasional {@code 0} is + * retried, but without waiting: after {@value #MAX_CONSECUTIVE_EMPTY_READS} empty reads in a + * row this gives up rather than spin. Non-blocking channels are not supported. * * @return the number of bytes read, which is always positive, or {@code -1} at end of file * @throws IOException if the channel keeps returning nothing @@ -46,17 +45,17 @@ private int readSome(ByteBuffer buf) throws IOException { return read; } } - throw new IOException("channel returned no data " + MAX_CONSECUTIVE_EMPTY_READS - + " times in a row at offset " + zipChannel.position()); + throw new IOException(zipChannel.getClass().getName() + " returned no data " + + MAX_CONSECUTIVE_EMPTY_READS + " times in a row at offset " + zipChannel.position() + + "; ZipReader requires a blocking channel"); } /** - * 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. + * Fills {@code buf} from the channel, reading as many times as it takes, since a single + * {@link SeekableByteChannel#read} may come up short without being at the end of the file. * * @return false at end of file, in which case {@code buf} holds nothing worth reading + * @throws IOException if the channel stalls (see {@link #readSome}) */ private boolean fill(ByteBuffer buf) throws IOException { buf.clear(); @@ -143,8 +142,8 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { /** * The longest comment the end of central directory record can carry, since its length lives * in a 2-byte field. This is how far back from the end of the archive the scan for the record - * looks. Trailing data after the record is tolerated, but only within this same budget, - * because nothing records how long it is. + * looks, so trailing data after the record is tolerated only if it fits in the same window + * as the comment. */ private static final long MAX_END_OF_CENTRAL_DIRECTORY_COMMENT_SIZE = 0xFFFF; @@ -152,9 +151,8 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { private static final int CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE = 46; /** - * 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 offset escapes as an {@link IllegalArgumentException} from the channel rather than + * Positions the channel at an offset read from the archive, which a corrupt archive can set + * to anything, in a 32-bit field or a zip64 one. Unchecked, a negative offset escapes as an {@link IllegalArgumentException} from the channel rather than * as a zip error, and one past the end fails at the next read with a generic complaint that * names neither the offset nor the field it came from. */ @@ -181,12 +179,10 @@ private boolean commentEndsWithinArchive(long eoCDRStart) throws IOException { CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { long eoCDRStart = zipChannel.size() - END_OF_CENTRAL_DIRECTORY_SIZE; // 22 is the minimum size of the EOCDR - // the record can only be pushed back from the end of the archive by its own comment, plus - // whatever trailing data fits in the same budget, so there is no reason to look back any - // further than the longest possible comment. 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 + // in a conformant archive only the comment follows the record, so the scan stops after the + // longest possible comment. an unbounded scan does a positioned four byte read for every + // byte of the archive before it can reject a non-zip, and 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)); @@ -195,8 +191,8 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { zipChannel.position(eoCDRStart); Integer signature = readInteger(); if (signature == null) { - // every offset this scan probes has a whole record behind it, so an end of file - // here means the channel holds less than its size claims + // every probed offset is at most size - 22, so a whole record's worth of bytes + // should follow it; an end of file here means the channel holds less than size() throw new InvalidZipException("Archive ended at offset " + zipChannel.position() + ", before its reported size of " + zipChannel.size() + " bytes"); } @@ -222,7 +218,6 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { int totalNumEntries = readUnsignedShort(); long sizeOfCentralDirectory = readUnsignedInt(); long offsetToStartOfCentralDirectory = readUnsignedInt(); - // the comment length comes next, and the scan has already checked it // 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 @@ -234,10 +229,8 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { return new CentralDirectoryRecord(totalNumEntries, offsetToStartOfCentralDirectory); } - // 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 but a comment whose length is honest; measuring from the end lands at the wrong - // offset for anything else and blames the locator for it + // the locator immediately precedes the record (APPNOTE 4.3.15), so measure from the record + // rather than from the end of the archive, which may hold trailing data long zip64CentralDirectoryLocatorStart = eoCDRStart - ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE; if (zip64CentralDirectoryLocatorStart < 0) { throw new InvalidZipException( @@ -317,7 +310,9 @@ private long findStartOfData() throws IOException { zipChannel.position(offsetToLocalHeader); Integer signature = readInteger(); if (signature == null || signature != LOCAL_FILE_HEADER_SIGNATURE) { - throw new InvalidZipException("Invalid Local Header Signature"); + throw new InvalidZipException("Invalid local file header signature for entry [" + fileName + + "] at offset " + offsetToLocalHeader + ": " + + (signature == null ? "the archive ends there" : "found 0x" + Integer.toHexString(signature))); } zipChannel.position(zipChannel.position() + Short.BYTES @@ -463,8 +458,8 @@ public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); - // the zip64 count is a 64-bit value straight from the archive. a corrupt one that comes - // out negative would otherwise read as an empty archive, with no sign anything was wrong + // a corrupt zip64 count can be negative, which would read as an empty archive, or more + // than the rest of the archive can hold, which would read past the central directory long bytesAvailable = zipChannel.size() - centralDirectoryRecord.offsetToStart; long mostEntriesThatFit = bytesAvailable / CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; if (centralDirectoryRecord.numEntries < 0 || centralDirectoryRecord.numEntries > mostEntriesThatFit) { 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 55cdd045..ac65eb3e 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java @@ -26,8 +26,7 @@ public class ZipWriter { * The fields are unsigned on the wire, but readers that widen them with a signed read see * anything at or above 2 GiB as negative. Switching to ZIP64 at 2 GiB costs * {@value #ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE} bytes per affected entry and keeps - * those readers working, including versions of this SDK that predate the unsigned reads in - * {@link ZipReader}. + * those readers working, including SDK releases before 0.19.0. */ static final long MAX_NON_ZIP64_VALUE = Integer.MAX_VALUE; @@ -36,21 +35,19 @@ public class ZipWriter { * {@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 above 32,767 entries costs one zip64 end of central - * directory record and locator for the whole archive. + * We stop at {@link Short#MAX_VALUE} instead, for the reason given on + * {@link #MAX_NON_ZIP64_VALUE}: SDK releases before 0.19.0 read the count as signed, see any + * count above {@code 0x7FFF} as negative, and find no entries at all. Switching to ZIP64 + * above 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 longest entry name, in UTF-8 bytes, we will write. The filename length field is 2 bytes * and unsigned, so the format allows {@code 0xFFFF}. We stop at {@link Short#MAX_VALUE} for - * the same reason as {@link #MAX_NON_ZIP64_ENTRY_COUNT}: versions of this SDK that predate - * the unsigned reads in {@link ZipReader} read the length as signed, and a negative length - * fails there. + * the same reason as {@link #MAX_NON_ZIP64_ENTRY_COUNT}: SDK releases before 0.19.0 read the + * length as signed, and a negative length fails there. */ static final int MAX_FILENAME_LENGTH = Short.MAX_VALUE; @@ -109,6 +106,8 @@ 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. + * + * @throws SDKException if the name is longer than {@link #MAX_FILENAME_LENGTH} bytes */ private static byte[] encodeFilename(String name) { var bytes = name.getBytes(StandardCharsets.UTF_8); @@ -287,12 +286,11 @@ 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 + // if any of these doesn't fit, the archive goes zip64 and every end of central directory + // field gets the sentinel, even the ones that fit. SDK readers before 0.19.0 only check + // the offset for the sentinel, so it has to be there for them to find the zip64 record. + // the count limit is fixed; the offset and size use the configured threshold, which is + // MAX_NON_ZIP64_VALUE outside of tests var isZip64 = hasZip64Entry || numEntries > MAX_NON_ZIP64_ENTRY_COUNT || needsZip64(startOfCentralDirectory, sizeOfCentralDirectory); @@ -390,7 +388,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() + // encodeFilename bounds this to Short.MAX_VALUE, so the cast cannot truncate buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.put(filename); @@ -450,7 +448,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() + // encodeFilename bounds this to Short.MAX_VALUE, so the cast cannot truncate 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 6c9cebba..b86d36e7 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -270,8 +270,8 @@ private static void assertReadsEveryEntry(SeekableByteChannel channel) throws IO /** * 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. + * than fail somewhere downstream. Whether the record is gone, clipped, or the file is shorter + * than one, the scan finds no acceptable signature within its window. */ @Test public void testTruncatedArchiveIsRejected() throws IOException { @@ -352,8 +352,8 @@ public void testZip64ArchiveWithTrailingDataStillReads() throws IOException { } /** - * 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 + * The scan only looks back as far as the longest possible comment, 65,535 bytes; trailing data + * is tolerated only within that same window. Beyond it 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. */ @@ -432,7 +432,7 @@ public void testSignatureInsideTheCommentIsNotMistakenForTheRecord() throws IOEx Arrays.fill(comment, (byte) 0xFF); // so the decoy's own comment length reads as 0xFFFF ByteBuffer.wrap(comment).order(ByteOrder.LITTLE_ENDIAN).putInt(0, EOCD_SIGNATURE); - // commons-compress takes the decoy for the record, so this one is ours alone + // an independent reader may take the decoy for the record, so this one is ours alone try (var channel = new SeekableInMemoryByteChannel(withComment(zip64Archive(), comment))) { assertReadsEveryEntry(channel); } @@ -478,7 +478,7 @@ public void testArchiveReadThroughAChannelThatReturnsEmptyReads() throws IOExcep * spinning on it — whether it stops before the archive is opened or partway through an entry. */ @Test - public void testChannelThatStopsReturningDataFails() throws IOException { + public void testChannelThatStopsReturningDataFails() { assertTimeoutPreemptively(Duration.ofSeconds(10), () -> { try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { channel.stalled = true; @@ -487,10 +487,19 @@ public void testChannelThatStopsReturningDataFails() throws IOException { .hasMessageContaining("no data"); } - try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0)) { - var reader = new ZipReader(channel); + // getData() reads the local header, so the stall has to start after it returns to + // reach the stream itself: once for a block read, once for a single byte + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0); + var data = new ZipReader(channel).getEntries().get(0).getData()) { + channel.stalled = true; + assertThatThrownBy(() -> data.read(new byte[4])) + .isInstanceOf(IOException.class) + .hasMessageContaining("no data"); + } + try (var channel = new StallingChannel(new SeekableInMemoryByteChannel(zip64Archive()), 0); + var data = new ZipReader(channel).getEntries().get(0).getData()) { channel.stalled = true; - assertThatThrownBy(() -> readEntry(reader, "0.payload")) + assertThatThrownBy(data::read) .isInstanceOf(IOException.class) .hasMessageContaining("no data"); } @@ -577,17 +586,91 @@ public void testEntryRunningPastTheEndOfTheArchiveFailsToRead() throws IOExcepti var reader = new ZipReader(channel); assertThatThrownBy(() -> readEntry(reader, "small.txt")) .isInstanceOf(InvalidZipException.class) - .hasMessageContaining("bytes into the 1000000 byte entry [small.txt]"); + .hasMessageContaining("[small.txt]") + .hasMessageContaining("1000000"); // and a byte at a time, which takes a different path through the stream - assertThatThrownBy(() -> { - try (var data = reader.getEntries().get(0).getData()) { - while (data.read() != -1) { - // drain - } - } - }).isInstanceOf(InvalidZipException.class) - .hasMessageContaining("byte entry [small.txt]"); + var entry = reader.getEntries().get(0); + assertThatThrownBy(() -> drainByteAtATime(entry)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("[small.txt]"); + } + } + + /** + * {@link java.io.InputStream} requires a zero-length read to return 0, even at the end of the + * stream. Passed through to the channel, it would read nothing every time and trip the + * stalled-channel check on a valid archive. + */ + @Test + public void testZeroLengthReadReturnsZero() throws IOException { + try (var channel = new SeekableInMemoryByteChannel(plainArchive())) { + var entry = new ZipReader(channel).getEntries().get(0); + try (var data = entry.getData()) { + var buf = new byte[4]; + assertThat(data.read(buf, 0, 0)).isZero(); + assertThat(data.read()).isEqualTo('t'); + assertThat(data.read(buf, 0, 0)).isZero(); + assertThat(data.read(buf, 0, 4)).isEqualTo(3); + // the entry is used up, but a zero-length read is still 0 rather than -1 + assertThat(data.read(buf, 0, 0)).isZero(); + assertThat(data.read(buf, 0, 4)).isEqualTo(-1); + } + } + } + + /** + * An end of central directory record that claims zip64 when nothing in front of it is a + * locator. The locator is looked for immediately before the record, and what is there instead + * has to be reported as a bad signature at that offset. + */ + @Test + public void testMissingZip64LocatorIsRejected() throws IOException { + var archive = plainArchive(); + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN) + .putInt(archive.length - EOCD_SIZE + 16, 0xFFFFFFFF); + int locatorStart = archive.length - EOCD_SIZE - ZIP64_EOCD_LOCATOR_SIZE; + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("locator") + .hasMessageContaining("offset " + locatorStart); + } + + /** + * A locator pointer that stays inside the archive but lands on something other than the zip64 + * end of central directory record. The bounds check passes, so the signature check is all + * that stands between the reader and a misparse. + */ + @Test + public void testZip64LocatorPointingAtTheWrongRecordIsRejected() throws IOException { + var archive = zip64Archive(); + int pointer = archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE) + 8; + // offset 0 holds the first local file header + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putLong(pointer, 0L); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("0x" + Integer.toHexString(0x04034b50)); + } + + /** + * A channel whose size claims more bytes than it holds. Every offset the scan probes should + * have a whole record after it, so running out of data there is a zip error, not a miss. + */ + @Test + public void testChannelSmallerThanItsReportedSizeIsRejected() throws IOException { + var archive = plainArchive(); + long claimedSize = archive.length + 30L; + try (var channel = new DelegatingChannel(new SeekableInMemoryByteChannel(archive)) { + @Override + public long size() { + return claimedSize; + } + }) { + assertThatThrownBy(() -> new ZipReader(channel)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining(Long.toString(claimedSize)); } } @@ -716,6 +799,14 @@ private static String readEntry(ZipReader reader, String name) throws IOExceptio return data.toString(StandardCharsets.UTF_8); } + private static void drainByteAtATime(ZipReader.Entry entry) throws IOException { + try (var data = entry.getData()) { + while (data.read() != -1) { + // drain + } + } + } + private static String readEntry(ZipFile zip, String name) throws IOException { var data = new ByteArrayOutputStream(); zip.getInputStream(zip.getEntry(name)).transferTo(data); 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 aa6c3150..98085b1f 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java @@ -21,6 +21,7 @@ import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; import java.nio.file.StandardOpenOption; +import java.util.Collections; import java.util.Locale; import java.util.Random; import java.util.zip.CRC32; @@ -193,6 +194,11 @@ public void entryCountAloneDrivesTheEndOfCentralDirectorySentinel() throws IOExc try (var chan = new SeekableInMemoryByteChannel(overflows)) { assertThat(new ZipReader(chan).getEntries()).hasSize(Short.MAX_VALUE + 1); } + // an independent reader finds the zip64 records well formed too + try (var chan = new SeekableInMemoryByteChannel(overflows)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(Collections.list(z.getEntries())).hasSize(Short.MAX_VALUE + 1); + } } /** @@ -219,9 +225,9 @@ public void centralDirectoryOffsetAloneDrivesTheEndOfCentralDirectorySentinel() } /** - * 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. + * The central directory size is also a 4-byte field. An empty entry costs 46 bytes plus its + * name in the directory but only 30 plus its name 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 { @@ -286,6 +292,37 @@ public void filenameLengthIsMeasuredInUtf8Bytes() throws IOException { } } + /** + * The same for a streamed entry, which is how a TDF writes its payload. Its central directory + * header is built when the archive is finished, and carries a zip64 extra field after the + * name. + */ + @Test + public void streamedEntryNameIsMeasuredInUtf8Bytes() throws IOException { + var name = "🔒両.payload"; + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + try (var entry = writer.stream(name)) { + entry.write("contents".getBytes(StandardCharsets.UTF_8)); + } + writer.data("after.txt", "more".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(chan); + assertThat(readEntry(reader, name)).isEqualTo("contents"); + assertThat(readEntry(reader, "after.txt")).isEqualTo("more"); + } + 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"); + assertThat(getDataStream(z, z.getEntry("after.txt")).toString(StandardCharsets.UTF_8)) + .isEqualTo("more"); + } + } + /** * The longest name we write is {@link ZipWriter#MAX_FILENAME_LENGTH} UTF-8 bytes. One byte * more is rejected on both the byte array and the streaming path — the second is how a TDF