fix(sdk): harden ZipReader central directory parsing against hostile fields (DSPX-5103)

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");
+            // 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
                     statement.value = value.getAsString();
@@ -1158,6 +1155,18 @@ public AssertionConfig.Statement deserialize(JsonElement json, Type typeOfT, Jso
             }
             return statement;
         }
+
+        /** 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()) {
+                return null;
+            }
+            if (!element.isJsonPrimitive()) {
+                throw new JsonParseException(String.format("assertion statement %s must be a string", member));
+            }
+            return element.getAsString();
+        }
     }
 
     public EncryptionInformation encryptionInformation;

sdk/src/main/java/io/opentdf/platform/sdk/SDK.java

@@ -176,6 +176,11 @@ public static boolean isTDF(SeekableByteChannel channel) {
             zipReader = new ZipReader(channel);
         } catch (IOException | InvalidZipException e) {
             return false;
+        } catch (RuntimeException e) {
+            // 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;
         }
         var entries = zipReader.getEntries();
         var names = entries.stream().map(ZipReader.Entry::getName).collect(Collectors.toSet());
@@ -191,14 +196,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 +392,10 @@ public static class MalformedTDFException extends SDKException {
         public MalformedTDFException(String errorMessage) {
             super(errorMessage);
         }
+
+        public MalformedTDFException(String errorMessage, Exception reason) {
+            super(errorMessage, reason);
+        }
     }
 
     /**

sdk/src/main/java/io/opentdf/platform/sdk/TDF.java

@@ -788,12 +788,8 @@ 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);
-        }
+        TDFReader tdfReader = TDFReader.open(tdf);
+        Manifest manifest = tdfReader.readManifest();
 
         byte[] payloadKey = new byte[GCM_KEY_SIZE];
         String unencryptedMetadata = null;

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,50 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException {
         payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData();
     }
 
+    /** Like the constructor, but reports a malformed container as {@link SDK.MalformedTDFException}. */
+    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 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);
+        }
+    }
+
+    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() {
+        // 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);
+        } 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,17 +111,17 @@ 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);
         }
         return totalRead;
     }
 
     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);
     }
 }

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);
     }
 
     /**
@@ -412,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 {
@@ -478,7 +490,55 @@ 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;
+
+    /** 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 "
+                    + headerStart + " is too short to hold the " + what + " its sentinel promises");
+        }
+    }
+
+    /** 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();
+        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;
+    }
+
+    /** 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());
+    }
+
+    /**
+     * 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) {
+        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");
@@ -501,48 +561,63 @@ 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);
         }
 
-        // Parse the extra field
-        for (final long startPos = zipChannel.position(); zipChannel.position() < startPos + extraFieldLength; ) {
-            long fieldStart = zipChannel.position();
+        long extraFieldStart = zipChannel.position();
+        long extraFieldEnd = extraFieldStart + extraFieldLength;
+        requireWithinCentralDirectory(extraFieldLength + " byte extra field", extraFieldEnd,
+                centralDirectoryEnd, headerStart);
+
+        // 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);
             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();
-                }
-                if (compressedSize == ZIP64_MAGICVAL) {
-                    compressedSize = readLong();
-                }
-                if (relativeOffsetOfLocalHeader == ZIP64_MAGICVAL) {
-                    relativeOffsetOfLocalHeader = readLong();
-                }
+                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);
                     diskNumberStart = readInt();
                 }
             }
-            // Skip other extra fields
-            zipChannel.position(fieldStart + dataSize + 4);
+            fieldStart = dataEnd;
         }
-
-        // 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) {
+        // fewer bytes than a record header may be left over; like other readers, ignore them
+
+        long fileCommentEnd = extraFieldEnd + fileCommentLength;
+        requireWithinCentralDirectory(fileCommentLength + " byte comment", fileCommentEnd,
+                centralDirectoryEnd, headerStart);
+        // 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)
-                    + "] 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);
     }
@@ -551,17 +626,17 @@ public ZipReader(SeekableByteChannel channel) throws IOException {
         zipChannel = channel;
         var centralDirectoryRecord = readEndOfCentralDirectory();
         seekWithinArchive("the central directory", centralDirectoryRecord.offsetToStart);
-        // 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;
+        long centralDirectoryEnd = centralDirectoryRecord.offsetToStart + centralDirectoryRecord.size;
+        // 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
-                    + " 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));
         }
     }