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];