From 8934cc4fb18b984eecae59f602259278baa866de Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 9 Oct 2026 14:38:42 -0400 Subject: [PATCH 01/10] fix(sdk): harden ZipReader central directory parsing against hostile fields (DSPX-5103) A malformed archive could hang SDK.isTDF / loadTDF or make them throw unchecked exceptions that callers screening untrusted input don't catch. - Walk the central directory extra field record by record: every step advances, a record may not claim more bytes than the extra field has left, and zip64 values must fit inside their record. - Reject zip64 sizes/offsets above Long.MAX_VALUE, extra fields and comments that run past the archive, and entries whose local header offset or compressed size do not fit in it. - Cap the manifest at 10 MiB (Go default, web limit) before reading it. - loadTDF and SDK.readManifest report malformed containers and manifests as SDK.MalformedTDFException; isTDF returns false for any parse failure. - Read a JSON null in an assertion statement's format/schema/value as null instead of throwing or producing the string "null". Adds regression tests with preemptive timeouts and Jazzer seeds for each hostile shape. Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/Manifest.java | 31 +++- .../java/io/opentdf/platform/sdk/SDK.java | 18 +- .../java/io/opentdf/platform/sdk/TDF.java | 9 +- .../io/opentdf/platform/sdk/TDFReader.java | 63 ++++++- .../io/opentdf/platform/sdk/ZipReader.java | 99 +++++++++-- .../io/opentdf/platform/sdk/HostileZips.java | 167 ++++++++++++++++++ .../io/opentdf/platform/sdk/ManifestTest.java | 46 +++++ .../java/io/opentdf/platform/sdk/SDKTest.java | 48 +++++ .../opentdf/platform/sdk/TDFReaderTest.java | 4 +- .../java/io/opentdf/platform/sdk/TDFTest.java | 43 +++++ .../opentdf/platform/sdk/ZipReaderTest.java | 56 ++++++ .../dspx5103-compressed-size-0x7fffffff | Bin 0 -> 237 bytes .../dspx5103-extra-field-data-size-0xfffc | Bin 0 -> 245 bytes ...extra-field-data-size-past-the-extra-field | Bin 0 -> 245 bytes .../dspx5103-extra-field-length-0xffff | Bin 0 -> 237 bytes .../dspx5103-file-comment-length-0xffff | Bin 0 -> 237 bytes .../fuzzTDF/dspx5103-file-name-length-0xffff | Bin 0 -> 237 bytes .../dspx5103-local-header-offset-0xfffffff0 | Bin 0 -> 237 bytes ...x5103-zip64-compressed-size-long-min-value | Bin 0 -> 249 bytes ...64-extra-field-too-short-for-its-sentinels | Bin 0 -> 257 bytes ...p64-local-header-offset-0xffffffffffffffff | Bin 0 -> 249 bytes ...zip64-uncompressed-size-0xffffffffffffffff | Bin 0 -> 249 bytes .../dspx5103-compressed-size-0x7fffffff | Bin 0 -> 237 bytes .../dspx5103-extra-field-data-size-0xfffc | Bin 0 -> 245 bytes ...extra-field-data-size-past-the-extra-field | Bin 0 -> 245 bytes .../dspx5103-extra-field-length-0xffff | Bin 0 -> 237 bytes .../dspx5103-file-comment-length-0xffff | Bin 0 -> 237 bytes .../dspx5103-file-name-length-0xffff | Bin 0 -> 237 bytes .../dspx5103-local-header-offset-0xfffffff0 | Bin 0 -> 237 bytes ...x5103-zip64-compressed-size-long-min-value | Bin 0 -> 249 bytes ...64-extra-field-too-short-for-its-sentinels | Bin 0 -> 257 bytes ...p64-local-header-offset-0xffffffffffffffff | Bin 0 -> 249 bytes ...zip64-uncompressed-size-0xffffffffffffffff | Bin 0 -> 249 bytes 33 files changed, 544 insertions(+), 40 deletions(-) create mode 100644 sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-compressed-size-0x7fffffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-0xfffc create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-past-the-extra-field create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-comment-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-name-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-local-header-offset-0xfffffff0 create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-compressed-size-long-min-value create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-extra-field-too-short-for-its-sentinels create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-local-header-offset-0xffffffffffffffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-uncompressed-size-0xffffffffffffffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-compressed-size-0x7fffffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-0xfffc create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-past-the-extra-field create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-comment-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-name-length-0xffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-local-header-offset-0xfffffff0 create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-compressed-size-long-min-value create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-extra-field-too-short-for-its-sentinels create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-local-header-offset-0xffffffffffffffff create mode 100644 sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-uncompressed-size-0xffffffffffffffff diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java index e751a2120..96e8978e4 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -1141,14 +1141,11 @@ public AssertionConfig.Statement deserialize(JsonElement json, Type typeOfT, Jso } var obj = json.getAsJsonObject(); var statement = new AssertionConfig.Statement(); - if (obj.has("format")) { - statement.format = obj.get("format").getAsString(); - } - if (obj.has("schema")) { - statement.schema = obj.get("schema").getAsString(); - } - if (obj.has("value")) { - var value = obj.get("value"); + statement.format = optionalString(obj, "format"); + statement.schema = optionalString(obj, "schema"); + var value = obj.get("value"); + // an explicit JSON null is the same as no value at all, not the four character string "null" + if (value != null && !value.isJsonNull()) { if (value.isJsonPrimitive()) { // it's already a primitive (hopefully string) so we don't need its escaped value here statement.value = value.getAsString(); @@ -1158,6 +1155,24 @@ public AssertionConfig.Statement deserialize(JsonElement json, Type typeOfT, Jso } return statement; } + + /** + * Reads a member that has to be a JSON primitive if present. A missing member and an + * explicit {@code null} both come back as {@code null}; anything else, such as an object + * or array, is a malformed statement. Calling {@code getAsString} unchecked throws + * {@link UnsupportedOperationException} or {@link IllegalStateException} for these, which + * escapes manifest parsing as something other than a parse error. + */ + private static String optionalString(JsonObject obj, String member) { + var element = obj.get(member); + if (element == null || element.isJsonNull()) { + return null; + } + if (!element.isJsonPrimitive()) { + throw new JsonParseException(String.format("assertion statement %s must be a string", member)); + } + return element.getAsString(); + } } public EncryptionInformation encryptionInformation; diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index f4aa7d630..ab0f00017 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -176,6 +176,14 @@ public static boolean isTDF(SeekableByteChannel channel) { zipReader = new ZipReader(channel); } catch (IOException | InvalidZipException e) { return false; + } catch (RuntimeException e) { + // ZipReader reports every malformation it knows about as InvalidZipException, and its + // tests hold it to that. This is a backstop for the contract of this method, which + // screens untrusted input and answers yes or no: a parse failure we did not foresee, + // or an unchecked exception from the caller's channel, still means "not a TDF" + // rather than an exception from the very check meant to spare callers one. + ZipReader.logger.debug("unexpected exception checking whether a channel holds a TDF", e); + return false; } var entries = zipReader.getEntries(); var names = entries.stream().map(ZipReader.Entry::getName).collect(Collectors.toSet()); @@ -191,14 +199,12 @@ public static boolean isTDF(SeekableByteChannel channel) { * Reads the {@link Manifest} without decrypting the TDF * @param tdfBytes A SeekableByteChannel containing the TDF data * @return The parsed {@link Manifest} object + * @throws MalformedTDFException if the bytes are not a well formed TDF or its manifest is invalid * @throws SDKException if an SDK-specific error occurs * @throws IOException if an I/O error occurs */ public static Manifest readManifest(SeekableByteChannel tdfBytes) throws SDKException, IOException { - TDFReader reader = new TDFReader(tdfBytes); - try (var manifestJson = reader.manifest()) { - return Manifest.readManifest(manifestJson); - } + return TDFReader.open(tdfBytes).readManifest(); } /** @@ -389,6 +395,10 @@ public static class MalformedTDFException extends SDKException { public MalformedTDFException(String errorMessage) { super(errorMessage); } + + public MalformedTDFException(String errorMessage, Exception reason) { + super(errorMessage, reason); + } } /** diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java index 883d885da..201b8ca3f 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -788,12 +788,9 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig, Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) throws SDKException, IOException { - TDFReader tdfReader = new TDFReader(tdf); - // use Manifest.readManifest in order to validate the Manifest input - Manifest manifest; - try (var manifestJson = tdfReader.manifest()) { - manifest = Manifest.readManifest(manifestJson); - } + // a malformed container or manifest is reported as SDK.MalformedTDFException + TDFReader tdfReader = TDFReader.open(tdf); + Manifest manifest = tdfReader.readManifest(); byte[] payloadKey = new byte[GCM_KEY_SIZE]; String unencryptedMetadata = null; diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index 43fadb2a7..9f997679b 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -1,5 +1,7 @@ package io.opentdf.platform.sdk; +import com.google.gson.JsonParseException; + import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; @@ -53,15 +55,69 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); } + /** + * The largest manifest this reader will buffer, matching the default of the Go SDK and the + * limit of the web SDK. The manifest is read whole into memory, and its size comes from the + * archive, so without a limit a crafted container could make the reader allocate as much as + * the file is long. + */ + static final long MAX_MANIFEST_SIZE = 10L * 1024 * 1024; + + /** + * Opens a TDF read from untrusted bytes, reporting a container that is not a well formed + * TDF as a {@link SDK.MalformedTDFException} rather than as whichever unchecked exception + * the zip reader or this class's own checks happened to raise. + */ + static TDFReader open(SeekableByteChannel tdf) throws IOException { + try { + return new TDFReader(tdf); + } catch (InvalidZipException | IllegalArgumentException e) { + throw malformed("tdf is not a valid zip container", e); + } + } + + /** + * Reads and validates the manifest. Malformed JSON, and JSON that does not have the shape of + * a manifest, is reported as a {@link SDK.MalformedTDFException}. + */ + Manifest readManifest() { + try (Reader manifestJson = manifest()) { + return Manifest.readManifest(manifestJson); + } catch (InvalidZipException e) { + // the manifest entry runs past the end of the archive + throw malformed("error retrieving manifest from zip file", e); + } catch (JsonParseException | IllegalArgumentException | IllegalStateException + | UnsupportedOperationException e) { + // Gson reports a JSON value of the wrong type through IllegalStateException and + // UnsupportedOperationException (the JsonElement.getAsX accessors), and + // Manifest.readManifest reports missing required fields as IllegalArgumentException. + // Every one of these here means the manifest itself is malformed. + throw malformed("tdf manifest is invalid", e); + } catch (IOException e) { + throw new SDKException("error reading manifest from zip file", e); + } + } + + private static SDK.MalformedTDFException malformed(String what, RuntimeException e) { + return new SDK.MalformedTDFException(what + ": " + e.getMessage(), e); + } + /** * The manifest entry as a character stream; the caller must close it. Returned * as a stream rather than a String because a manifest with tens of millions of * segments exceeds the maximum size of a Java String. */ Reader manifest() { + long size = manifestEntry.getSize(); + if (size > MAX_MANIFEST_SIZE) { + throw new SDK.MalformedTDFException("tdf manifest is " + size + " bytes, which exceeds the " + + MAX_MANIFEST_SIZE + " byte limit"); + } try { return new BufferedReader( new InputStreamReader(manifestEntry.getData(), StandardCharsets.UTF_8), MANIFEST_BUFFER_SIZE); + } catch (InvalidZipException e) { + throw malformed("error retrieving manifest from zip file", e); } catch (IOException e) { throw new SDKException("error retrieving manifest from zip file", e); } @@ -81,10 +137,7 @@ int readPayloadBytes(byte[] buf) { } PolicyObject readPolicyObject() { - try (Reader manifestRaw = manifest()) { - return Manifest.decodePolicyObject(Manifest.readManifest(manifestRaw)); - } catch (IOException e) { - throw new SDKException("error reading manifest from zip file", e); - } + Manifest manifest = readManifest(); + return Manifest.decodePolicyObject(manifest); } } 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 d2fd6e916..d7297a4b3 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -374,6 +374,15 @@ public String getName() { return fileName; } + /** + * The uncompressed size the central directory records for this entry. It comes from the + * archive and is not checked against the data, so treat it as a claim: it lets a caller + * refuse an entry too large to buffer before reading any of it. + */ + long getSize() { + return fileSize; + } + /** * Checks that this entry's local header offset points inside the archive, so a corrupt * or truncated central directory fails here rather than at an arbitrary position. @@ -478,6 +487,37 @@ public int read(byte[] b, int off, int len) throws IOException { }; } } + + /** Each extra field record starts with a 2-byte id and a 2-byte data size. */ + private static final int EXTRA_FIELD_HEADER_SIZE = 4; + + /** + * Checks that a {@code size} byte value read at the current position stays inside the zip64 + * extra field record ending at {@code fieldEnd}. A record too short for the values its + * sentinels promise would otherwise be read past, into whatever follows it. + */ + private void requireWithinField(String what, int size, long fieldEnd, long headerStart) throws IOException { + if (zipChannel.position() + size > fieldEnd) { + throw new InvalidZipException("The zip64 extra field of the central directory file header at offset " + + headerStart + " is too short to hold the " + what + " its sentinel promises"); + } + } + + /** + * Reads one 8-byte value out of a zip64 extra field record. The format calls these unsigned, + * but nothing can be larger than {@link Long#MAX_VALUE} bytes, so a value that comes out + * negative as a Java {@code long} is a corrupt or hostile archive rather than a big one. + */ + private long readZip64Value(String what, long fieldEnd, long headerStart) throws IOException { + requireWithinField(what, Long.BYTES, fieldEnd, headerStart); + long value = readLong(); + if (value < 0) { + throw new InvalidZipException("The zip64 " + what + " of the central directory file header at offset " + + headerStart + " is " + Long.toUnsignedString(value) + ", which no archive can hold"); + } + return value; + } + public Entry readCentralDirectoryFileHeader() throws IOException { Integer signature = readInteger(); if (signature == null || signature != CENTRAL_FILE_HEADER_SIGNATURE) { @@ -508,41 +548,70 @@ public Entry readCentralDirectoryFileHeader() throws IOException { + (fileNameStart - CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE)); } - // Parse the extra field - for (final long startPos = zipChannel.position(); zipChannel.position() < startPos + extraFieldLength; ) { - long fieldStart = zipChannel.position(); + long headerStart = fileNameStart - CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; + long extraFieldStart = zipChannel.position(); + long extraFieldEnd = extraFieldStart + extraFieldLength; + if (extraFieldEnd > zipChannel.size()) { + throw new InvalidZipException("The " + extraFieldLength + " byte extra field of the central directory" + + " file header at offset " + headerStart + " runs past the end of this " + + zipChannel.size() + " byte archive"); + } + + // walk the extra field as a sequence of (id, size, data) records. each step moves past a + // whole record, header included, so the walk always advances, and no record may claim + // more bytes than are left in the extra field. a sign-extended or oversized data size + // used to send the position backwards or beyond the field, looping forever or reading + // the next header as if it were part of this one + long fieldStart = extraFieldStart; + while (extraFieldEnd - fieldStart >= EXTRA_FIELD_HEADER_SIZE) { + zipChannel.position(fieldStart); int headerId = readUnsignedShort(); int dataSize = readUnsignedShort(); + long dataStart = fieldStart + EXTRA_FIELD_HEADER_SIZE; + long dataEnd = dataStart + dataSize; + if (dataEnd > extraFieldEnd) { + throw new InvalidZipException("Extra field 0x" + Integer.toHexString(headerId) + " of the central" + + " directory file header at offset " + headerStart + " claims " + dataSize + + " bytes, but only " + (extraFieldEnd - dataStart) + " remain in the extra field"); + } if (headerId == ZIP64_EXTID) { // APPNOTE 4.5.3 order: original size, compressed size, then local header offset if (uncompressedSize == ZIP64_MAGICVAL) { - uncompressedSize = readLong(); + uncompressedSize = readZip64Value("uncompressed size", dataEnd, headerStart); } if (compressedSize == ZIP64_MAGICVAL) { - compressedSize = readLong(); + compressedSize = readZip64Value("compressed size", dataEnd, headerStart); } if (relativeOffsetOfLocalHeader == ZIP64_MAGICVAL) { - relativeOffsetOfLocalHeader = readLong(); + relativeOffsetOfLocalHeader = readZip64Value("local header offset", dataEnd, headerStart); } // a 2-byte field, so its sentinel is 0xFFFF rather than 0xFFFFFFFF if (diskNumberStart == ZIP64_MAGIC_SHORT) { + requireWithinField("disk number", Integer.BYTES, dataEnd, headerStart); diskNumberStart = readInt(); } } - // Skip other extra fields - zipChannel.position(fieldStart + dataSize + 4); + fieldStart = dataEnd; } + // fewer bytes than a record header may be left over; like other readers, ignore them - // a zip64 size is unsigned on the wire, but nothing holds more than Long.MAX_VALUE bytes, so - // one that reads as negative is corrupt. left alone, it reads as an empty entry, not an error - if (uncompressedSize < 0) { + long fileCommentEnd = extraFieldEnd + fileCommentLength; + if (fileCommentEnd > zipChannel.size()) { + throw new InvalidZipException("The " + fileCommentLength + " byte comment of the central directory" + + " file header at offset " + headerStart + " runs past the end of this " + + zipChannel.size() + " byte archive"); + } + // an honest archive stores every byte of an entry's data after its local header, so an + // entry that claims to start or end beyond the archive cannot be read. the uncompressed + // size is left to the reader, which reports a short entry as truncated when it gets there + if (relativeOffsetOfLocalHeader >= zipChannel.size() + || compressedSize > zipChannel.size() - relativeOffsetOfLocalHeader) { throw new InvalidZipException("Entry [" + new String(fileName.array(), StandardCharsets.UTF_8) - + "] claims a size of " + Long.toUnsignedString(uncompressedSize) - + " bytes, which no archive can hold"); + + "] claims " + compressedSize + " bytes at local header offset " + relativeOffsetOfLocalHeader + + ", which does not fit in this " + zipChannel.size() + " byte archive"); } - - zipChannel.position(zipChannel.position() + fileCommentLength); + zipChannel.position(fileCommentEnd); return new Entry(fileName.array(), relativeOffsetOfLocalHeader, uncompressedSize); } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java new file mode 100644 index 000000000..0e0c41f05 --- /dev/null +++ b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java @@ -0,0 +1,167 @@ +package io.opentdf.platform.sdk; + +import java.io.ByteArrayOutputStream; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Consumer; + +/** + * Builds small stored (uncompressed) zip archives by hand, so that a test can put any value it + * likes into a central directory field. {@link ZipWriter} and other writers only produce honest + * archives; these are for the hostile shapes from DSPX-5103, where lengths, sizes and offsets + * read as negative or point outside the archive. + */ +final class HostileZips { + private HostileZips() { + } + + /** One archive entry, plus overrides for what its central directory header claims. */ + static final class Entry { + final String name; + final byte[] data; + byte[] centralExtra = new byte[0]; + Long compressedSize; + Long uncompressedSize; + Long localHeaderOffset; + Integer fileNameLength; + Integer extraFieldLength; + Integer fileCommentLength; + + Entry(String name, byte[] data) { + this.name = name; + this.data = data; + } + } + + /** + * A minimal TDF-shaped archive: a payload and a manifest. {@code hostile} adjusts the + * manifest entry, which is written last so its central directory header is the last one. + */ + static byte[] tdfWith(String manifestJson, Consumer hostile) { + var payload = new Entry("0.payload", "payload bytes".getBytes(StandardCharsets.UTF_8)); + var manifest = new Entry("0.manifest.json", manifestJson.getBytes(StandardCharsets.UTF_8)); + hostile.accept(manifest); + return build(List.of(payload, manifest)); + } + + /** The named hostile shapes, each applied to the manifest entry of {@link #tdfWith}. */ + static Map> hostileShapes() { + var shapes = new LinkedHashMap>(); + // the shape that looped forever when the data size was read signed: 0xFFFC is -4, + // which sent the position straight back to the start of the record + shapes.put("extra field data size 0xFFFC", e -> e.centralExtra = extraRecord(0xCAFE, 0xFFFC, new byte[4])); + shapes.put("extra field data size past the extra field", e -> e.centralExtra = extraRecord(0xCAFE, 16, new byte[4])); + shapes.put("zip64 local header offset 0xFFFFFFFFFFFFFFFF", e -> { + e.localHeaderOffset = 0xFFFFFFFFL; + e.centralExtra = extraRecord(0x0001, 8, longs(-1L)); + }); + shapes.put("zip64 uncompressed size 0xFFFFFFFFFFFFFFFF", e -> { + e.uncompressedSize = 0xFFFFFFFFL; + e.centralExtra = extraRecord(0x0001, 8, longs(-1L)); + }); + shapes.put("zip64 compressed size Long.MIN_VALUE", e -> { + e.compressedSize = 0xFFFFFFFFL; + e.centralExtra = extraRecord(0x0001, 8, longs(Long.MIN_VALUE)); + }); + // both sizes carry the sentinel, but the record only has room for one of them; the + // second must not be read out of whatever follows the record + shapes.put("zip64 extra field too short for its sentinels", e -> { + e.uncompressedSize = 0xFFFFFFFFL; + e.compressedSize = 0xFFFFFFFFL; + e.centralExtra = concat(extraRecord(0x0001, 8, longs(e.data.length)), extraRecord(0xCAFE, 4, new byte[4])); + }); + shapes.put("file name length 0xFFFF", e -> e.fileNameLength = 0xFFFF); + shapes.put("extra field length 0xFFFF", e -> e.extraFieldLength = 0xFFFF); + shapes.put("file comment length 0xFFFF", e -> e.fileCommentLength = 0xFFFF); + shapes.put("local header offset 0xFFFFFFF0", e -> e.localHeaderOffset = 0xFFFFFFF0L); + shapes.put("compressed size 0x7FFFFFFF", e -> e.compressedSize = 0x7FFFFFFFL); + return shapes; + } + + static byte[] extraRecord(int headerId, int dataSize, byte[] data) { + var buf = ByteBuffer.allocate(4 + data.length).order(ByteOrder.LITTLE_ENDIAN); + buf.putShort((short) headerId).putShort((short) dataSize).put(data); + return buf.array(); + } + + static byte[] longs(long... values) { + var buf = ByteBuffer.allocate(values.length * Long.BYTES).order(ByteOrder.LITTLE_ENDIAN); + for (long v : values) { + buf.putLong(v); + } + return buf.array(); + } + + static byte[] concat(byte[]... parts) { + var out = new ByteArrayOutputStream(); + for (var p : parts) { + out.writeBytes(p); + } + return out.toByteArray(); + } + + static byte[] build(List entries) { + var out = new ByteArrayOutputStream(); + var offsets = new ArrayList(); + for (var e : entries) { + offsets.add((long) out.size()); + var name = e.name.getBytes(StandardCharsets.UTF_8); + var local = ByteBuffer.allocate(30 + name.length).order(ByteOrder.LITTLE_ENDIAN); + local.putInt(0x04034b50) + .putShort((short) 20) // version needed + .putShort((short) 0) // flags + .putShort((short) 0) // stored + .putShort((short) 0).putShort((short) 0) // time, date + .putInt(0) // crc, unchecked by the reader + .putInt(e.data.length).putInt(e.data.length) + .putShort((short) name.length).putShort((short) 0) + .put(name); + out.writeBytes(local.array()); + out.writeBytes(e.data); + } + + long centralDirectoryStart = out.size(); + for (int i = 0; i < entries.size(); i++) { + var e = entries.get(i); + var name = e.name.getBytes(StandardCharsets.UTF_8); + var central = ByteBuffer.allocate(46 + name.length + e.centralExtra.length).order(ByteOrder.LITTLE_ENDIAN); + central.putInt(0x02014b50) + .putShort((short) 45).putShort((short) 45) // version made by, needed + .putShort((short) 0).putShort((short) 0) // flags, stored + .putShort((short) 0).putShort((short) 0) // time, date + .putInt(0) // crc + .putInt((int) orElse(e.compressedSize, e.data.length)) + .putInt((int) orElse(e.uncompressedSize, e.data.length)) + .putShort((short) orElse(e.fileNameLength, name.length)) + .putShort((short) orElse(e.extraFieldLength, e.centralExtra.length)) + .putShort((short) orElse(e.fileCommentLength, 0)) + .putShort((short) 0) // disk number start + .putShort((short) 0) // internal attributes + .putInt(0) // external attributes + .putInt((int) orElse(e.localHeaderOffset, offsets.get(i))) + .put(name) + .put(e.centralExtra); + out.writeBytes(central.array()); + } + long centralDirectorySize = out.size() - centralDirectoryStart; + + var eocd = ByteBuffer.allocate(22).order(ByteOrder.LITTLE_ENDIAN); + eocd.putInt(0x06054b50) + .putShort((short) 0).putShort((short) 0) + .putShort((short) entries.size()).putShort((short) entries.size()) + .putInt((int) centralDirectorySize) + .putInt((int) centralDirectoryStart) + .putShort((short) 0); + out.writeBytes(eocd.array()); + return out.toByteArray(); + } + + private static long orElse(Number override, long dflt) { + return override == null ? dflt : override.longValue(); + } +} diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java index c49a709df..afba20d14 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java @@ -1,6 +1,9 @@ package io.opentdf.platform.sdk; import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonNull; +import com.google.gson.JsonObject; import com.google.gson.JsonParseException; import com.google.gson.JsonParser; import org.junit.jupiter.api.Test; @@ -476,4 +479,47 @@ static Stream nonAlignedPlacements() { Arguments.of("root", "", ",\"tdf_spec_version\":\"4.3.0\""), Arguments.of("payload", ",\"tdf_spec_version\":\"4.3.0\"", "")); } + + private String manifestWithObjectStatementValue() throws IOException { + try (var mStream = getClass().getResourceAsStream("/io.opentdf.platform.sdk.TestData/manifest-with-object-statement-value.json")) { + assert mStream != null; + return new String(mStream.readAllBytes(), StandardCharsets.UTF_8); + } + } + + /** The first assertion's statement with {@code member} replaced, keeping explicit nulls. */ + private String withStatementMember(String member, com.google.gson.JsonElement replacement) throws IOException { + var root = JsonParser.parseString(manifestWithObjectStatementValue()).getAsJsonObject(); + var statement = root.getAsJsonArray("assertions").get(0).getAsJsonObject().getAsJsonObject("statement"); + statement.add(member, replacement); + return new GsonBuilder().serializeNulls().create().toJson(root); + } + + /** + * DSPX-5103: an explicit JSON null in a statement field is the same as leaving it out. It + * used to throw UnsupportedOperationException for format and schema, which escaped manifest + * parsing, and to become the string "null" for value. + */ + @Test + void testNullStatementFieldsReadAsNull() throws IOException { + for (var member : List.of("format", "schema", "value")) { + var manifest = Manifest.readManifest(withStatementMember(member, JsonNull.INSTANCE)); + var statement = manifest.assertions.get(0).statement; + var actual = member.equals("format") ? statement.format + : member.equals("schema") ? statement.schema : statement.value; + assertThat(actual).as(member).isNull(); + } + } + + /** A statement format or schema that is not a string is a parse error, not an unchecked crash. */ + @Test + void testNonStringStatementFormatIsAParseError() throws IOException { + for (var member : List.of("format", "schema")) { + var json = withStatementMember(member, new JsonObject()); + assertThatThrownBy(() -> Manifest.readManifest(json)) + .as(member) + .isInstanceOf(JsonParseException.class) + .hasMessageContaining(member); + } + } } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index bd099a001..8b3f1842a 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -15,9 +15,12 @@ import java.nio.ByteBuffer; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Random; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.mockito.Mockito.mock; class SDKTest { @@ -108,6 +111,51 @@ void testExaminingZipThatListsAnUnrelatedNameTwice() throws IOException { } } + /** + * DSPX-5103: {@link SDK#isTDF} answers a malformed archive with false. It used to hang on + * one of these shapes and let an unchecked exception escape on others. + */ + @Test + void testExaminingHostileArchivesReturnsFalse() { + HostileZips.hostileShapes().forEach((shape, hostile) -> { + var archive = HostileZips.tdfWith("{}", hostile); + boolean result = assertTimeoutPreemptively(Duration.ofSeconds(5), + () -> SDK.isTDF(new SeekableInMemoryByteChannel(archive)), shape); + assertThat(result).as(shape).isFalse(); + }); + } + + /** The unmodified hand-built archive those shapes start from is a TDF. */ + @Test + void testExaminingHandBuiltTDF() { + assertThat(SDK.isTDF(new SeekableInMemoryByteChannel(HostileZips.tdfWith("{}", e -> { })))).isTrue(); + } + + /** A channel that throws something unchecked is not a TDF either, rather than a crash in the check. */ + @Test + void testExaminingAChannelThatThrowsUncheckedReturnsFalse() { + var channel = new SeekableInMemoryByteChannel(HostileZips.tdfWith("{}", e -> { })) { + @Override + public int read(ByteBuffer buf) { + throw new IllegalStateException("broken channel"); + } + }; + assertThat(SDK.isTDF(channel)).isFalse(); + } + + /** {@link SDK#readManifest} reports the same archives as a malformed TDF. */ + @Test + void testReadingTheManifestOfHostileArchivesThrowsMalformedTDF() { + HostileZips.hostileShapes().forEach((shape, hostile) -> { + var archive = HostileZips.tdfWith("{}", hostile); + assertTimeoutPreemptively(Duration.ofSeconds(5), () -> + assertThatThrownBy(() -> SDK.readManifest(new SeekableInMemoryByteChannel(archive))) + .as(shape) + .isInstanceOf(SDK.MalformedTDFException.class), + shape); + }); + } + /** Builds a zip holding the named entries; contents are irrelevant to {@link SDK#isTDF}. */ private static SeekableInMemoryByteChannel zipOf(String... names) throws IOException { var out = new ByteArrayOutputStream(); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 43f2a02dc..2d54f3326 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -102,8 +102,8 @@ void rejectsEmptyManifest(String manifest) throws IOException { var readerConfig = Config.newTDFReaderConfig(); assertThatThrownBy(() -> tdf.loadTDF(tdfBytes, readerConfig)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Manifest is empty"); + .isInstanceOf(SDK.MalformedTDFException.class) + .hasMessage("tdf manifest is invalid: Manifest is empty"); } } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index aa5b38e6b..13cdae30e 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -26,6 +26,7 @@ import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.security.KeyPair; +import java.time.Duration; import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Arrays; @@ -45,6 +46,7 @@ import static io.opentdf.platform.sdk.TDF.GLOBAL_KEY_SALT; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -1502,6 +1504,47 @@ private static Config.KASInfo[] getSingleRSAKASInfo() { return getKASInfos(i -> i == 0); } + /** + * DSPX-5103: a malformed container reaches the caller of {@code loadTDF} as a + * {@link SDK.MalformedTDFException}, never as a hang or an IllegalArgumentException, + * UnsupportedOperationException or other unchecked exception from inside the parser. + */ + @Test + void loadTDFReportsHostileArchivesAsMalformed() { + HostileZips.hostileShapes().forEach((shape, hostile) -> + assertLoadTDFFailsAsMalformed(shape, HostileZips.tdfWith("{}", hostile))); + } + + /** The manifest is buffered whole, so one that claims to be larger than the limit is refused unread. */ + @Test + void loadTDFRefusesAnOversizedManifest() { + var archive = HostileZips.tdfWith("{}", e -> e.uncompressedSize = TDFReader.MAX_MANIFEST_SIZE + 1); + assertThatThrownBy(() -> loadTDFFrom(archive)) + .isInstanceOf(SDK.MalformedTDFException.class) + .hasMessageContaining("exceeds the " + TDFReader.MAX_MANIFEST_SIZE + " byte limit"); + } + + @Test + void loadTDFReportsAnInvalidManifestAsMalformed() { + var assertionWithObjectFormat = "{\"payload\":{},\"assertions\":[{\"statement\":{\"format\":{}}}]}"; + for (var manifest : List.of("not json", "[]", "{\"payload\": 5}", "{}", assertionWithObjectFormat)) { + assertLoadTDFFailsAsMalformed(manifest, HostileZips.tdfWith(manifest, e -> { })); + } + } + + private static void assertLoadTDFFailsAsMalformed(String description, byte[] archive) { + assertTimeoutPreemptively(Duration.ofSeconds(5), () -> + assertThatThrownBy(() -> loadTDFFrom(archive)) + .as(description) + .isInstanceOf(SDK.MalformedTDFException.class), + description); + } + + private static void loadTDFFrom(byte[] archive) throws IOException { + new TDF(new FakeServicesBuilder().setKas(kas).build()) + .loadTDF(new SeekableInMemoryByteChannel(archive), Config.newTDFReaderConfig()); + } + private static boolean isHexChar(byte b) { return (b >= 'a' && b <= 'f') || (b >= '0' && b <= '9'); } 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 13289d500..caf4c898c 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -761,6 +761,62 @@ public long size() { } } + /** + * DSPX-5103: central directory fields that read as negative, or that point outside the + * archive or outside their own extra field, are zip errors. None of them may hang the reader + * (a data size of 0xFFFC once looped forever) or escape as an unchecked exception other than + * {@link InvalidZipException}. + */ + @Test + public void testHostileCentralDirectoryFieldsAreRejected() { + HostileZips.hostileShapes().forEach((shape, hostile) -> { + var archive = HostileZips.tdfWith(MANIFEST, hostile); + assertTimeoutPreemptively(Duration.ofSeconds(5), () -> + assertThatThrownBy(() -> { + try (var channel = new SeekableInMemoryByteChannel(archive)) { + // reading every entry, not just the directory, so a shape that only + // fails once its data is read is caught too + testReadingZipChannel(channel, false); + } + }).as(shape).isInstanceOf(InvalidZipException.class), + shape); + }); + } + + /** The hand-built archives the hostile shapes start from are valid until a shape is applied. */ + @Test + public void testHandBuiltArchiveReads() throws IOException { + var archive = HostileZips.tdfWith(MANIFEST, e -> { }); + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(channel); + assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + assertThat(readEntry(reader, "0.payload")).isEqualTo("payload bytes"); + } + } + + /** + * A zip64 record whose values are all within range is still honoured, after an unrelated + * record and followed by a few bytes too short to be a record, which readers ignore. + */ + @Test + public void testZip64ExtraFieldAmongOtherRecordsStillReads() throws IOException { + var archive = HostileZips.tdfWith(MANIFEST, e -> { + e.uncompressedSize = 0xFFFFFFFFL; + e.compressedSize = 0xFFFFFFFFL; + e.localHeaderOffset = 0xFFFFFFFFL; + long offset = "0.payload".length() + 30 + "payload bytes".length(); + e.centralExtra = HostileZips.concat( + HostileZips.extraRecord(0xCAFE, 2, new byte[2]), + HostileZips.extraRecord(0x0001, 24, HostileZips.longs(e.data.length, e.data.length, offset)), + new byte[3]); + }); + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var reader = new ZipReader(channel); + assertThat(readEntry(reader, "0.manifest.json")).isEqualTo(MANIFEST); + assertThat(readEntry(reader, "0.payload")).isEqualTo("payload bytes"); + } + } + /** The same three entries as {@link #zip64Archive()}, written without anything zip64. */ private static byte[] plainArchive() throws IOException { var out = new ByteArrayOutputStream(); diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-compressed-size-0x7fffffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-compressed-size-0x7fffffff new file mode 100644 index 0000000000000000000000000000000000000000..884c36025041fcc609056e7d51b8ffc2cff94f5c GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;DA7|NsB>FnbW%Ou*VuYz^>cWdkW-0>Tm? JoebhI001<{AnO1C literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-0xfffc b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-0xfffc new file mode 100644 index 0000000000000000000000000000000000000000..c3160aec4302526798eead9b0b8ba671b1eb980e GIT binary patch literal 245 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGjqAE)%de6kGqD`tu*8BEXxK4MZ~m LVGWQ@25}ews}3PV literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-past-the-extra-field b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-data-size-past-the-extra-field new file mode 100644 index 0000000000000000000000000000000000000000..419f5c3f2de4b30aecddd387d751d6f00463b2e4 GIT binary patch literal 245 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGjqAE)%de6kGqD5&$a*@MdKLF_?g` J21qA^MFDnMA6WnZ literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-extra-field-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..aca32c9086cc4401e26009777cd6b43c1d045520 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVbA~n2yG@{Z78+|c(byB6fgl{36M?( GaTox`!yj(| literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-comment-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-comment-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..67aee97f0738f648bf0da762b187e65b8e717e47 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7c*`ELZVH!=q+E8o_@MdKLNizXq36M?( GaTox?#~*J1 literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-name-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-file-name-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..39089bdf9807064b8eef2de8a645aefe67fe91d0 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cX3zis$eK*R+E8o_@MdKLDPaP_5+I!n H;xGUJ$A%wZ literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-local-header-offset-0xfffffff0 b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-local-header-offset-0xfffffff0 new file mode 100644 index 0000000000000000000000000000000000000000..dfdcf67c3dacb8a9aa65a9246ad2a196eb3d0516 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7c*&c+p5C8xFH_$_|HNcyd4Wxhx2upx; HGFUYL@Y5kG literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-compressed-size-long-min-value b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-compressed-size-long-min-value new file mode 100644 index 0000000000000000000000000000000000000000..21f049c3269877f25f43d8759dcdc0af51e00c7e GIT binary patch literal 249 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;DA7KM+9d;X%@60@j9NDpF literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-local-header-offset-0xffffffffffffffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-local-header-offset-0xffffffffffffffff new file mode 100644 index 0000000000000000000000000000000000000000..985640f5449ae50632c1b8460b33336a8028f768 GIT binary patch literal 249 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGj?IuKz$_pod~BBLfEmNEieHyjj^m Os+oYW0Z1o0}Ux0RWlDDTDw3 literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-compressed-size-0x7fffffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-compressed-size-0x7fffffff new file mode 100644 index 0000000000000000000000000000000000000000..884c36025041fcc609056e7d51b8ffc2cff94f5c GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;DA7|NsB>FnbW%Ou*VuYz^>cWdkW-0>Tm? JoebhI001<{AnO1C literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-0xfffc b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-0xfffc new file mode 100644 index 0000000000000000000000000000000000000000..c3160aec4302526798eead9b0b8ba671b1eb980e GIT binary patch literal 245 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGjqAE)%de6kGqD`tu*8BEXxK4MZ~m LVGWQ@25}ews}3PV literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-past-the-extra-field b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-data-size-past-the-extra-field new file mode 100644 index 0000000000000000000000000000000000000000..419f5c3f2de4b30aecddd387d751d6f00463b2e4 GIT binary patch literal 245 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGjqAE)%de6kGqD5&$a*@MdKLF_?g` J21qA^MFDnMA6WnZ literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-extra-field-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..aca32c9086cc4401e26009777cd6b43c1d045520 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVbA~n2yG@{Z78+|c(byB6fgl{36M?( GaTox`!yj(| literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-comment-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-comment-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..67aee97f0738f648bf0da762b187e65b8e717e47 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7c*`ELZVH!=q+E8o_@MdKLNizXq36M?( GaTox?#~*J1 literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-name-length-0xffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-file-name-length-0xffff new file mode 100644 index 0000000000000000000000000000000000000000..39089bdf9807064b8eef2de8a645aefe67fe91d0 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cX3zis$eK*R+E8o_@MdKLDPaP_5+I!n H;xGUJ$A%wZ literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-local-header-offset-0xfffffff0 b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-local-header-offset-0xfffffff0 new file mode 100644 index 0000000000000000000000000000000000000000..dfdcf67c3dacb8a9aa65a9246ad2a196eb3d0516 GIT binary patch literal 237 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7c*&c+p5C8xFH_$_|HNcyd4Wxhx2upx; HGFUYL@Y5kG literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-compressed-size-long-min-value b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-compressed-size-long-min-value new file mode 100644 index 0000000000000000000000000000000000000000..21f049c3269877f25f43d8759dcdc0af51e00c7e GIT binary patch literal 249 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;DA7KM+9d;X%@60@j9NDpF literal 0 HcmV?d00001 diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-local-header-offset-0xffffffffffffffff b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzZipRead/dspx5103-zip64-local-header-offset-0xffffffffffffffff new file mode 100644 index 0000000000000000000000000000000000000000..985640f5449ae50632c1b8460b33336a8028f768 GIT binary patch literal 249 zcmWIWW@Zs#KnA=(4hVAsv4LJeVr5Q#VhWT}NUAJJEe^oY!vs_d!u()8xruq1X{p5} zdRfK!dDXQ6-i%DT47xxm1VFX~Aqrx_t;D7cVGj?IuKz$_pod~BBLfEmNEieHyjj^m Os+oYW0Z1o0}Ux0RWlDDTDw3 literal 0 HcmV?d00001 From eeefd36fd9aced882dac1749928c1238282958af Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 9 Oct 2026 15:24:21 -0400 Subject: [PATCH 02/10] fix(sdk): drop the fixed manifest size cap (DSPX-5103) DSPX-4502 targets 50 TiB payloads whose manifests exceed 1 GiB, so any fixed manifest size limit breaks legitimate TDFs. A hostile declared size is already contained by the archive bounds checks: an entry whose stored bytes do not fit in the archive is rejected, and a manifest that claims more than the archive holds fails as truncated, so the reader never buffers more than the archive contains. Memory scaling for huge manifests is DSPX-4584. Replace the oversized-manifest test with one for a manifest running past the archive and one reading an 11 MiB manifest. Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/TDFReader.java | 17 ++------ .../io/opentdf/platform/sdk/ZipReader.java | 9 ----- .../java/io/opentdf/platform/sdk/TDFTest.java | 40 ++++++++++++++++--- 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index 9f997679b..f7f80ea7d 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -55,14 +55,6 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); } - /** - * The largest manifest this reader will buffer, matching the default of the Go SDK and the - * limit of the web SDK. The manifest is read whole into memory, and its size comes from the - * archive, so without a limit a crafted container could make the reader allocate as much as - * the file is long. - */ - static final long MAX_MANIFEST_SIZE = 10L * 1024 * 1024; - /** * Opens a TDF read from untrusted bytes, reporting a container that is not a well formed * TDF as a {@link SDK.MalformedTDFException} rather than as whichever unchecked exception @@ -108,11 +100,10 @@ private static SDK.MalformedTDFException malformed(String what, RuntimeException * segments exceeds the maximum size of a Java String. */ Reader manifest() { - long size = manifestEntry.getSize(); - if (size > MAX_MANIFEST_SIZE) { - throw new SDK.MalformedTDFException("tdf manifest is " + size + " bytes, which exceeds the " - + MAX_MANIFEST_SIZE + " byte limit"); - } + // deliberately no fixed size cap: very large payloads (DSPX-4502) have manifests well + // over a gigabyte. a hostile declared size cannot make this allocate more than the + // archive holds, because the stream stops at the end of the archive and reports the + // entry as truncated, and ZipReader rejects entries whose stored bytes do not fit try { return new BufferedReader( new InputStreamReader(manifestEntry.getData(), StandardCharsets.UTF_8), MANIFEST_BUFFER_SIZE); 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 d7297a4b3..bbc0fe86e 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -374,15 +374,6 @@ public String getName() { return fileName; } - /** - * The uncompressed size the central directory records for this entry. It comes from the - * archive and is not checked against the data, so treat it as a claim: it lets a caller - * refuse an entry too large to buffer before reading any of it. - */ - long getSize() { - return fileSize; - } - /** * Checks that this entry's local header offset points inside the archive, so a corrupt * or truncated central directory fails here rather than at an arbitrary position. diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index 13cdae30e..7e9598111 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -24,6 +24,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.io.StringWriter; import java.nio.charset.StandardCharsets; import java.security.KeyPair; import java.time.Duration; @@ -1515,13 +1516,42 @@ void loadTDFReportsHostileArchivesAsMalformed() { assertLoadTDFFailsAsMalformed(shape, HostileZips.tdfWith("{}", hostile))); } - /** The manifest is buffered whole, so one that claims to be larger than the limit is refused unread. */ + /** + * A manifest that claims more bytes than the archive holds is truncated, and is reported as + * malformed rather than read short or buffered without bound. This, not a size cap, is what + * stops a hostile declared size. + */ @Test - void loadTDFRefusesAnOversizedManifest() { - var archive = HostileZips.tdfWith("{}", e -> e.uncompressedSize = TDFReader.MAX_MANIFEST_SIZE + 1); - assertThatThrownBy(() -> loadTDFFrom(archive)) + void loadTDFReportsAManifestRunningPastTheArchiveAsMalformed() { + // the manifest opens a JSON comment, so the parser reads on to the end of the archive + // looking for its close, rather than failing at the first byte past the entry + var uncompressedPastTheEnd = HostileZips.tdfWith("/*", e -> e.uncompressedSize = 0x7FFFFFFFL); + assertThatThrownBy(() -> loadTDFFrom(uncompressedPastTheEnd)) .isInstanceOf(SDK.MalformedTDFException.class) - .hasMessageContaining("exceeds the " + TDFReader.MAX_MANIFEST_SIZE + " byte limit"); + .hasCauseInstanceOf(InvalidZipException.class) + .hasMessageContaining("Archive ended"); + + var storedPastTheEnd = HostileZips.tdfWith("{}", e -> e.compressedSize = 0x7FFFFFFFL); + assertThatThrownBy(() -> loadTDFFrom(storedPastTheEnd)) + .isInstanceOf(SDK.MalformedTDFException.class) + .hasCauseInstanceOf(InvalidZipException.class); + } + + /** + * There is deliberately no fixed manifest size cap: very large payloads (DSPX-4502) have + * manifests far bigger than any fixed limit would allow. A manifest larger than the 10 MiB + * cap this reader briefly had is read whole. + */ + @Test + void readsAManifestLargerThanTenMebibytes() throws IOException { + var padding = "a".repeat(11 * 1024 * 1024); + var manifest = "{\"padding\":\"" + padding + "\"}"; + var archive = HostileZips.tdfWith(manifest, e -> { }); + var read = new StringWriter(); + try (var reader = TDFReader.open(new SeekableInMemoryByteChannel(archive)).manifest()) { + reader.transferTo(read); + } + assertThat(read.toString()).isEqualTo(manifest); } @Test From 29a0e00d70a92cbed67632cf2c9a98b90e492280 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 14 Oct 2026 09:38:50 -0400 Subject: [PATCH 03/10] fix(sdk): report a truncated payload as MalformedTDFException (DSPX-5103) A payload whose declared size runs past the end of the archive is only found when it is read, and readPayloadBytes let the resulting InvalidZipException escape unwrapped. Wrap it as MalformedTDFException, as manifest() already does. --- .../io/opentdf/platform/sdk/TDFReader.java | 3 +++ .../opentdf/platform/sdk/TDFReaderTest.java | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index f7f80ea7d..ab7106fb4 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -121,6 +121,9 @@ int readPayloadBytes(byte[] buf) { while (totalRead < buf.length && (nread = payload.read(buf, totalRead, buf.length - totalRead)) >= 0) { totalRead += nread; } + } catch (InvalidZipException e) { + // the payload entry runs past the end of the archive + throw malformed("error reading from payload in TDF", e); } catch (IOException e) { throw new SDKException("error reading from payload in TDF", e); } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 2d54f3326..6993fe1e4 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -10,6 +10,7 @@ import java.io.Reader; import java.io.StringWriter; import java.nio.charset.StandardCharsets; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -191,4 +192,22 @@ void readsThePayloadAlongsideASpecNamedManifest() throws IOException { assertThat(buf).startsWith(expected); } } + + /** + * A payload whose declared size runs past the end of the archive is only detected when it is + * read. Like a truncated manifest, it is a malformed TDF, not a raw zip reader exception. + */ + @Test + void reportsATruncatedPayloadAsMalformed() throws IOException { + var payload = new HostileZips.Entry(TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD.getBytes(StandardCharsets.UTF_8)); + payload.uncompressedSize = 1_000_000L; + var manifest = new HostileZips.Entry("0.manifest.json", MANIFEST.getBytes(StandardCharsets.UTF_8)); + try (var tdf = new SeekableInMemoryByteChannel(HostileZips.build(List.of(payload, manifest)))) { + var reader = new TDFReader(tdf); + + assertThatThrownBy(() -> reader.readPayloadBytes(new byte[1_000_000])) + .isInstanceOf(SDK.MalformedTDFException.class) + .hasCauseInstanceOf(InvalidZipException.class); + } + } } From ab83f6342dbbe9295b7c1e943a6d5daaeecae914 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 14 Oct 2026 09:49:48 -0400 Subject: [PATCH 04/10] fix(sdk): bound central directory headers by the declared directory size (DSPX-5103) The reader checked each central directory header only against the end of the archive. A header with an inflated filename length could read the end of central directory record, and any trailing bytes, into the entry name. Keep the declared central directory size from both the plain and the zip64 record. Reject a directory that runs into the record after it, bound the entry count by the declared size, and hold each header's fixed part, filename, extra field and comment to the end of the directory. Every SDK release records this size exactly, so archives it wrote still read. Reported by CodeRabbit on #398. Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/ZipReader.java | 64 +++++++++++++------ .../opentdf/platform/sdk/ZipReaderTest.java | 39 +++++++++++ 2 files changed, 82 insertions(+), 21 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 bbc0fe86e..61fb4c30c 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -120,10 +120,13 @@ private int readUnsignedShort() throws IOException { private static class CentralDirectoryRecord { final long numEntries; final long offsetToStart; + /** Never negative, and ends exactly where the record that follows the directory starts. */ + final long size; - public CentralDirectoryRecord(long numEntries, long offsetToStart) { + public CentralDirectoryRecord(long numEntries, long offsetToStart, long size) { this.numEntries = numEntries; this.offsetToStart = offsetToStart; + this.size = size; } } @@ -194,7 +197,7 @@ private CentralDirectoryRecord centralDirectoryEndingAt(String recordName, long + " describes a central directory of " + Long.toUnsignedString(size) + " bytes at offset " + offset + ", which does not end where that record starts"); } - return new CentralDirectoryRecord(numEntries, offset); + return new CentralDirectoryRecord(numEntries, offset, size); } /** @@ -510,6 +513,28 @@ private long readZip64Value(String what, long fieldEnd, long headerStart) throws } public Entry readCentralDirectoryFileHeader() throws IOException { + return readCentralDirectoryFileHeader(zipChannel.size()); + } + + /** + * Throws if a part of the central directory file header at {@code headerStart} ends after + * {@code centralDirectoryEnd}. Checking against the end of the directory rather than the end + * of the archive keeps an inflated length from reading the records that follow the directory + * as part of an entry. + */ + private static void requireWithinCentralDirectory(String what, long end, long centralDirectoryEnd, + long headerStart) { + if (end > centralDirectoryEnd) { + throw new InvalidZipException("The " + what + " of the central directory file header at offset " + + headerStart + " ends at offset " + end + ", past the end of the central directory at offset " + + centralDirectoryEnd); + } + } + + private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IOException { + long headerStart = zipChannel.position(); + requireWithinCentralDirectory("fixed part", headerStart + CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE, + centralDirectoryEnd, headerStart); Integer signature = readInteger(); if (signature == null || signature != CENTRAL_FILE_HEADER_SIGNATURE) { throw new InvalidZipException("Invalid Central Directory File Header Signature"); @@ -532,21 +557,18 @@ public Entry readCentralDirectoryFileHeader() throws IOException { long relativeOffsetOfLocalHeader = readUnsignedInt(); long fileNameStart = zipChannel.position(); + requireWithinCentralDirectory(fileNameLength + " byte filename", fileNameStart + fileNameLength, + centralDirectoryEnd, headerStart); ByteBuffer fileName = ByteBuffer.allocate(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)); + + " byte filename of the central directory file header at offset " + headerStart); } - long headerStart = fileNameStart - CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; long extraFieldStart = zipChannel.position(); long extraFieldEnd = extraFieldStart + extraFieldLength; - if (extraFieldEnd > zipChannel.size()) { - throw new InvalidZipException("The " + extraFieldLength + " byte extra field of the central directory" - + " file header at offset " + headerStart + " runs past the end of this " - + zipChannel.size() + " byte archive"); - } + requireWithinCentralDirectory(extraFieldLength + " byte extra field", extraFieldEnd, + centralDirectoryEnd, headerStart); // walk the extra field as a sequence of (id, size, data) records. each step moves past a // whole record, header included, so the walk always advances, and no record may claim @@ -588,11 +610,8 @@ public Entry readCentralDirectoryFileHeader() throws IOException { // fewer bytes than a record header may be left over; like other readers, ignore them long fileCommentEnd = extraFieldEnd + fileCommentLength; - if (fileCommentEnd > zipChannel.size()) { - throw new InvalidZipException("The " + fileCommentLength + " byte comment of the central directory" - + " file header at offset " + headerStart + " runs past the end of this " - + zipChannel.size() + " byte archive"); - } + requireWithinCentralDirectory(fileCommentLength + " byte comment", fileCommentEnd, + centralDirectoryEnd, headerStart); // an honest archive stores every byte of an entry's data after its local header, so an // entry that claims to start or end beyond the archive cannot be read. the uncompressed // size is left to the reader, which reports a short entry as truncated when it gets there @@ -611,17 +630,20 @@ public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); + // readEndOfCentralDirectory only accepts a directory that ends where the record after it + // starts. every header is then held to the declared size, so the records that follow the + // directory are never read as directory data + long centralDirectoryEnd = centralDirectoryRecord.offsetToStart + centralDirectoryRecord.size; // 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; + // than the directory can hold, which would read past its end + long mostEntriesThatFit = centralDirectoryRecord.size / 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); + + " entries, but its " + centralDirectoryRecord.size + " bytes can hold at most " + + mostEntriesThatFit); } for (long i = 0; i < centralDirectoryRecord.numEntries; i++) { - entries.add(readCentralDirectoryFileHeader()); + entries.add(readCentralDirectoryFileHeader(centralDirectoryEnd)); } } 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 caf4c898c..a6b7246eb 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -783,6 +783,45 @@ public void testHostileCentralDirectoryFieldsAreRejected() { }); } + /** + * The last central directory header claims a filename longer than the one it holds, so the + * name runs into the end of central directory record. Checked only against the end of the + * archive, that record's bytes would come back as part of the entry name. + */ + @Test + public void testFilenameRunningPastTheCentralDirectoryIsRejected() { + var archive = HostileZips.tdfWith(MANIFEST, e -> e.fileNameLength = e.name.length() + 10); + + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("past the end of the central directory"); + } + + /** + * A central directory whose declared size runs into the record after it, in both a plain + * archive and a zip64 one. With no other candidate record to fall back on, the scan reports + * why it passed over the real one. + */ + @Test + public void testCentralDirectorySizeRunningIntoTheNextRecordIsRejected() throws IOException { + var plain = plainArchive(); + var plainBuf = ByteBuffer.wrap(plain).order(ByteOrder.LITTLE_ENDIAN); + int sizeField = plain.length - EOCD_SIZE + 12; + plainBuf.putInt(sizeField, plainBuf.getInt(sizeField) + 1); + + var zip64 = zip64Archive(); + int zip64Eocd = zip64.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE + ZIP64_EOCD_SIZE); + var zip64Buf = ByteBuffer.wrap(zip64).order(ByteOrder.LITTLE_ENDIAN); + assertThat(zip64Buf.getInt(zip64Eocd)).isEqualTo(ZIP64_EOCD_SIGNATURE); + zip64Buf.putLong(zip64Eocd + 40, zip64Buf.getLong(zip64Eocd + 40) + 1); + + for (var archive : List.of(plain, zip64)) { + assertThatThrownBy(() -> readArchive(archive)) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("which does not end where that record starts"); + } + } + /** The hand-built archives the hostile shapes start from are valid until a shape is applied. */ @Test public void testHandBuiltArchiveReads() throws IOException { From b138ed47b5f38a0dfc96c02f60cee3fa13c42429 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 22 Oct 2026 12:36:02 -0400 Subject: [PATCH 05/10] fix(sdk): wrap only the exceptions manifest parsing can raise (DSPX-5103) Gson's fromJson rethrows an IllegalStateException raised while parsing as JsonSyntaxException, and optionalString removed the last source of UnsupportedOperationException, so neither can reach TDFReader.readManifest. Narrow its catch to JsonParseException and IllegalArgumentException. Also correct the comment on TDFReader.manifest(): the archive bound comes from the entry's stream failing at the end of the archive, not from the compressed size check in ZipReader, which this read does not use. Signed-off-by: Dave Mihalcik Signed-off-by: Dave Mihalcik --- .../java/io/opentdf/platform/sdk/TDFReader.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index ab7106fb4..144994483 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -78,12 +78,8 @@ Manifest readManifest() { } catch (InvalidZipException e) { // the manifest entry runs past the end of the archive throw malformed("error retrieving manifest from zip file", e); - } catch (JsonParseException | IllegalArgumentException | IllegalStateException - | UnsupportedOperationException e) { - // Gson reports a JSON value of the wrong type through IllegalStateException and - // UnsupportedOperationException (the JsonElement.getAsX accessors), and - // Manifest.readManifest reports missing required fields as IllegalArgumentException. - // Every one of these here means the manifest itself is malformed. + } catch (JsonParseException | IllegalArgumentException e) { + // Manifest.readManifest reports a missing or misshapen field as IllegalArgumentException throw malformed("tdf manifest is invalid", e); } catch (IOException e) { throw new SDKException("error reading manifest from zip file", e); @@ -100,10 +96,8 @@ private static SDK.MalformedTDFException malformed(String what, RuntimeException * segments exceeds the maximum size of a Java String. */ Reader manifest() { - // deliberately no fixed size cap: very large payloads (DSPX-4502) have manifests well - // over a gigabyte. a hostile declared size cannot make this allocate more than the - // archive holds, because the stream stops at the end of the archive and reports the - // entry as truncated, and ZipReader rejects entries whose stored bytes do not fit + // no fixed size cap: manifests for very large payloads exceed 1 GiB. a hostile declared + // size is still bounded by the archive, since the entry's stream throws where it ends try { return new BufferedReader( new InputStreamReader(manifestEntry.getData(), StandardCharsets.UTF_8), MANIFEST_BUFFER_SIZE); From 1243e820f7f368c6eee2ea16e3c3757e0f88f6fb Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 22 Oct 2026 12:36:54 -0400 Subject: [PATCH 06/10] chore(sdk): trim the ZipReader hardening comments (DSPX-5103) Drop ticket references and bug history from comments; both belong in commit messages, and the history was stale anyway: main already reads the extra field data size unsigned, so it never looped there. Shorten javadoc on private helpers to the reason each one exists, and reword the entry count comment, which the per-header central directory bound made inaccurate. Signed-off-by: Dave Mihalcik Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/Manifest.java | 10 +---- .../java/io/opentdf/platform/sdk/SDK.java | 7 +--- .../java/io/opentdf/platform/sdk/TDF.java | 1 - .../io/opentdf/platform/sdk/TDFReader.java | 6 +-- .../io/opentdf/platform/sdk/ZipReader.java | 37 +++++-------------- .../io/opentdf/platform/sdk/HostileZips.java | 10 +---- .../io/opentdf/platform/sdk/ManifestTest.java | 7 +--- .../java/io/opentdf/platform/sdk/SDKTest.java | 6 --- .../opentdf/platform/sdk/TDFReaderTest.java | 5 +-- .../java/io/opentdf/platform/sdk/TDFTest.java | 16 ++------ .../opentdf/platform/sdk/ZipReaderTest.java | 7 +--- 11 files changed, 23 insertions(+), 89 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java index 96e8978e4..5b5edc397 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -1144,7 +1144,7 @@ public AssertionConfig.Statement deserialize(JsonElement json, Type typeOfT, Jso statement.format = optionalString(obj, "format"); statement.schema = optionalString(obj, "schema"); var value = obj.get("value"); - // an explicit JSON null is the same as no value at all, not the four character string "null" + // explicit JSON null means no value, not the string "null" if (value != null && !value.isJsonNull()) { if (value.isJsonPrimitive()) { // it's already a primitive (hopefully string) so we don't need its escaped value here @@ -1156,13 +1156,7 @@ public AssertionConfig.Statement deserialize(JsonElement json, Type typeOfT, Jso return statement; } - /** - * Reads a member that has to be a JSON primitive if present. A missing member and an - * explicit {@code null} both come back as {@code null}; anything else, such as an object - * or array, is a malformed statement. Calling {@code getAsString} unchecked throws - * {@link UnsupportedOperationException} or {@link IllegalStateException} for these, which - * escapes manifest parsing as something other than a parse error. - */ + /** The member as a string, or null if absent or JSON null; a non-primitive is a JsonParseException. */ private static String optionalString(JsonObject obj, String member) { var element = obj.get(member); if (element == null || element.isJsonNull()) { diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index ab0f00017..633f10405 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -177,11 +177,8 @@ public static boolean isTDF(SeekableByteChannel channel) { } catch (IOException | InvalidZipException e) { return false; } catch (RuntimeException e) { - // ZipReader reports every malformation it knows about as InvalidZipException, and its - // tests hold it to that. This is a backstop for the contract of this method, which - // screens untrusted input and answers yes or no: a parse failure we did not foresee, - // or an unchecked exception from the caller's channel, still means "not a TDF" - // rather than an exception from the very check meant to spare callers one. + // backstop for the yes-or-no contract: an unforeseen parse failure, or an unchecked + // exception from the caller's channel, still means "not a TDF" ZipReader.logger.debug("unexpected exception checking whether a channel holds a TDF", e); return false; } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java index 201b8ca3f..4ad242984 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -788,7 +788,6 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig, Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) throws SDKException, IOException { - // a malformed container or manifest is reported as SDK.MalformedTDFException TDFReader tdfReader = TDFReader.open(tdf); Manifest manifest = tdfReader.readManifest(); diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index 144994483..0f669962b 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -55,11 +55,7 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); } - /** - * Opens a TDF read from untrusted bytes, reporting a container that is not a well formed - * TDF as a {@link SDK.MalformedTDFException} rather than as whichever unchecked exception - * the zip reader or this class's own checks happened to raise. - */ + /** Like the constructor, but reports a malformed container as {@link SDK.MalformedTDFException}. */ static TDFReader open(SeekableByteChannel tdf) throws IOException { try { return new TDFReader(tdf); 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 61fb4c30c..bc6b327c8 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -485,11 +485,7 @@ public int read(byte[] b, int off, int len) throws IOException { /** Each extra field record starts with a 2-byte id and a 2-byte data size. */ private static final int EXTRA_FIELD_HEADER_SIZE = 4; - /** - * Checks that a {@code size} byte value read at the current position stays inside the zip64 - * extra field record ending at {@code fieldEnd}. A record too short for the values its - * sentinels promise would otherwise be read past, into whatever follows it. - */ + /** Throws if {@code size} bytes at the current position would run past {@code fieldEnd}. */ private void requireWithinField(String what, int size, long fieldEnd, long headerStart) throws IOException { if (zipChannel.position() + size > fieldEnd) { throw new InvalidZipException("The zip64 extra field of the central directory file header at offset " @@ -497,11 +493,7 @@ private void requireWithinField(String what, int size, long fieldEnd, long heade } } - /** - * Reads one 8-byte value out of a zip64 extra field record. The format calls these unsigned, - * but nothing can be larger than {@link Long#MAX_VALUE} bytes, so a value that comes out - * negative as a Java {@code long} is a corrupt or hostile archive rather than a big one. - */ + /** Reads an 8-byte zip64 value; one above {@link Long#MAX_VALUE} is rejected as corrupt. */ private long readZip64Value(String what, long fieldEnd, long headerStart) throws IOException { requireWithinField(what, Long.BYTES, fieldEnd, headerStart); long value = readLong(); @@ -517,10 +509,8 @@ public Entry readCentralDirectoryFileHeader() throws IOException { } /** - * Throws if a part of the central directory file header at {@code headerStart} ends after - * {@code centralDirectoryEnd}. Checking against the end of the directory rather than the end - * of the archive keeps an inflated length from reading the records that follow the directory - * as part of an entry. + * Bounded by the directory, not the archive, so an inflated length can't pull the records that + * follow the directory into an entry. */ private static void requireWithinCentralDirectory(String what, long end, long centralDirectoryEnd, long headerStart) { @@ -570,11 +560,8 @@ private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IO requireWithinCentralDirectory(extraFieldLength + " byte extra field", extraFieldEnd, centralDirectoryEnd, headerStart); - // walk the extra field as a sequence of (id, size, data) records. each step moves past a - // whole record, header included, so the walk always advances, and no record may claim - // more bytes than are left in the extra field. a sign-extended or oversized data size - // used to send the position backwards or beyond the field, looping forever or reading - // the next header as if it were part of this one + // each step moves past a whole record, header included, so the walk always advances and + // never leaves the extra field long fieldStart = extraFieldStart; while (extraFieldEnd - fieldStart >= EXTRA_FIELD_HEADER_SIZE) { zipChannel.position(fieldStart); @@ -612,9 +599,8 @@ private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IO long fileCommentEnd = extraFieldEnd + fileCommentLength; requireWithinCentralDirectory(fileCommentLength + " byte comment", fileCommentEnd, centralDirectoryEnd, headerStart); - // an honest archive stores every byte of an entry's data after its local header, so an - // entry that claims to start or end beyond the archive cannot be read. the uncompressed - // size is left to the reader, which reports a short entry as truncated when it gets there + // the entry's stored bytes must fit after its local header; the uncompressed size is + // checked when the entry is read if (relativeOffsetOfLocalHeader >= zipChannel.size() || compressedSize > zipChannel.size() - relativeOffsetOfLocalHeader) { throw new InvalidZipException("Entry [" + new String(fileName.array(), StandardCharsets.UTF_8) @@ -630,12 +616,9 @@ public ZipReader(SeekableByteChannel channel) throws IOException { zipChannel = channel; var centralDirectoryRecord = readEndOfCentralDirectory(); seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart); - // readEndOfCentralDirectory only accepts a directory that ends where the record after it - // starts. every header is then held to the declared size, so the records that follow the - // directory are never read as directory data long centralDirectoryEnd = centralDirectoryRecord.offsetToStart + centralDirectoryRecord.size; - // a corrupt zip64 count can be negative, which would read as an empty archive, or more - // than the directory can hold, which would read past its end + // a corrupt zip64 count can be negative, which would read as an empty archive; an + // excessive one would only fail at the first header past the end, with a vaguer message long mostEntriesThatFit = centralDirectoryRecord.size / CENTRAL_DIRECTORY_FILE_HEADER_MIN_SIZE; if (centralDirectoryRecord.numEntries < 0 || centralDirectoryRecord.numEntries > mostEntriesThatFit) { throw new InvalidZipException("The central directory claims " + centralDirectoryRecord.numEntries diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java index 0e0c41f05..711512cdd 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java @@ -10,12 +10,7 @@ import java.util.Map; import java.util.function.Consumer; -/** - * Builds small stored (uncompressed) zip archives by hand, so that a test can put any value it - * likes into a central directory field. {@link ZipWriter} and other writers only produce honest - * archives; these are for the hostile shapes from DSPX-5103, where lengths, sizes and offsets - * read as negative or point outside the archive. - */ +/** Hand-built stored zips whose central directory fields can be set to values no real writer produces. */ final class HostileZips { private HostileZips() { } @@ -52,8 +47,7 @@ static byte[] tdfWith(String manifestJson, Consumer hostile) { /** The named hostile shapes, each applied to the manifest entry of {@link #tdfWith}. */ static Map> hostileShapes() { var shapes = new LinkedHashMap>(); - // the shape that looped forever when the data size was read signed: 0xFFFC is -4, - // which sent the position straight back to the start of the record + // 0xFFFC is -4 if read signed, which would rewind the walk to the start of the record shapes.put("extra field data size 0xFFFC", e -> e.centralExtra = extraRecord(0xCAFE, 0xFFFC, new byte[4])); shapes.put("extra field data size past the extra field", e -> e.centralExtra = extraRecord(0xCAFE, 16, new byte[4])); shapes.put("zip64 local header offset 0xFFFFFFFFFFFFFFFF", e -> { diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java index afba20d14..18445ef8b 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java @@ -495,11 +495,7 @@ private String withStatementMember(String member, com.google.gson.JsonElement re return new GsonBuilder().serializeNulls().create().toJson(root); } - /** - * DSPX-5103: an explicit JSON null in a statement field is the same as leaving it out. It - * used to throw UnsupportedOperationException for format and schema, which escaped manifest - * parsing, and to become the string "null" for value. - */ + /** An explicit JSON null in a statement field reads as absent, not as the string "null". */ @Test void testNullStatementFieldsReadAsNull() throws IOException { for (var member : List.of("format", "schema", "value")) { @@ -511,7 +507,6 @@ void testNullStatementFieldsReadAsNull() throws IOException { } } - /** A statement format or schema that is not a string is a parse error, not an unchecked crash. */ @Test void testNonStringStatementFormatIsAParseError() throws IOException { for (var member : List.of("format", "schema")) { diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index 8b3f1842a..c096bcaeb 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -111,10 +111,6 @@ void testExaminingZipThatListsAnUnrelatedNameTwice() throws IOException { } } - /** - * DSPX-5103: {@link SDK#isTDF} answers a malformed archive with false. It used to hang on - * one of these shapes and let an unchecked exception escape on others. - */ @Test void testExaminingHostileArchivesReturnsFalse() { HostileZips.hostileShapes().forEach((shape, hostile) -> { @@ -131,7 +127,6 @@ void testExaminingHandBuiltTDF() { assertThat(SDK.isTDF(new SeekableInMemoryByteChannel(HostileZips.tdfWith("{}", e -> { })))).isTrue(); } - /** A channel that throws something unchecked is not a TDF either, rather than a crash in the check. */ @Test void testExaminingAChannelThatThrowsUncheckedReturnsFalse() { var channel = new SeekableInMemoryByteChannel(HostileZips.tdfWith("{}", e -> { })) { @@ -143,7 +138,6 @@ public int read(ByteBuffer buf) { assertThat(SDK.isTDF(channel)).isFalse(); } - /** {@link SDK#readManifest} reports the same archives as a malformed TDF. */ @Test void testReadingTheManifestOfHostileArchivesThrowsMalformedTDF() { HostileZips.hostileShapes().forEach((shape, hostile) -> { diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 6993fe1e4..612f578f5 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -193,10 +193,7 @@ void readsThePayloadAlongsideASpecNamedManifest() throws IOException { } } - /** - * A payload whose declared size runs past the end of the archive is only detected when it is - * read. Like a truncated manifest, it is a malformed TDF, not a raw zip reader exception. - */ + /** An oversized payload is only detected on read, and still surfaces as a malformed TDF. */ @Test void reportsATruncatedPayloadAsMalformed() throws IOException { var payload = new HostileZips.Entry(TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD.getBytes(StandardCharsets.UTF_8)); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index 7e9598111..0f7d8cb80 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -1505,11 +1505,6 @@ private static Config.KASInfo[] getSingleRSAKASInfo() { return getKASInfos(i -> i == 0); } - /** - * DSPX-5103: a malformed container reaches the caller of {@code loadTDF} as a - * {@link SDK.MalformedTDFException}, never as a hang or an IllegalArgumentException, - * UnsupportedOperationException or other unchecked exception from inside the parser. - */ @Test void loadTDFReportsHostileArchivesAsMalformed() { HostileZips.hostileShapes().forEach((shape, hostile) -> @@ -1517,9 +1512,8 @@ void loadTDFReportsHostileArchivesAsMalformed() { } /** - * A manifest that claims more bytes than the archive holds is truncated, and is reported as - * malformed rather than read short or buffered without bound. This, not a size cap, is what - * stops a hostile declared size. + * A size past the end of the archive is malformed, not read short: an oversized uncompressed + * size fails when the manifest is read, an oversized compressed size when the archive is opened. */ @Test void loadTDFReportsAManifestRunningPastTheArchiveAsMalformed() { @@ -1537,11 +1531,7 @@ void loadTDFReportsAManifestRunningPastTheArchiveAsMalformed() { .hasCauseInstanceOf(InvalidZipException.class); } - /** - * There is deliberately no fixed manifest size cap: very large payloads (DSPX-4502) have - * manifests far bigger than any fixed limit would allow. A manifest larger than the 10 MiB - * cap this reader briefly had is read whole. - */ + /** Guards against reintroducing a fixed manifest size cap. */ @Test void readsAManifestLargerThanTenMebibytes() throws IOException { var padding = "a".repeat(11 * 1024 * 1024); 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 a6b7246eb..f5736c24e 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -761,12 +761,7 @@ public long size() { } } - /** - * DSPX-5103: central directory fields that read as negative, or that point outside the - * archive or outside their own extra field, are zip errors. None of them may hang the reader - * (a data size of 0xFFFC once looped forever) or escape as an unchecked exception other than - * {@link InvalidZipException}. - */ + /** Every hostile shape fails with {@link InvalidZipException}, without hanging. */ @Test public void testHostileCentralDirectoryFieldsAreRejected() { HostileZips.hostileShapes().forEach((shape, hostile) -> { From 0eacdaa481a26f8fc65a7c7a5a95ad177218e20a Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 22 Oct 2026 12:39:34 -0400 Subject: [PATCH 07/10] fix(sdk): test the open() wrap and the zip64 record bound directly (DSPX-5103) TDFReader.open wraps the IllegalArgumentException its constructor raises for a zip that is not a TDF, but only the constructor was tested, so dropping the wrap left every test green. Test it through open(). The zip64 extra field too short for its sentinels shape followed the record with bytes that read as an oversized compressed size, so the archive bound rejected it even without the record bound. Follow it with two empty records instead, which read as a size that fits, and update both fuzz seeds for the shape to match. Signed-off-by: Dave Mihalcik Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/HostileZips.java | 8 +++++--- .../opentdf/platform/sdk/TDFReaderTest.java | 18 ++++++++++++++++++ ...64-extra-field-too-short-for-its-sentinels | Bin 257 -> 257 bytes ...64-extra-field-too-short-for-its-sentinels | Bin 257 -> 257 bytes 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java index 711512cdd..9db94d05e 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/HostileZips.java @@ -62,12 +62,14 @@ static Map> hostileShapes() { e.compressedSize = 0xFFFFFFFFL; e.centralExtra = extraRecord(0x0001, 8, longs(Long.MIN_VALUE)); }); - // both sizes carry the sentinel, but the record only has room for one of them; the - // second must not be read out of whatever follows the record + // both sizes use the sentinel, but the zip64 record holds only one value. the two empty + // records after it read as a compressed size of 16, which fits, so only the record bound + // rejects this shapes.put("zip64 extra field too short for its sentinels", e -> { e.uncompressedSize = 0xFFFFFFFFL; e.compressedSize = 0xFFFFFFFFL; - e.centralExtra = concat(extraRecord(0x0001, 8, longs(e.data.length)), extraRecord(0xCAFE, 4, new byte[4])); + e.centralExtra = concat(extraRecord(0x0001, 8, longs(e.data.length)), + extraRecord(0x0010, 0, new byte[0]), extraRecord(0x0000, 0, new byte[0])); }); shapes.put("file name length 0xFFFF", e -> e.fileNameLength = 0xFFFF); shapes.put("extra field length 0xFFFF", e -> e.extraFieldLength = 0xFFFF); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index 612f578f5..b366566dd 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -178,6 +178,24 @@ void rejectsAnArchiveThatListsTheManifestNameTwice() throws IOException { } } + /** A well formed zip that is not a TDF: no manifest, no payload, or a name listed twice. */ + @Test + void openReportsAZipThatIsNotATDFAsMalformed() throws IOException { + var notTDFs = List.of( + new String[] { TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD }, + new String[] { "manifest.json", MANIFEST }, + new String[] { TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST, "manifest.json", MANIFEST }); + for (var entries : notTDFs) { + try (var tdf = archiveOf(entries)) { + assertThatThrownBy(() -> TDFReader.open(tdf)) + .as(String.join(", ", entries)) + .isInstanceOf(SDK.MalformedTDFException.class) + .hasCauseInstanceOf(IllegalArgumentException.class); + } + } + } + /** The buffer is one byte longer than the payload, so a short read would show up. */ @Test void readsThePayloadAlongsideASpecNamedManifest() throws IOException { diff --git a/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-extra-field-too-short-for-its-sentinels b/sdk/src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/fuzzTDF/dspx5103-zip64-extra-field-too-short-for-its-sentinels index ec54a94cd7bd4a7d03f9b63181a2fabd38c7caf2..5e96973ac6ba7d7e9bec02570d353a95191e5800 100644 GIT binary patch delta 37 fcmZo Date: Thu, 22 Oct 2026 15:29:22 -0400 Subject: [PATCH 08/10] chore(sdk): address SonarCloud findings on the ZipReader hardening (DSPX-5103) - ZipReader: read the zip64 sentinel fields through one helper, bringing readCentralDirectoryFileHeader under the cognitive complexity limit - ZipReaderTest: keep one throwing call per assertThatThrownBy lambda - ManifestTest, SDKTest: replace assert statements, which only run with -ea, with Objects.requireNonNull; the object-statement test reuses the existing resource helper Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/ZipReader.java | 18 +++++++++--------- .../io/opentdf/platform/sdk/ManifestTest.java | 12 ++++-------- .../java/io/opentdf/platform/sdk/SDKTest.java | 4 ++-- .../io/opentdf/platform/sdk/ZipReaderTest.java | 11 ++++------- 4 files changed, 19 insertions(+), 26 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 bc6b327c8..d246113ba 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -504,6 +504,11 @@ private long readZip64Value(String what, long fieldEnd, long headerStart) throws return value; } + /** The field as read, or the next zip64 value if the field holds the 0xFFFFFFFF sentinel. */ + private long zip64ValueIfSentinel(long field, String what, long fieldEnd, long headerStart) throws IOException { + return field == ZIP64_MAGICVAL ? readZip64Value(what, fieldEnd, headerStart) : field; + } + public Entry readCentralDirectoryFileHeader() throws IOException { return readCentralDirectoryFileHeader(zipChannel.size()); } @@ -577,15 +582,10 @@ private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IO if (headerId == ZIP64_EXTID) { // APPNOTE 4.5.3 order: original size, compressed size, then local header offset - if (uncompressedSize == ZIP64_MAGICVAL) { - uncompressedSize = readZip64Value("uncompressed size", dataEnd, headerStart); - } - if (compressedSize == ZIP64_MAGICVAL) { - compressedSize = readZip64Value("compressed size", dataEnd, headerStart); - } - if (relativeOffsetOfLocalHeader == ZIP64_MAGICVAL) { - relativeOffsetOfLocalHeader = readZip64Value("local header offset", dataEnd, headerStart); - } + uncompressedSize = zip64ValueIfSentinel(uncompressedSize, "uncompressed size", dataEnd, headerStart); + compressedSize = zip64ValueIfSentinel(compressedSize, "compressed size", dataEnd, headerStart); + relativeOffsetOfLocalHeader = zip64ValueIfSentinel(relativeOffsetOfLocalHeader, + "local header offset", dataEnd, headerStart); // a 2-byte field, so its sentinel is 0xFFFF rather than 0xFFFFFFFF if (diskNumberStart == ZIP64_MAGIC_SHORT) { requireWithinField("disk number", Integer.BYTES, dataEnd, headerStart); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java index 18445ef8b..eafd1fd7f 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java @@ -17,6 +17,7 @@ import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; @@ -334,12 +335,7 @@ public void close() { @Test void testReadingManifestWithObjectStatementValue() throws IOException { - final Manifest manifest; - try (var mStream = getClass().getResourceAsStream("/io.opentdf.platform.sdk.TestData/manifest-with-object-statement-value.json")) { - assert mStream != null; - var manifestJson = new String(mStream.readAllBytes(), StandardCharsets.UTF_8); - manifest = Manifest.readManifest(manifestJson); - } + var manifest = Manifest.readManifest(manifestWithObjectStatementValue()); assertThat(manifest.assertions).hasSize(2); @@ -481,8 +477,8 @@ static Stream nonAlignedPlacements() { } private String manifestWithObjectStatementValue() throws IOException { - try (var mStream = getClass().getResourceAsStream("/io.opentdf.platform.sdk.TestData/manifest-with-object-statement-value.json")) { - assert mStream != null; + try (var mStream = Objects.requireNonNull(getClass().getResourceAsStream( + "/io.opentdf.platform.sdk.TestData/manifest-with-object-statement-value.json"))) { return new String(mStream.readAllBytes(), StandardCharsets.UTF_8); } } diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index c096bcaeb..138bd5358 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -16,6 +16,7 @@ import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; import java.time.Duration; +import java.util.Objects; import java.util.Random; import static org.assertj.core.api.Assertions.assertThat; @@ -27,8 +28,7 @@ class SDKTest { @Test void testExaminingValidZTDF() throws IOException { - try (var ztdf = SDKTest.class.getClassLoader().getResourceAsStream("sample.txt.tdf")) { - assert ztdf != null; + try (var ztdf = Objects.requireNonNull(SDKTest.class.getClassLoader().getResourceAsStream("sample.txt.tdf"))) { var chan = new SeekableInMemoryByteChannel(ztdf.readAllBytes()); assertThat(SDK.isTDF(chan)).isTrue(); } 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 f5736c24e..03f21913c 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -766,14 +766,11 @@ public long size() { public void testHostileCentralDirectoryFieldsAreRejected() { HostileZips.hostileShapes().forEach((shape, hostile) -> { var archive = HostileZips.tdfWith(MANIFEST, hostile); + // reading every entry, not just the directory, so a shape that only fails once its + // data is read is caught too assertTimeoutPreemptively(Duration.ofSeconds(5), () -> - assertThatThrownBy(() -> { - try (var channel = new SeekableInMemoryByteChannel(archive)) { - // reading every entry, not just the directory, so a shape that only - // fails once its data is read is caught too - testReadingZipChannel(channel, false); - } - }).as(shape).isInstanceOf(InvalidZipException.class), + assertThatThrownBy(() -> testReadingZipChannel(new SeekableInMemoryByteChannel(archive), false)) + .as(shape).isInstanceOf(InvalidZipException.class), shape); }); } From 110e6acc84f153242d05e5e3b34853f8be0559c7 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 23 Oct 2026 11:53:47 -0400 Subject: [PATCH 09/10] chore(sdk): fix the two remaining SonarCloud test smells (DSPX-5103) Signed-off-by: Dave Mihalcik --- .../test/java/io/opentdf/platform/sdk/TDFTest.java | 2 +- .../io/opentdf/platform/sdk/ZipReaderTest.java | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index 0f7d8cb80..0ddb0b9d5 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -1541,7 +1541,7 @@ void readsAManifestLargerThanTenMebibytes() throws IOException { try (var reader = TDFReader.open(new SeekableInMemoryByteChannel(archive)).manifest()) { reader.transferTo(read); } - assertThat(read.toString()).isEqualTo(manifest); + assertThat(read).hasToString(manifest); } @Test 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 03f21913c..534466d97 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -766,12 +766,14 @@ public long size() { public void testHostileCentralDirectoryFieldsAreRejected() { HostileZips.hostileShapes().forEach((shape, hostile) -> { var archive = HostileZips.tdfWith(MANIFEST, hostile); - // reading every entry, not just the directory, so a shape that only fails once its - // data is read is caught too - assertTimeoutPreemptively(Duration.ofSeconds(5), () -> - assertThatThrownBy(() -> testReadingZipChannel(new SeekableInMemoryByteChannel(archive), false)) - .as(shape).isInstanceOf(InvalidZipException.class), - shape); + try (var channel = new SeekableInMemoryByteChannel(archive)) { + // reading every entry, not just the directory, so a shape that only fails once its + // data is read is caught too + assertTimeoutPreemptively(Duration.ofSeconds(5), () -> + assertThatThrownBy(() -> testReadingZipChannel(channel, false)) + .as(shape).isInstanceOf(InvalidZipException.class), + shape); + } }); } From 418fb6a973ccaebecfce168a4c0235cf42aaf41b Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 23 Oct 2026 13:41:40 -0400 Subject: [PATCH 10/10] fix(sdk): reject an entry whose data starts past the end of the archive (DSPX-5103) Signed-off-by: Dave Mihalcik --- .../io/opentdf/platform/sdk/ZipReader.java | 16 +++++++++++++--- .../opentdf/platform/sdk/ZipReaderTest.java | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 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 d246113ba..c1d24e39a 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -415,7 +415,16 @@ private long findStartOfData() throws IOException { int filenameLength = readUnsignedShort(); int extrafieldLength = readUnsignedShort(); - return zipChannel.position() + filenameLength + extrafieldLength; + long startOfData = zipChannel.position() + filenameLength + extrafieldLength; + // the central directory can't see these two lengths, so this is the first place a + // start past the end shows. an empty entry is never read, so unchecked it would come + // back empty rather than fail as truncated + if (startOfData > zipChannel.size()) { + throw new InvalidZipException("The local file header of entry [" + fileName + "] at offset " + + offsetToLocalHeader + " puts its data at offset " + startOfData + + ", past the end of this " + zipChannel.size() + " byte archive"); + } + return startOfData; } public InputStream getData() throws IOException { @@ -599,8 +608,9 @@ private Entry readCentralDirectoryFileHeader(long centralDirectoryEnd) throws IO long fileCommentEnd = extraFieldEnd + fileCommentLength; requireWithinCentralDirectory(fileCommentLength + " byte comment", fileCommentEnd, centralDirectoryEnd, headerStart); - // the entry's stored bytes must fit after its local header; the uncompressed size is - // checked when the entry is read + // counted from the start of its local header, the entry's stored bytes must fit in the + // archive. where its data actually starts, and its uncompressed size, are checked when + // the entry is read if (relativeOffsetOfLocalHeader >= zipChannel.size() || compressedSize > zipChannel.size() - relativeOffsetOfLocalHeader) { throw new InvalidZipException("Entry [" + new String(fileName.array(), StandardCharsets.UTF_8) 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 534466d97..7cc188b53 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -827,6 +827,25 @@ public void testHandBuiltArchiveReads() throws IOException { } } + /** + * The local header of an empty entry claims a filename long enough to put its data past the + * end of the archive. The central directory can't see that length, and an empty entry is + * never read, so without a check on the local header it would read as empty. + */ + @Test + public void testEmptyEntryWhoseDataStartsPastTheEndIsRejected() throws IOException { + var archive = HostileZips.build(List.of(new HostileZips.Entry("empty", new byte[0]))); + // the filename length of the first local file header, which starts at offset 0 + ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).putShort(26, (short) 0xFFFF); + + try (var channel = new SeekableInMemoryByteChannel(archive)) { + var entry = new ZipReader(channel).getEntries().get(0); + assertThatThrownBy(entry::getData) + .isInstanceOf(InvalidZipException.class) + .hasMessageContaining("past the end of this"); + } + } + /** * A zip64 record whose values are all within range is still honoured, after an unrelated * record and followed by a few bytes too short to be a record, which readers ignore.