From 49689fe4d7ef8c567a48e714e318994de66ad1da Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 3 Sep 2026 12:46:04 -0400 Subject: [PATCH] fix(sdk): DSPX-4589 zip64 end of central directory sentinels, truncated archive detection, and defaulted segment sizes Four conformance fixes found while auditing the TDF zip container against PKWARE APPNOTE.TXT and against manifests written by other SDKs. 1. ZipWriter only set the zip64 flag on the end of central directory record when the entry count exceeded 0xFF or the central directory offset/size exceeded 0xFFFF. Those masks do not match the field widths: the entry count is 2 bytes and the offset and size are 4 bytes each. Archives with between 256 and 65534 entries were needlessly promoted to zip64, and the offset/size checks now go through needsZip64 so they honor the same 2 GiB ceiling as the per-entry fields. 2. ZipReader treated a short read while scanning backwards for the end of central directory signature as a signature match, so a truncated archive could fall out of the scan loop and parse whatever followed as an end of central directory record. It now only breaks on a real match and throws InvalidZipException otherwise, and rejects an archive too small to hold the zip64 locator it claims to have. 3. ZipWriter computed the central directory filename length from String.length() rather than from the UTF-8 encoded byte count. The value was assigned to a field that write() never read, so the bytes on the wire were already correct, but the dead field is removed, the name is encoded once instead of twice, and a name too long for the 2 byte length field is now rejected instead of silently truncated. 4. Manifest deserialization did not apply segmentSizeDefault and encryptedSegmentSizeDefault to segments that omit their own sizes, which the spec allows and which the web SDK does for every segment. Every web SDK TDF larger than one segment failed to decrypt. A Gson TypeAdapterFactory on IntegrityInformation now fills the defaults in from the parse tree, which distinguishes an absent key from a literal 0 without boxing the public Segment fields, and TDF.Reader reports a segment with a non-positive encrypted size as an invalid TDF rather than failing later with an unrelated integrity complaint. --- .../io/opentdf/platform/sdk/Manifest.java | 95 +++++++++ .../java/io/opentdf/platform/sdk/TDF.java | 13 ++ .../io/opentdf/platform/sdk/ZipReader.java | 14 +- .../io/opentdf/platform/sdk/ZipWriter.java | 54 +++-- .../io/opentdf/platform/sdk/ManifestTest.java | 70 ++++++ .../java/io/opentdf/platform/sdk/TDFTest.java | 125 +++++++++++ .../opentdf/platform/sdk/ZipReaderTest.java | 49 +++++ .../opentdf/platform/sdk/ZipWriterTest.java | 201 ++++++++++++++++++ 8 files changed, 606 insertions(+), 15 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java index 20fbb13e..5f241fc0 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java @@ -6,10 +6,17 @@ import com.google.gson.JsonDeserializer; import com.google.gson.JsonElement; import com.google.gson.JsonParseException; +import com.google.gson.JsonArray; +import com.google.gson.JsonObject; import com.google.gson.JsonSerializationContext; import com.google.gson.JsonSerializer; +import com.google.gson.TypeAdapter; +import com.google.gson.TypeAdapterFactory; import com.google.gson.annotations.JsonAdapter; import com.google.gson.annotations.SerializedName; +import com.google.gson.reflect.TypeToken; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonWriter; import com.nimbusds.jose.JOSEException; import com.nimbusds.jose.JWSAlgorithm; import com.nimbusds.jose.JWSHeader; @@ -56,6 +63,7 @@ public class Manifest { private static final Gson gson = new GsonBuilder() .registerTypeAdapter(AssertionConfig.Statement.class, new AssertionValueAdapter()) + .registerTypeAdapterFactory(new IntegrityInformationAdapterFactory()) .create(); @SerializedName(value = "schemaVersion") String tdfVersion; @@ -98,7 +106,17 @@ public JsonElement serialize(Object src, Type typeOfSrc, JsonSerializationContex static public class Segment { public String hash; + /** + * The plaintext length of this segment. Optional in the JSON: when a producer leaves it + * out it means {@link IntegrityInformation#segmentSizeDefault}, which + * {@link IntegrityInformationAdapterFactory} fills in during deserialization. + */ public long segmentSize; + /** + * The on-the-wire length of this segment. Optional in the JSON the same way + * {@link #segmentSize} is, defaulting to + * {@link IntegrityInformation#encryptedSegmentSizeDefault}. + */ public long encryptedSegmentSize; @Override @@ -167,6 +185,83 @@ public int hashCode() { } } + /** + * Applies {@code segmentSizeDefault} / {@code encryptedSegmentSizeDefault} to any segment + * that left the corresponding per-segment key out of its JSON. + *

+ * The per-segment values are optional overrides: {@code manifest.schema.json} marks the two + * defaults required on {@code integrityInformation} but puts no {@code required} list on + * {@code segments/items}. web-sdk omits a per-segment size whenever it equals the default, + * which is every full segment of a payload larger than one segment. Gson leaves an absent + * key at {@code 0}, so before this the reader allocated a zero length buffer for those + * segments and failed inside the integrity check. + *

+ * This runs as a post-deserialization fixup rather than by boxing the fields to {@code Long}, + * which keeps {@link Segment#segmentSize} a primitive for callers, keeps the value/absence + * distinction out of the public API, and applies to every path that parses a manifest. A + * primitive alone cannot tell an absent key from a literal {@code 0}, so the decision is made + * against the parse tree rather than against the deserialized value. + */ + private static class IntegrityInformationAdapterFactory implements TypeAdapterFactory { + private static final String SEGMENTS = "segments"; + private static final String SEGMENT_SIZE = "segmentSize"; + private static final String ENCRYPTED_SEGMENT_SIZE = "encryptedSegmentSize"; + + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + if (!IntegrityInformation.class.equals(type.getRawType())) { + return null; + } + final TypeAdapter delegate = gson.getDelegateAdapter(this, type); + final TypeAdapter elementAdapter = gson.getAdapter(JsonElement.class); + return new TypeAdapter() { + @Override + public void write(JsonWriter out, T value) throws IOException { + delegate.write(out, value); + } + + @Override + public T read(JsonReader in) throws IOException { + JsonElement tree = elementAdapter.read(in); + T value = delegate.fromJsonTree(tree); + if (value instanceof IntegrityInformation && tree != null && tree.isJsonObject()) { + applySegmentSizeDefaults((IntegrityInformation) value, tree.getAsJsonObject()); + } + return value; + } + }; + } + + private static void applySegmentSizeDefaults(IntegrityInformation integrityInformation, JsonObject json) { + List segments = integrityInformation.segments; + JsonElement rawSegments = json.get(SEGMENTS); + if (segments == null || rawSegments == null || !rawSegments.isJsonArray()) { + return; + } + JsonArray rawSegmentArray = rawSegments.getAsJsonArray(); + int count = Math.min(segments.size(), rawSegmentArray.size()); + for (int i = 0; i < count; i++) { + Segment segment = segments.get(i); + JsonElement rawSegment = rawSegmentArray.get(i); + if (segment == null || rawSegment == null || !rawSegment.isJsonObject()) { + continue; + } + JsonObject rawSegmentObject = rawSegment.getAsJsonObject(); + if (!hasValue(rawSegmentObject, SEGMENT_SIZE)) { + segment.segmentSize = integrityInformation.segmentSizeDefault; + } + if (!hasValue(rawSegmentObject, ENCRYPTED_SEGMENT_SIZE)) { + segment.encryptedSegmentSize = integrityInformation.encryptedSegmentSizeDefault; + } + } + } + + private static boolean hasValue(JsonObject object, String memberName) { + JsonElement member = object.get(memberName); + return member != null && !member.isJsonNull(); + } + } + static public class PolicyBinding { public String alg; public String hash; 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 b54901b9..f18d1ce1 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDF.java @@ -421,6 +421,19 @@ public void readPayload(OutputStream outputStream) throws SDK.SegmentSignatureMi } for (Manifest.Segment segment : manifest.encryptionInformation.integrityInformation.segments) { + if (segment.encryptedSegmentSize <= 0) { + // an encrypted segment always carries at least an IV and a tag, so this only + // happens on a manifest that supplied neither a per-segment + // encryptedSegmentSize nor a usable encryptedSegmentSizeDefault. reported + // here rather than letting a zero length buffer reach the integrity check, + // where it surfaces as an unrelated complaint about the payload being too + // small to GMAC + throw new IllegalStateException("invalid TDF: segment has an encrypted size of " + + segment.encryptedSegmentSize + + ". the manifest supplied neither a per-segment encryptedSegmentSize" + + " nor a usable encryptedSegmentSizeDefault"); + } + if (segment.encryptedSegmentSize > Config.MAX_SEGMENT_SIZE) { throw new IllegalStateException("Segment size " + segment.encryptedSegmentSize + " exceeded limit " + Config.MAX_SEGMENT_SIZE); diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java index 576ba5de..aa8e1003 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipReader.java @@ -105,19 +105,25 @@ public CentralDirectoryRecord(long numEntries, long offsetToStart) { CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { long eoCDRStart = zipChannel.size() - END_OF_CENTRAL_DIRECTORY_SIZE; // 22 is the minimum size of the EOCDR + boolean found = false; while (eoCDRStart >= 0) { zipChannel.position(eoCDRStart); Integer signature = readInteger(); - if (signature == null || signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE) { + // a short read means there aren't four bytes here to compare against, which is not + // the same thing as having found the signature. treating it as a match let a + // truncated archive fall out of this loop and parse whatever followed as an end of + // central directory record + if (signature != null && signature == END_OF_CENTRAL_DIRECTORY_SIGNATURE) { if (logger.isDebugEnabled()) { logger.debug("Found end of central directory signature at {}", zipChannel.position() - Integer.BYTES); } + found = true; break; } eoCDRStart--; } - if (eoCDRStart < 0) { + if (!found) { throw new InvalidZipException("Didn't find the end of central directory"); } @@ -141,6 +147,10 @@ CentralDirectoryRecord readEndOfCentralDirectory() throws IOException { } long zip64CentralDirectoryLocatorStart = zipChannel.size() - (ZIP64_END_OF_CENTRAL_DIRECTORY_LOCATOR_SIZE + END_OF_CENTRAL_DIRECTORY_SIZE + commentLength); + if (zip64CentralDirectoryLocatorStart < 0) { + throw new InvalidZipException( + "Archive is too small to hold the zip64 end of central directory locator it claims to have"); + } zipChannel.position(zip64CentralDirectoryLocatorStart); return extractZIP64CentralDirectoryInfo(); } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java index 65357573..d8a836e4 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/ZipWriter.java @@ -30,6 +30,17 @@ public class ZipWriter { * {@link ZipReader}. */ static final long MAX_NON_ZIP64_VALUE = Integer.MAX_VALUE; + + /** + * The largest entry count we will write into the 2-byte end of central directory field. + * {@code 0xFFFF} itself is the sentinel that sends the real count to the zip64 end of central + * directory record, so it cannot be used as a literal value. + */ + private static final long MAX_NON_ZIP64_ENTRY_COUNT = 0xFFFE; + + /** The largest name we can describe in the 2-byte filename length field. */ + private static final int MAX_FILENAME_LENGTH = 0xFFFF; + private static final long ZIP_64_END_OF_CD_RECORD_SIZE = 56; private static final int ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE = 28; @@ -81,16 +92,30 @@ static void checkFitsInCentralDirectory(String name, long offset, long size) { } } + /** + * Encodes an entry name the way it is written to the archive. Zip filename lengths are + * counted in bytes, so anything derived from {@link String#length()} β€” which counts UTF-16 + * code units β€” desyncs the central directory for a non-ASCII name. + */ + private static byte[] encodeFilename(String name) { + var bytes = name.getBytes(StandardCharsets.UTF_8); + if (bytes.length > MAX_FILENAME_LENGTH) { + throw new SDKException("zip entry name is " + bytes.length + + " bytes when encoded as UTF-8, which does not fit in the " + + MAX_FILENAME_LENGTH + " byte filename length field"); + } + return bytes; + } + public OutputStream stream(String name) throws IOException { var startPosition = out.position; long fileTime, fileDate; fileTime = fileDate = getTimeDateUnMSDosFormat(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; localFileHeader.crc32 = 0; localFileHeader.generalPurposeBitFlag = (1 << 3) | (1 << 11); // we are using the data descriptor and we are using UTF-8 localFileHeader.compressedSize = ZIP_64_MAGIC_VAL; @@ -170,12 +195,12 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream checkFitsInCentralDirectory(fileInfo.filename, fileInfo.offset, fileInfo.size); } + var nameBytes = encodeFilename(fileInfo.filename); CDFileHeader cdFileHeader = new CDFileHeader(); cdFileHeader.generalPurposeBitFlag = fileInfo.flag; cdFileHeader.lastModifiedTime = fileInfo.fileTime; cdFileHeader.lastModifiedDate = fileInfo.fileDate; cdFileHeader.crc32 = (int) fileInfo.crc; - cdFileHeader.filenameLength = (short) fileInfo.filename.length(); cdFileHeader.extraFieldLength = 0; cdFileHeader.compressedSize = (int) fileInfo.size; cdFileHeader.uncompressedSize = (int) fileInfo.size; @@ -188,7 +213,7 @@ private static void writeCentralDirectoryHeader(FileInfo fileInfo, OutputStream cdFileHeader.extraFieldLength = ZIP_64_GLOBAL_EXTENDED_INFO_EXTRA_FIELD_SIZE; } - cdFileHeader.write(out, fileInfo.filename.getBytes(StandardCharsets.UTF_8)); + cdFileHeader.write(out, nameBytes); if (fileInfo.isZip64) { Zip64GlobalExtendedInfoExtraField zip64ExtendedInfoExtraField = new Zip64GlobalExtendedInfoExtraField(); @@ -208,18 +233,17 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o crc.update(data); var crcValue = crc.getValue(); - var nameBytes = name.getBytes(StandardCharsets.UTF_8); + var nameBytes = encodeFilename(name); LocalFileHeader localFileHeader = new LocalFileHeader(); localFileHeader.lastModifiedTime = (int) fileTime; localFileHeader.lastModifiedDate = (int) fileDate; - localFileHeader.filenameLength = (short) nameBytes.length; localFileHeader.generalPurposeBitFlag = 0; localFileHeader.crc32 = (int) crcValue; localFileHeader.compressedSize = data.length; localFileHeader.uncompressedSize = data.length; localFileHeader.extraFieldLength = 0; - localFileHeader.write(out, name.getBytes(StandardCharsets.UTF_8)); + localFileHeader.write(out, nameBytes); out.write(data); @@ -238,10 +262,14 @@ private FileInfo writeByteArray(String name, byte[] data, CountingOutputStream o private void writeEndOfCentralDirectory(boolean hasZip64Entry, long numEntries, long startOfCentralDirectory, long sizeOfCentralDirectory, CountingOutputStream out) throws IOException { + // each of these corresponds to a field in the end of central directory record that has to + // carry a sentinel β€” and send its real value to the zip64 record β€” once the value stops + // fitting. the entry count is 2 bytes; the offset and size are 4 bytes each, and are held + // to the same 2 GiB ceiling as the per-entry fields so that readers which widen them with + // a signed read never see a negative value. see MAX_NON_ZIP64_VALUE. var isZip64 = hasZip64Entry - || (numEntries & ~0xFF) != 0 - || (startOfCentralDirectory & ~0xFFFF) != 0 - || (sizeOfCentralDirectory & ~0xFFFF) != 0; + || numEntries > MAX_NON_ZIP64_ENTRY_COUNT + || needsZip64(startOfCentralDirectory, sizeOfCentralDirectory); if (isZip64) { var endPosition = out.position; @@ -322,7 +350,6 @@ private static class LocalFileHeader { int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength = 0; void write(OutputStream out, byte[] filename) throws IOException { @@ -337,7 +364,8 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); - buffer.putShort(filenameLength); + // the length of the encoded bytes, never String.length() + buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.put(filename); @@ -376,7 +404,6 @@ private static class CDFileHeader { int crc32; int compressedSize; int uncompressedSize; - short filenameLength; short extraFieldLength; final short fileCommentLength = 0; final short diskNumberStart = 0; @@ -397,6 +424,7 @@ void write(OutputStream out, byte[] filename) throws IOException { buffer.putInt(crc32); buffer.putInt(compressedSize); buffer.putInt(uncompressedSize); + // the length of the encoded bytes, never String.length() buffer.putShort((short) filename.length); buffer.putShort(extraFieldLength); buffer.putShort(fileCommentLength); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java index 220ca6d1..fcc720d6 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ManifestTest.java @@ -149,6 +149,76 @@ void testAssertionNull() { assertEquals(manifest.assertions.size(), 0); } + private static final long SEGMENT_SIZE_DEFAULT = 1048576; + private static final long ENCRYPTED_SEGMENT_SIZE_DEFAULT = 1048604; + + /** A minimal but valid manifest wrapped around whatever {@code segments} array you give it. */ + private static String manifestWithSegments(String segmentsJson) { + return "{\n" + + " \"encryptionInformation\": {\n" + + " \"integrityInformation\": {\n" + + " \"encryptedSegmentSizeDefault\": " + ENCRYPTED_SEGMENT_SIZE_DEFAULT + ",\n" + + " \"rootSignature\": { \"alg\": \"HS256\", \"sig\": \"c2ln\" },\n" + + " \"segmentHashAlg\": \"GMAC\",\n" + + " \"segmentSizeDefault\": " + SEGMENT_SIZE_DEFAULT + ",\n" + + " \"segments\": [" + segmentsJson + "]\n" + + " },\n" + + " \"keyAccess\": [ { \"protocol\": \"kas\", \"type\": \"wrapped\"," + + " \"url\": \"http://localhost:65432/kas\", \"wrappedKey\": \"a2V5\" } ],\n" + + " \"method\": { \"algorithm\": \"AES-256-GCM\", \"isStreamable\": true, \"iv\": \"aXY=\" },\n" + + " \"policy\": \"cG9saWN5\",\n" + + " \"type\": \"split\"\n" + + " },\n" + + " \"payload\": { \"isEncrypted\": true, \"protocol\": \"zip\"," + + " \"type\": \"reference\", \"url\": \"0.payload\" }\n" + + "}"; + } + + /** + * web-sdk leaves {@code segmentSize} and {@code encryptedSegmentSize} out of a segment + * whenever they equal the manifest level defaults. That is legal: {@code manifest.schema.json} + * marks the two defaults required on {@code integrityInformation} but puts no + * {@code required} list on {@code segments/items}, so the per-segment values are optional + * overrides and an absent one means "the default", not zero. + */ + @Test + void testAbsentSegmentSizesFallBackToTheManifestDefaults() { + Manifest manifest = Manifest.readManifest(manifestWithSegments( + "{ \"hash\": \"aGFzaDA=\" }," + + "{ \"hash\": \"aGFzaDE=\", \"segmentSize\": 12 }," + + "{ \"hash\": \"aGFzaDI=\", \"segmentSize\": 3, \"encryptedSegmentSize\": 31 }")); + + var segments = manifest.encryptionInformation.integrityInformation.segments; + assertThat(segments).hasSize(3); + + assertThat(segments.get(0).segmentSize).isEqualTo(SEGMENT_SIZE_DEFAULT); + assertThat(segments.get(0).encryptedSegmentSize).isEqualTo(ENCRYPTED_SEGMENT_SIZE_DEFAULT); + + assertThat(segments.get(1).segmentSize).isEqualTo(12); + assertThat(segments.get(1).encryptedSegmentSize).isEqualTo(ENCRYPTED_SEGMENT_SIZE_DEFAULT); + + assertThat(segments.get(2).segmentSize).isEqualTo(3); + assertThat(segments.get(2).encryptedSegmentSize).isEqualTo(31); + + // and the values we filled in survive a round trip through the serializer + assertEquals(manifest, Manifest.readManifest(Manifest.toJson(manifest))); + } + + /** + * An explicit zero is a value rather than an absent key, so it is left alone. The reader + * rejects it with its own error; silently rewriting it to the default would hide a corrupt + * manifest. + */ + @Test + void testExplicitZeroSegmentSizeIsNotTreatedAsAbsent() { + Manifest manifest = Manifest.readManifest(manifestWithSegments( + "{ \"hash\": \"aGFzaDA=\", \"segmentSize\": 0, \"encryptedSegmentSize\": 0 }")); + + var segment = manifest.encryptionInformation.integrityInformation.segments.get(0); + assertThat(segment.segmentSize).isZero(); + assertThat(segment.encryptedSegmentSize).isZero(); + } + @Test void testReadingManifestWithObjectStatementValue() throws IOException { final Manifest manifest; 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 3fbb3eac..172294c2 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java @@ -7,6 +7,7 @@ import com.nimbusds.jose.jwk.JWK; import com.google.gson.Gson; import com.google.gson.JsonObject; +import com.google.gson.JsonParser; import io.opentdf.platform.policy.KeyAccessServer; import io.opentdf.platform.policy.kasregistry.KeyAccessServerRegistryServiceClient; import io.opentdf.platform.policy.kasregistry.ListKeyAccessServersRequest; @@ -34,6 +35,7 @@ import java.util.Random; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.function.Consumer; import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -46,6 +48,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TDFTest { protected static KeyAccessServerRegistryServiceClient kasRegistryService; @@ -731,6 +734,128 @@ public void testCreatingTDFWithMultipleSegments() throws Exception { } + /** web-sdk's {@code DEFAULT_SEGMENT_SIZE}, the size at which its omissions start. */ + private static final int WEB_SDK_DEFAULT_SEGMENT_SIZE = 1024 * 1024; + + /** + * web-sdk drops {@code segmentSize} and {@code encryptedSegmentSize} from a segment whenever + * they equal the manifest level defaults, which is every full segment of a payload larger + * than one segment. Reproduces that encoding on a java-produced TDF rather than carrying a + * web-sdk fixture, so the test stays self-contained. Without the fallback the reader + * allocates a zero length buffer for those segments and fails inside the integrity check. + */ + @Test + public void testReadingATDFThatOmitsDefaultedSegmentSizes() throws Exception { + // two full segments and a partial one, the shape that first exposed this + var data = new byte[2 * WEB_SDK_DEFAULT_SEGMENT_SIZE + 4242]; + new Random(4589).nextBytes(data); + + var tdf = new TDF(new FakeServicesBuilder().setKas(kas) + .setKeyAccessServerRegistryService(kasRegistryService).build()); + var original = new ByteArrayOutputStream(); + var tdfObject = tdf.createTDF(new ByteArrayInputStream(data), original, + Config.newTDFConfig( + Config.withAutoconfigure(false), + Config.withKasInformation(getRSAKASInfos()), + Config.withSegmentSize(WEB_SDK_DEFAULT_SEGMENT_SIZE))); + assertThat(tdfObject.getManifest().encryptionInformation.integrityInformation.segments) + .withFailMessage("the test needs more than one segment to be meaningful") + .hasSizeGreaterThan(1); + + var rewritten = rewriteManifest(original.toByteArray(), manifest -> { + var integrityInformation = manifest.getAsJsonObject("encryptionInformation") + .getAsJsonObject("integrityInformation"); + var segmentSizeDefault = integrityInformation.get("segmentSizeDefault").getAsLong(); + var encryptedSegmentSizeDefault = integrityInformation.get("encryptedSegmentSizeDefault").getAsLong(); + + int omitted = 0; + for (var element : integrityInformation.getAsJsonArray("segments")) { + var segment = element.getAsJsonObject(); + if (segment.get("segmentSize").getAsLong() == segmentSizeDefault) { + segment.remove("segmentSize"); + omitted++; + } + if (segment.get("encryptedSegmentSize").getAsLong() == encryptedSegmentSizeDefault) { + segment.remove("encryptedSegmentSize"); + } + } + assertThat(omitted) + .withFailMessage("no segment matched the defaults, so nothing was omitted") + .isGreaterThan(0); + }); + + var unwrapped = new ByteArrayOutputStream(); + tdf.loadTDF(new SeekableInMemoryByteChannel(rewritten), platformUrl).readPayload(unwrapped); + + assertThat(unwrapped.toByteArray()) + .withFailMessage("extracted data does not match") + .containsExactly(data); + } + + /** + * An explicit zero is a corrupt manifest rather than an omitted default, and has to say so + * instead of reaching the integrity check and complaining that the payload is too small to + * GMAC. + */ + @Test + public void testZeroLengthSegmentIsRejectedWithAClearError() throws Exception { + var data = "some data to encrypt".getBytes(StandardCharsets.UTF_8); + var tdf = new TDF(new FakeServicesBuilder().setKas(kas) + .setKeyAccessServerRegistryService(kasRegistryService).build()); + var original = new ByteArrayOutputStream(); + tdf.createTDF(new ByteArrayInputStream(data), original, + Config.newTDFConfig( + Config.withAutoconfigure(false), + Config.withKasInformation(getRSAKASInfos()))); + + var rewritten = rewriteManifest(original.toByteArray(), manifest -> manifest + .getAsJsonObject("encryptionInformation") + .getAsJsonObject("integrityInformation") + .getAsJsonArray("segments") + .get(0).getAsJsonObject() + .addProperty("encryptedSegmentSize", 0)); + + var reader = tdf.loadTDF(new SeekableInMemoryByteChannel(rewritten), platformUrl); + assertThatThrownBy(() -> reader.readPayload(new ByteArrayOutputStream())) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("encrypted size of 0"); + } + + /** Rebuilds a TDF with its manifest edited in place, leaving the payload bytes untouched. */ + private static byte[] rewriteManifest(byte[] tdfBytes, Consumer edit) throws IOException { + final JsonObject manifest; + final byte[] payload; + try (var channel = new SeekableInMemoryByteChannel(tdfBytes)) { + var reader = new ZipReader(channel); + manifest = JsonParser + .parseString(readZipEntry(reader, TDFWriter.TDF_MANIFEST_FILE_NAME) + .toString(StandardCharsets.UTF_8)) + .getAsJsonObject(); + payload = readZipEntry(reader, TDFWriter.TDF_PAYLOAD_FILE_NAME).toByteArray(); + } + + edit.accept(manifest); + + var out = new ByteArrayOutputStream(); + var writer = new TDFWriter(out); + try (var payloadStream = writer.payload()) { + payloadStream.write(payload); + } + writer.appendManifest(manifest.toString()); + writer.finish(); + return out.toByteArray(); + } + + private static ByteArrayOutputStream readZipEntry(ZipReader reader, String name) throws IOException { + var entry = reader.getEntries().stream() + .filter(e -> e.getName().equals(name)) + .findFirst() + .orElseThrow(() -> new AssertionError("no entry named " + name)); + var data = new ByteArrayOutputStream(); + entry.getData().transferTo(data); + return data; + } + /** * The unsigned 96-bit big-endian encoding of {@code value}, for asserting on * expected IVs. diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java index fa2014bd..38466d59 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipReaderTest.java @@ -16,6 +16,7 @@ import java.nio.ByteOrder; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; import java.util.Random; @@ -23,6 +24,7 @@ import java.util.stream.IntStream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class ZipReaderTest { @@ -258,6 +260,53 @@ private static void assertReadsEveryEntry(byte[] archive) throws IOException { } } + /** + * Scanning for the end of central directory used to treat a short read the same as finding + * the signature, so a truncated archive fell out of the scan and parsed whatever followed as + * the record. Truncation has to surface as "this is not a zip", not as a confusing failure + * somewhere downstream. + */ + @Test + public void testTruncatedArchiveIsRejected() throws IOException { + var archive = zip64Archive(); + + // the trailing end of central directory record and its locator are gone + assertThatThrownBy(() -> readArchive( + Arrays.copyOf(archive, archive.length - (EOCD_SIZE + ZIP64_EOCD_LOCATOR_SIZE)))) + .isInstanceOf(InvalidZipException.class); + + // only the last few bytes of the record are gone + assertThatThrownBy(() -> readArchive(Arrays.copyOf(archive, archive.length - 4))) + .isInstanceOf(InvalidZipException.class); + + // cut down to less than a single end of central directory record + assertThatThrownBy(() -> readArchive(Arrays.copyOf(archive, EOCD_SIZE - 1))) + .isInstanceOf(InvalidZipException.class); + + assertThatThrownBy(() -> readArchive(new byte[0])) + .isInstanceOf(InvalidZipException.class); + } + + /** + * The end of central directory record claims a zip64 locator that the archive is too short to + * hold. Reaching for it has to be a zip error rather than an out of range seek. + */ + @Test + public void testArchiveTooShortForTheZip64LocatorIsRejected() throws IOException { + var archive = zip64Archive(); + // drop everything before the trailing records, leaving the end of central directory (which + // still carries its sentinels) with nothing in front of it + var truncated = Arrays.copyOfRange(archive, archive.length - EOCD_SIZE, archive.length); + + assertThatThrownBy(() -> readArchive(truncated)).isInstanceOf(InvalidZipException.class); + } + + private static void readArchive(byte[] archive) throws IOException { + try (var channel = new SeekableInMemoryByteChannel(archive)) { + new ZipReader(channel); + } + } + private static String readEntry(ZipReader reader, String name) throws IOException { var entry = reader.getEntries().stream() .filter(e -> e.getName().equals(name)) diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java index d1d287b2..13821751 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/ZipWriterTest.java @@ -15,6 +15,8 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; import java.nio.channels.FileChannel; import java.nio.channels.SeekableByteChannel; import java.nio.charset.StandardCharsets; @@ -157,6 +159,125 @@ public void rejectsAnOutOfRangeZip64Threshold() { assertThatThrownBy(() -> new ZipWriter(out, 1L << 32)).isInstanceOf(IllegalArgumentException.class); } + /** + * The entry count in the end of central directory record is a 2-byte field, and {@code 0xFFFF} + * in it is the sentinel that sends the real count to the zip64 record. An archive can need + * zip64 for its entry count alone while every one of its entries, and its whole central + * directory, stays comfortably inside 32 bits. + */ + @Test + public void entryCountAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + var justFits = archiveOfEmptyEntries(0xFFFE, ZipWriter.MAX_NON_ZIP64_VALUE); + assertThat(endOfCentralDirectory(justFits).totalEntries).isEqualTo(0xFFFE); + assertThat(containsZip64EndOfCentralDirectory(justFits)) + .withFailMessage("an archive whose entry count still fits should not be zip64") + .isFalse(); + + var overflows = archiveOfEmptyEntries(0xFFFF, ZipWriter.MAX_NON_ZIP64_VALUE); + var eocd = endOfCentralDirectory(overflows); + assertThat(eocd.totalEntries).isEqualTo(0xFFFF); + assertThat(eocd.entriesOnThisDisk).isEqualTo(0xFFFF); + assertThat(containsZip64EndOfCentralDirectory(overflows)) + .withFailMessage("the entry count no longer fits, so the archive has to be zip64") + .isTrue(); + + // and the real count survives, which it only can if it went into the zip64 record + try (var chan = new SeekableInMemoryByteChannel(overflows)) { + assertThat(new ZipReader(chan).getEntries().size()).isEqualTo(0xFFFF); + } + } + + /** + * The central directory offset is a 4-byte field. Held to the same 2 GiB ceiling as the + * per-entry fields, so the lowered threshold drives it here. + */ + @Test + public void centralDirectoryOffsetAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // one entry, small enough to stay non-zip64 itself, whose data pushes the start of the + // central directory past the threshold while the directory stays under it + var belowThreshold = archiveOfOneEntry(50, 100); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + var archive = archiveOfOneEntry(100, 100); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.offsetOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory past the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "big.bin"); + } + + /** + * The central directory size is also a 4-byte field. An empty entry costs 46 bytes in the + * directory but only 30 before it, so a pile of them makes the directory outgrow its own + * offset and this sentinel fires while the offset one does not. + */ + @Test + public void centralDirectorySizeAloneDrivesTheEndOfCentralDirectorySentinel() throws IOException { + // 5 entries: the directory starts at 180 and is 260 bytes long, both under the threshold + var belowThreshold = archiveOfEmptyEntries(5, 400); + assertThat(containsZip64EndOfCentralDirectory(belowThreshold)) + .withFailMessage("nothing here crosses the threshold, so the archive should not be zip64") + .isFalse(); + + // 10 entries: the directory still starts at 360 but is now 520 bytes long + var archive = archiveOfEmptyEntries(10, 400); + var eocd = endOfCentralDirectory(archive); + assertThat(eocd.sizeOfCentralDirectory).isEqualTo(ZIP64_SENTINEL); + assertThat(containsZip64EndOfCentralDirectory(archive)) + .withFailMessage("a central directory larger than the threshold has to be zip64") + .isTrue(); + + assertOnlyTheEndOfCentralDirectoryIsZip64(archive, "e00000"); + } + + /** + * Zip counts filename lengths in bytes. Deriving one from {@link String#length()}, which + * counts UTF-16 code units, desyncs the central directory by the difference for any entry + * name that isn't pure ASCII. + */ + @Test + public void filenameLengthIsMeasuredInUtf8Bytes() throws IOException { + // a surrogate pair (2 code units, 4 bytes) and a BMP character (1 code unit, 3 bytes), + // so String.length() is 7 where the encoded form is 11 bytes + var name = "πŸ”’δΈ‘.txt"; + var nameBytes = name.getBytes(StandardCharsets.UTF_8); + assertThat(name.length()).isNotEqualTo(nameBytes.length); + + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + writer.data(name, "contents".getBytes(StandardCharsets.UTF_8)); + writer.finish(); + var archive = out.toByteArray(); + + // the sole local file header starts at 0, and its filename length is at offset 26 + assertThat(readUnsignedShort(archive, 26)).isEqualTo(nameBytes.length); + // the central directory file header keeps its filename length at offset 28 + var centralDirectory = (int) endOfCentralDirectory(archive).offsetOfCentralDirectory; + assertThat(readUnsignedShort(archive, centralDirectory + 28)).isEqualTo(nameBytes.length); + + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(readEntry(new ZipReader(chan), name)).isEqualTo("contents"); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(getDataStream(z, z.getEntry(name)).toString(StandardCharsets.UTF_8)) + .isEqualTo("contents"); + } + } + + @Test + public void rejectsAnEntryNameTooLongToDescribe() { + var name = "δΈ‘".repeat(30_000); // 3 bytes each, so well past the 0xFFFF byte field + var writer = new ZipWriter(new ByteArrayOutputStream()); + assertThatThrownBy(() -> writer.data(name, new byte[0])) + .isInstanceOf(SDKException.class) + .hasMessageContaining("filename length field"); + } + @Test @Disabled("this takes a long time and shouldn't run on build machines") public void testWritingLargeFile() throws IOException { @@ -282,6 +403,86 @@ private static boolean containsZip64EndOfCentralDirectory(byte[] archive) { return false; } + private static final long ZIP64_SENTINEL = 0xFFFFFFFFL; + private static final int END_OF_CENTRAL_DIRECTORY_SIZE = 22; + private static final int END_OF_CENTRAL_DIRECTORY_SIGNATURE = 0x06054b50; + + /** An archive of {@code count} empty entries, whose names are all the same length. */ + private static byte[] archiveOfEmptyEntries(int count, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + var empty = new byte[0]; + for (int i = 0; i < count; i++) { + writer.data(String.format("e%05d", i), empty); + } + writer.finish(); + return out.toByteArray(); + } + + /** An archive of one entry named {@code big.bin} carrying {@code dataSize} bytes. */ + private static byte[] archiveOfOneEntry(int dataSize, long threshold) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out, threshold); + writer.data("big.bin", new byte[dataSize]); + writer.finish(); + return out.toByteArray(); + } + + /** + * Shows that it was an end of central directory field, and not an entry, that made the + * archive zip64: no entry carries a zip64 extra field, and the whole thing still reads. + */ + private static void assertOnlyTheEndOfCentralDirectoryIsZip64(byte[] archive, String anEntryName) + throws IOException { + try (var chan = new SeekableInMemoryByteChannel(archive)) { + ZipFile z = new ZipFile.Builder().setSeekableByteChannel(chan).get(); + assertThat(zip64ExtraField(z, anEntryName)) + .withFailMessage("no entry should be zip64 here, only the end of central directory") + .isNull(); + } + try (var chan = new SeekableInMemoryByteChannel(archive)) { + assertThat(new ZipReader(chan).getEntries().isEmpty()) + .withFailMessage("the archive should still be readable") + .isFalse(); + } + } + + private static int readUnsignedShort(byte[] archive, int position) { + return ByteBuffer.wrap(archive).order(ByteOrder.LITTLE_ENDIAN).getShort(position) & 0xFFFF; + } + + /** The fields of the trailing end of central directory record. We never write a comment. */ + private static final class EndOfCentralDirectory { + final int entriesOnThisDisk; + final int totalEntries; + final long sizeOfCentralDirectory; + final long offsetOfCentralDirectory; + + EndOfCentralDirectory(int entriesOnThisDisk, int totalEntries, long sizeOfCentralDirectory, + long offsetOfCentralDirectory) { + this.entriesOnThisDisk = entriesOnThisDisk; + this.totalEntries = totalEntries; + this.sizeOfCentralDirectory = sizeOfCentralDirectory; + this.offsetOfCentralDirectory = offsetOfCentralDirectory; + } + } + + private static EndOfCentralDirectory endOfCentralDirectory(byte[] archive) { + var buf = ByteBuffer + .wrap(archive, archive.length - END_OF_CENTRAL_DIRECTORY_SIZE, END_OF_CENTRAL_DIRECTORY_SIZE) + .order(ByteOrder.LITTLE_ENDIAN); + assertThat(buf.getInt()) + .withFailMessage("the archive doesn't end in an end of central directory record") + .isEqualTo(END_OF_CENTRAL_DIRECTORY_SIGNATURE); + buf.getShort(); // disk number + buf.getShort(); // disk the central directory starts on + return new EndOfCentralDirectory( + buf.getShort() & 0xFFFF, + buf.getShort() & 0xFFFF, + buf.getInt() & 0xFFFFFFFFL, + buf.getInt() & 0xFFFFFFFFL); + } + private static long crcOfWholeFile(File file) throws IOException { var crc = new CRC32(); var buf = new byte[1 << 16];