From cff59da45590fa2614406b41bb0b1530c782088e Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 9 Oct 2026 14:30:26 -0400 Subject: [PATCH 1/5] spec: scaffold for DSPX-5103 harden java ZipReader against hostile central directory fields Signed-off-by: Dave Mihalcik --- spec/DSPX-5103.md | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 spec/DSPX-5103.md diff --git a/spec/DSPX-5103.md b/spec/DSPX-5103.md new file mode 100644 index 00000000..2297b6f1 --- /dev/null +++ b/spec/DSPX-5103.md @@ -0,0 +1,54 @@ +--- +ticket: DSPX-5103 +title: java-sdk: ZipReader/manifest parsing hangs or throws unchecked exceptions on malformed archives (isTDF / loadTDF) +status: draft +authors: [dmihalcik@virtru.com] +branches: [opentdf/java-sdk:DSPX-5103-harden-java-zipreader-against-hostile-central-dire] +prs: [] +created: 2026-10-09 +updated: 2026-10-09 +--- + +# java-sdk: ZipReader/manifest parsing hangs or throws unchecked exceptions on malformed archives (isTDF / loadTDF) + +## Summary +Java counterpart of DSPX-5102 (opentdf/platform#4202). The Go SDK panics on malformed archives passed to IsValidTdf / LoadTDF. The Java ZIP reader has the same root cause: sizes, offsets and lengths are read as signed values and never validated. In Java this surfaces as a hang or an unchecked exception escaping SDK.isTDF / loadTDF, rather than a panic. +Found by source inspection of java-sdk @ 57d070b0 (0.18.0). Paths are under sdk/src/main/java/io/opentdf/platform/sdk/. Not yet reproduced at runtime, except where noted as confirmed by reading the code. +Findings1. Infinite loop on a negative extra-field dataSize (DoS)ZipReader.java:262-283: int dataSize = readShort(); sign-extends, so 0xFFFC becomes -4. The end of each loop iteration runs zipChannel.position(fieldStart + dataSize + 4), which resets the position to fieldStart. The for loop then never advances. A 4-byte edit to any TDF's central directory hangs the reading thread. This is the most severe item here. +2. Negative lengths reach ByteBuffer.allocate / position()ZipReader.java:247,255: int fileNameLength = readShort(); turns a filename length above 32767 into a negative number, and ByteBuffer.allocate(negative) throws IllegalArgumentException. This is the closest analog of the Go makeslice panic. +ZipReader.java:270-276: a ZIP64 extra-field offset of 0xFFFFFFFFFFFFFFFF is read as -1. zipChannel.position(-1) (:167) then throws IllegalArgumentException. +ZipReader.java:133: the ZIP64 EOCD-locator offset is passed to position() without a check. A negative value throws the same way. +A ZIP64 size of -1 makes doneReading() return true immediately, so the manifest reads as "". Gson then fails. This is not a crash, but it produces a misleading error. +extraFieldLength and fileCommentLength also come from signed readShort(). +3. No manifest size capTDFReader.java:43 reads the manifest entry into an unbounded ByteArrayOutputStream via transferTo. A huge declared size is limited only by the real file size, so a crafted multi-GB container can OOM the reader. Go caps the manifest with maxSize, and web-sdk caps it at 10 MB. +4. Unchecked exceptions escape isTDFSDK.isTDF (SDK.java:164-176) catches only IOException | InvalidZipException. Everything in items 1–2 propagates out of a method whose contract is to return a boolean. TDF.loadTDF (TDF.java:~567) does not wrap these exceptions either. +5. Null sub-fields in an assertion statementManifest.java:540,543: "format": null or "schema": null inside an assertion statement throws an unchecked UnsupportedOperationException from JsonNull.getAsString() during readManifest. "value": null is silently turned into the string "null". A top-level "statement": null is handled safely. Gson is case-sensitive, so the Go "Statement": null trick does not apply. +Out of scope hereThe reader trusting payload.isEncrypted = false (item 3 of the GitHub issue) is DSPX-4748. It is still present: TDF.java:580 skips unwrap, and :657/:667/:677-686 fall back to keyless SHA-256. isTDF only checks entry names, so it also accepts these files. +ZIP64 conformance (APPNOTE thresholds and field order) is DSPX-4589. This ticket covers robustness against hostile input only. +Proposed fixRead all 16- and 32-bit ZIP fields as unsigned (Short.toUnsignedInt, Integer.toUnsignedLong), and use explicit sentinel comparisons instead of == -1. +Reject any ZIP64 value above Long.MAX_VALUE, and any offset or offset+size beyond channel.size(), with InvalidZipException. +Require each extra-field iteration to advance, and check that dataSize fits within the remaining extraFieldLength. web-sdk's sliceExtraFields (zip-reader.ts:316-339) is a good model. +Cap the manifest size (match go/web), and use getAsJsonPrimitive-style null checks in Manifest deserializers. +Make isTDF return false for any parse failure, and have loadTDF throw an SDK exception. +Acceptance criteriaRegression tests for each input above. None may hang or throw an unchecked exception; isTDF returns false and loadTDF throws a typed SDK exception. +Add the shapes to the Jazzer corpus (DSPX-5070), and add a timeout so a hang is reported as a failure. +Cover them in the xtest structural-mutation suite (DSPX-4923) so go, java and web are held to the same "clean rejection" rule. + +## Problem / Motivation +_Why does this work need to happen? What is the user/business pain?_ + +## Proposed Solution +_What will you build, at a functional level? Sketch the approach._ + +## Inputs / Outputs / Contracts +_Function signatures, data shapes, API contracts, CLI flags._ + +## Edge Cases & Constraints +_Boundary conditions, error states, performance limits, security considerations._ + +## Out of Scope +_What this work item explicitly does not cover._ + +## Acceptance Criteria +- [ ] _Clear, testable condition_ +- [ ] _…_ From 8f427c6d784275d0e1672ac3c192b29793000c93 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 9 Oct 2026 14:38:42 -0400 Subject: [PATCH 2/5] 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 | 17 +- .../java/io/opentdf/platform/sdk/TDF.java | 7 +- .../io/opentdf/platform/sdk/TDFReader.java | 55 +++++- .../io/opentdf/platform/sdk/ZipReader.java | 99 +++++++++-- .../io/opentdf/platform/sdk/HostileZips.java | 167 ++++++++++++++++++ .../io/opentdf/platform/sdk/ManifestTest.java | 48 +++++ .../java/io/opentdf/platform/sdk/SDKTest.java | 48 +++++ .../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 spec/DSPX-5103.md | 51 +++++- 33 files changed, 581 insertions(+), 41 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 ae03a1d5..e8636cb6 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -741,14 +741,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(); @@ -758,6 +755,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 f69b6896..ab0f0001 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,13 +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); - String manifestJson = reader.manifest(); - return Manifest.readManifest(manifestJson); + return TDFReader.open(tdfBytes).readManifest(); } /** @@ -388,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 fddead94..ff780ba8 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -803,10 +803,9 @@ Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig, Reader loadTDF(SeekableByteChannel tdf, Config.TDFReaderConfig tdfReaderConfig) throws SDKException, IOException { - TDFReader tdfReader = new TDFReader(tdf); - String manifestJson = tdfReader.manifest(); - // use Manifest.readManifest in order to validate the Manifest input - 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 93548d67..f2bbc9c8 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.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; @@ -49,10 +51,60 @@ 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() { + String manifestJson = manifest(); + try { + return Manifest.readManifest(manifestJson); + } 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); + } + } + + private static SDK.MalformedTDFException malformed(String what, RuntimeException e) { + return new SDK.MalformedTDFException(what + ": " + e.getMessage(), e); + } + String 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"); + } var out = new ByteArrayOutputStream(); try { manifestEntry.getData().transferTo(out); + } 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); } @@ -74,8 +126,7 @@ int readPayloadBytes(byte[] buf) { } PolicyObject readPolicyObject() { - String manifestJson = manifest(); - Manifest manifest = Manifest.readManifest(manifestJson); + 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 d2fd6e91..d7297a4b 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 00000000..0e0c41f0 --- /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 45de6945..839a4089 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,10 @@ 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; import org.junit.jupiter.params.ParameterizedTest; @@ -14,6 +18,7 @@ import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertEquals; public class ManifestTest { @@ -407,4 +412,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 bd099a00..8b3f1842 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/TDFTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java index aa5b38e6..13cdae30 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 13289d50..caf4c898 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 diff --git a/spec/DSPX-5103.md b/spec/DSPX-5103.md index 2297b6f1..7ffd0e52 100644 --- a/spec/DSPX-5103.md +++ b/spec/DSPX-5103.md @@ -1,10 +1,10 @@ --- ticket: DSPX-5103 title: java-sdk: ZipReader/manifest parsing hangs or throws unchecked exceptions on malformed archives (isTDF / loadTDF) -status: draft +status: in-review authors: [dmihalcik@virtru.com] branches: [opentdf/java-sdk:DSPX-5103-harden-java-zipreader-against-hostile-central-dire] -prs: [] +prs: [opentdf/java-sdk#418] created: 2026-10-09 updated: 2026-10-09 --- @@ -35,20 +35,53 @@ Add the shapes to the Jazzer corpus (DSPX-5070), and add a timeout so a hang is Cover them in the xtest structural-mutation suite (DSPX-4923) so go, java and web are held to the same "clean rejection" rule. ## Problem / Motivation -_Why does this work need to happen? What is the user/business pain?_ +`SDK.isTDF` is meant to screen untrusted bytes and answer yes or no, and `loadTDF` / `SDK.readManifest` +are the entry points for untrusted containers. A hostile archive could hang the calling thread or make +them throw unchecked exceptions (IllegalArgumentException, UnsupportedOperationException) that callers +screening input do not expect. A crafted manifest size could also force a large allocation. ## Proposed Solution -_What will you build, at a functional level? Sketch the approach._ +Stacked on opentdf/java-sdk#398 (DSPX-4589), which already reads the central directory fields unsigned +and bounds-checks the EOCD, zip64 locator, central directory offset and entry count. On top of that: + +- `ZipReader.readCentralDirectoryFileHeader` walks the extra field as whole records. Each step moves at + least one 4-byte header forward. A record whose data size runs past the extra field is rejected, and + so is a zip64 record too short for the values its sentinels promise. Fewer than 4 trailing bytes are + ignored, as Go's `archive/zip` does. +- Zip64 sizes and offsets above `Long.MAX_VALUE` are rejected. So are an extra field or file comment that + runs past the archive, and an entry whose local header offset or compressed size does not fit in it. + The uncompressed size stays lazily checked: reading a short entry reports truncation (from #398). +- `TDFReader` caps the manifest at 10 MiB (`MAX_MANIFEST_SIZE`, the Go default and the web limit). The + cap is checked against the declared size before anything is read. +- `TDFReader.open` / `TDFReader.readManifest` turn container and manifest malformation into + `SDK.MalformedTDFException`. `TDF.loadTDF` and `SDK.readManifest` go through them. +- `SDK.isTDF` returns false for any `RuntimeException`, as a backstop for its boolean contract. +- `Manifest.AssertionValueAdapter`: a JSON `null` in `format`, `schema` or `value` reads as `null`. A + non-primitive `format` or `schema` is a `JsonParseException`. ## Inputs / Outputs / Contracts -_Function signatures, data shapes, API contracts, CLI flags._ +- `SDK.isTDF(channel)`: never throws for malformed input; returns false. +- `TDF.loadTDF(...)`, `SDK.readManifest(...)`: a malformed container or manifest throws + `SDK.MalformedTDFException` (an `SDKException`). These used to throw `IllegalArgumentException`, + `InvalidZipException` or Gson exceptions. +- `ZipReader`: every malformation it detects is an `InvalidZipException`. +- New additive API: `SDK.MalformedTDFException(String, Exception)`. ## Edge Cases & Constraints -_Boundary conditions, error states, performance limits, security considerations._ +- The manifest cap is not yet configurable. Go exposes `WithMaxManifestSize`, so a reader option could + follow if anyone needs larger manifests. +- `TDFReader`'s constructor still throws `IllegalArgumentException` for a missing manifest or payload or + a duplicate entry. Only the `open` path converts these. ## Out of Scope -_What this work item explicitly does not cover._ +- Trusting `payload.isEncrypted = false` (DSPX-4748) is still open. +- ZIP64 writer conformance (DSPX-4589 / #398). +- The xtest structural-mutation suite (DSPX-4923). ## Acceptance Criteria -- [ ] _Clear, testable condition_ -- [ ] _…_ +- [x] Regression tests for each hostile shape (`HostileZips.hostileShapes()`), run against `ZipReader`, + `SDK.isTDF`, `SDK.readManifest` and `TDF.loadTDF`, each under a 5 s preemptive timeout. +- [x] Valid archives still read. This includes a zip64 extra field among other records, with trailing + padding. +- [x] Manifest tests for null and non-string statement fields. +- [x] The shapes are added as Jazzer seeds under `FuzzingInputs/fuzzZipRead` and `FuzzingInputs/fuzzTDF`. From e3f31b92f7bafb888422dd0658c5d6498cb0d995 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 9 Oct 2026 15:24:21 -0400 Subject: [PATCH 3/5] 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 | 33 ++++++++++++++++--- spec/DSPX-5103.md | 14 +++++--- 4 files changed, 41 insertions(+), 32 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 f2bbc9c8..80b55d30 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -51,14 +51,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 @@ -95,11 +87,10 @@ private static SDK.MalformedTDFException malformed(String what, RuntimeException } String 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 var out = new ByteArrayOutputStream(); try { manifestEntry.getData().transferTo(out); 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 d7297a4b..bbc0fe86 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 13cdae30..dd582274 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -1515,13 +1515,36 @@ 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() { + 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 -> { }); + assertThat(TDFReader.open(new SeekableInMemoryByteChannel(archive)).manifest()).isEqualTo(manifest); } @Test diff --git a/spec/DSPX-5103.md b/spec/DSPX-5103.md index 7ffd0e52..a050bdc6 100644 --- a/spec/DSPX-5103.md +++ b/spec/DSPX-5103.md @@ -38,7 +38,7 @@ Cover them in the xtest structural-mutation suite (DSPX-4923) so go, java and we `SDK.isTDF` is meant to screen untrusted bytes and answer yes or no, and `loadTDF` / `SDK.readManifest` are the entry points for untrusted containers. A hostile archive could hang the calling thread or make them throw unchecked exceptions (IllegalArgumentException, UnsupportedOperationException) that callers -screening input do not expect. A crafted manifest size could also force a large allocation. +screening input do not expect. ## Proposed Solution Stacked on opentdf/java-sdk#398 (DSPX-4589), which already reads the central directory fields unsigned @@ -51,8 +51,11 @@ and bounds-checks the EOCD, zip64 locator, central directory offset and entry co - Zip64 sizes and offsets above `Long.MAX_VALUE` are rejected. So are an extra field or file comment that runs past the archive, and an entry whose local header offset or compressed size does not fit in it. The uncompressed size stays lazily checked: reading a short entry reports truncation (from #398). -- `TDFReader` caps the manifest at 10 MiB (`MAX_MANIFEST_SIZE`, the Go default and the web limit). The - cap is checked against the declared size before anything is read. +- There is deliberately no fixed manifest size cap. DSPX-4502 targets 50 TiB payloads, whose manifests + exceed 1 GiB, so any fixed limit would break legitimate TDFs. A hostile declared size is handled by the + archive bounds checks instead. An entry whose stored bytes don't fit in the archive is rejected, and a + manifest whose declared size runs past the end fails as truncated. So reading a manifest never buffers + more than the archive holds. Scaling memory use for huge manifests is DSPX-4584. - `TDFReader.open` / `TDFReader.readManifest` turn container and manifest malformation into `SDK.MalformedTDFException`. `TDF.loadTDF` and `SDK.readManifest` go through them. - `SDK.isTDF` returns false for any `RuntimeException`, as a backstop for its boolean contract. @@ -68,8 +71,7 @@ and bounds-checks the EOCD, zip64 locator, central directory offset and entry co - New additive API: `SDK.MalformedTDFException(String, Exception)`. ## Edge Cases & Constraints -- The manifest cap is not yet configurable. Go exposes `WithMaxManifestSize`, so a reader option could - follow if anyone needs larger manifests. +- The manifest is still read whole into memory, bounded only by the archive's size (see DSPX-4584). - `TDFReader`'s constructor still throws `IllegalArgumentException` for a missing manifest or payload or a duplicate entry. Only the `open` path converts these. @@ -83,5 +85,7 @@ and bounds-checks the EOCD, zip64 locator, central directory offset and entry co `SDK.isTDF`, `SDK.readManifest` and `TDF.loadTDF`, each under a 5 s preemptive timeout. - [x] Valid archives still read. This includes a zip64 extra field among other records, with trailing padding. +- [x] A manifest whose declared size runs past the archive is rejected as malformed. A manifest larger + than 10 MiB is still read. - [x] Manifest tests for null and non-string statement fields. - [x] The shapes are added as Jazzer seeds under `FuzzingInputs/fuzzZipRead` and `FuzzingInputs/fuzzTDF`. From 9bc5236cf4bc696f1426efc2108c0ddcbfd8b9bc Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 14 Oct 2026 09:38:50 -0400 Subject: [PATCH 4/5] 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 80b55d30..65855860 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -110,6 +110,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 119dc1a3..f0a7e7dd 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -6,6 +6,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; 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; @@ -164,4 +165,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 fcc585a833273e558f1d0a754d7752254e46f22f Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 14 Oct 2026 09:49:48 -0400 Subject: [PATCH 5/5] 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 bbc0fe86..61fb4c30 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 caf4c898..a6b7246e 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 {