fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and defaulted segment sizes - #396
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and defaulted segment sizes#396dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
…ed 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.
dmihalcik-virtru
force-pushed
the
DSPX-4589-zip64-conformance
branch
from
September 3, 2026 20:12
9053c2c to
49689fe
Compare
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: https://virtru.atlassian.net/browse/DSPX-4589
Four zip64 / manifest conformance findings from the container audit. Findings 1-3 were written against pre-#393
main; each was re-verified against currentmain(98c839e2) before being changed, and the honest status of each is below.Finding 1 — end of central directory sentinel thresholds (fixed)
ZipWriter.finish()decided whether the archive needed a zip64 end of central directory record with:None of those masks match the real field widths. The entry count field is 2 bytes, not 1, and the central directory offset and size fields are 4 bytes each, not 2. The practical effect was that any archive with 256 or more entries was needlessly promoted to zip64, and the offset/size checks fired three orders of magnitude too early.
Now:
The offset and size go through
needsZip64, so they honor the same deliberate 2 GiB (Integer.MAX_VALUE) ceiling as the per-entry fields — released readers widen these unsigned wire fields with signed reads, so we never write a value that would come back negative. UsingneedsZip64also keeps theZipWriter(out, maxNonZip64Value)test seam that #393 added usable for these two fields.Not touched by #393 — verified with
jj diff -r 98c839e2.Finding 2 — truncated archive detection (fixed, but the branch was latent)
ZipReader.readEndOfCentralDirectory()scanned backwards for the EOCD signature and treated anullfromreadInteger()(a short read) as if it were a signature match, so a truncated archive could fall out of the loop and parse whatever bytes followed as an EOCD record. It now only breaks on a genuine match and throwsInvalidZipExceptionif the scan runs off the front, and separately rejects an archive too small to hold the zip64 locator its EOCD claims to have.Stated plainly: with the
SeekableInMemoryByteChannelthe SDK actually uses, the scan position is always<= size - 22, soreadInteger()always had four bytes available and thenullbranch was unreachable. This is a correctness/robustness fix (aFileChannelcould in principle short-read), not a bug anyone was hitting. The new tests still pass on unpatched code for the plain-truncation cases; they are there to lock the behavior in.Finding 3 — UTF-8 filename length (already correct on the wire; dead code removed)
The ticket says the central directory filename length was computed from
String.length(). That assignment did exist:but
CDFileHeader.write()never read the field — it wrote(short) filename.lengthfrom the already-encoded byte array. The bytes on the wire were already correct. No archive was ever mis-written.What changed: the misleading dead
filenameLengthfield is removed from bothLocalFileHeaderandCDFileHeader, the name is encoded to UTF-8 once instead of twice, and a newencodeFilenamehelper rejects a name whose encoded length exceeds0xFFFFwith anSDKExceptionrather than silently truncating it into the 2-byte field.Finding 4 — per-segment sizes not defaulted from the manifest defaults (fixed; this is the one with field impact)
integrityInformation.segments[].segmentSizeand.encryptedSegmentSizeare optional in the TDF spec; when absent, the reader is supposed to fall back tosegmentSizeDefault/encryptedSegmentSizeDefault. Gson left the absent primitives at0, and java-sdk read the payload with a zero-length segment. Every web SDK TDF larger than one default segment (1 MiB) failed to decrypt in java-sdk, surfacing as a confusing integrity error rather than as a manifest problem.A primitive
longcannot distinguish an absent JSON key from a literal0, so the fix consults the parse tree: a GsonTypeAdapterFactoryregistered forIntegrityInformationwalks the parsedsegmentsarray alongside the deserialized list and fills in the defaults only where the key is absent or JSONnull. BoxingSegment.segmentSizetoLongwould have been the other option, but it breaks the public API (==inSegment.equals, anint->Longassignment inTDF, existingassertEquals(Long, int)in tests) for no added behavior, so the post-deserialization fixup was chosen instead. Explicit0in the JSON is preserved as0.TDF.Reader.readPayloadadditionally rejects a segment with a non-positiveencryptedSegmentSizeup front — an encrypted segment always carries at least an IV and a tag — so a manifest that supplies neither a per-segment size nor a usable default now says so instead of failing downstream with an unrelated complaint about the payload being too small to GMAC.Tests
11 new tests, all in
sdk/src/test/java/io/opentdf/platform/sdk/:ZipWriterTest— one test per EOCD sentinel driver: entry count (0xFFFE non-zip64 vs 0xFFFF zip64, round-tripped throughZipReader), central directory offset, and central directory size, each isolated so only the EOCD is zip64 and no entry is; plusfilenameLengthIsMeasuredInUtf8Bytes("🔒両.txt",String.length()7 vs 11 UTF-8 bytes, asserted at both header offsets) andrejectsAnEntryNameTooLongToDescribe.ZipReaderTest—testTruncatedArchiveIsRejected(four truncation shapes) andtestArchiveTooShortForTheZip64LocatorIsRejected, both assertingInvalidZipException.ManifestTest—testAbsentSegmentSizesFallBackToTheManifestDefaults(absent / partially overridden / fully overridden, plus atoJsonround trip) andtestExplicitZeroSegmentSizeIsNotTreatedAsAbsent.TDFTest—testReadingATDFThatOmitsDefaultedSegmentSizesencrypts ~2 MiB + 4242 bytes at a 1 MiB segment size, strips every per-segment size equal to the default from the manifest, and asserts a byte-exact decrypt;testZeroLengthSegmentIsRejectedWithAClearError.The sentinel and default-fallback tests were confirmed to be genuine regression tests by reverting the EOCD mask fix and the
registerTypeAdapterFactoryline and watching them fail.CI-equivalent commands, both green on JDK 21:
The xtest cell
test_tdfs.py::test_chunky_roundtripcurrently SKIPS for java, becausextest/sdk/java/cli.shanswers no tosupports chunky. That shim lives in theopentdf/testsrepo, not in this one, so nothing in this PR changes it — the cell will keep skipping after merge and will not prove the finding 4 fix end to end.When this fix releases, someone must version-gate the
chunky)case inxtest/sdk/java/cli.shso java reports support at or above the releasing version. Deliberately not done here: this PR touches onlyopentdf/java-sdk, and a sibling ticket owns the tests repo.Draft because that harness gate is still open and the release version for the gate isn't known yet.