fix(sdk): DSPX-4590 zip64 conformance and per-segment size defaults - #3967
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
fix(sdk): DSPX-4590 zip64 conformance and per-segment size defaults#3967dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
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 |
github-actions
Bot
added
comp:sdk
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
The zip writer only switched to ZIP64 once a value exceeded 4 GiB, but java-sdk reads the 32-bit central-directory fields as signed, so any container in the 2-4 GiB band was written as zip32 with a value that deployed Java clients read back as negative. Switch at 2 GiB (math.MaxInt32) to match java-sdk's MAX_NON_ZIP64_VALUE, apply the same rule to the local-header offset, and make the threshold injectable so the ZIP64 path can be exercised in a unit test without allocating gigabytes. The reader's ZIP64 extra-field parser assumed the field was first in the extra area and that all three values were always present. APPNOTE 4.5.3 says each value appears only when its central-directory counterpart holds the 0xFFFFFFFF sentinel, in the order original size, compressed size, local header offset. Walk the whole extra area, skip foreign tags, and read only the values the sentinels advertise. Also: detect ZIP64 from any of the three EOCD sentinel fields rather than the offset alone, honour a per-entry ZIP64 extra field in a zip32 EOCD archive, widen the central-directory cursor arithmetic to uint64 so a long name plus extra plus comment cannot wrap at uint16, include the file-comment length in that cursor, and return an explicit error instead of silently truncating when a value will not fit a 32-bit field. Finally, segmentSize and encryptedSegmentSize are optional per-segment overrides: manifest.schema.json requires only the integrityInformation defaults, and web-sdk omits the per-segment keys whenever they equal the default, so every web-sdk container over one segment failed to decrypt in go-sdk. Fall back to the manifest defaults in the payload-size computation, WriteTo and ReadAt, and reject a segment that resolves to zero rather than letting it slip past the read-length check and fail later inside the GMAC calculation.
dmihalcik-virtru
force-pushed
the
DSPX-4590-zip64-conformance
branch
from
September 3, 2026 20:12
2e371da to
01d583c
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
dmihalcik-virtru
added a commit
that referenced
this pull request
Sep 4, 2026
The zip writer only switched to ZIP64 once a value exceeded 4 GiB, but java-sdk reads the 32-bit central-directory fields as signed, so any container in the 2-4 GiB band was written as zip32 with a value that deployed Java clients read back as negative. Switch at 2 GiB (math.MaxInt32) to match java-sdk's MAX_NON_ZIP64_VALUE, apply the same rule to the local-header offset, and make the threshold injectable so the ZIP64 path can be exercised in a unit test without allocating gigabytes. The reader's ZIP64 extra-field parser assumed the field was first in the extra area and that all three values were always present. APPNOTE 4.5.3 says each value appears only when its central-directory counterpart holds the 0xFFFFFFFF sentinel, in the order original size, compressed size, local header offset. Walk the whole extra area, skip foreign tags, and read only the values the sentinels advertise. Also: detect ZIP64 from any of the three EOCD sentinel fields rather than the offset alone, honour a per-entry ZIP64 extra field in a zip32 EOCD archive, widen the central-directory cursor arithmetic to uint64 so a long name plus extra plus comment cannot wrap at uint16, include the file-comment length in that cursor, and return an explicit error instead of silently truncating when a value will not fit a 32-bit field. segmentSize and encryptedSegmentSize are optional per-segment overrides: manifest.schema.json requires only the integrityInformation defaults, and web-sdk omits the per-segment keys whenever they equal the default, so every web-sdk container over one segment failed to decrypt in go-sdk. Fall back to the manifest defaults in the payload-size computation, WriteTo and ReadAt. Rebased onto #3933 (map ReadAt plaintext offsets from cumulative segment sizes), which rewrote ReadAt's segment lookup from a uniform DefaultSegmentSize stride to a walk over each segment's actual plaintext/ciphertext size -- necessary for the non-uniform segments sdk/experimental/tdf can emit, and something this change's original ReadAt fix did not attempt. Reconciling the two surfaced a further bug: resolveSegmentSizes treated a per-segment size of 0 as "omitted, use the default" independently for Size and EncryptedSize, but JSON can't distinguish an omitted key from an explicit 0, and go-sdk's own CreateTDF already writes segmentSize: 0 for the sole segment of an empty-payload TDF (no omitempty on the field). That made an empty TDF round-trip to the wrong payloadSize. resolveSegmentSizes now resolves EncryptedSize first -- its zero value is never ambiguous, since ciphertext can never legitimately be zero bytes -- and disambiguates a zero Size by comparing the resolved EncryptedSize against DefaultEncryptedSegSize rather than assuming Size and EncryptedSize are only ever omitted together. Checking go-sdk, java-sdk and web-sdk's actual manifest-writing source confirmed go-sdk and java-sdk always set both fields together (so the joint-zero assumption happened to hold for them), but web-sdk's lib/tdf3/src/tdf.ts decides whether to omit segmentSize and encryptedSegmentSize with two independent equals-the-default comparisons, not one joint check -- so the joint-zero-only version would have mis-resolved a segment where only one of the two happened to be omitted. The corrected comparison needs no assumption about the cipher's per-segment overhead (nonce/tag size stays out of manifest.go entirely): the overhead is constant across every segment in one manifest, so if the resolved EncryptedSize equals its default, the plaintext size must too, regardless of what that overhead number actually is. Also gives calculateSignature's too-short-ciphertext-for-GMAC error (previously a bare, unclassified error) a proper ErrTampered-wrapped sentinel, consistent with the rest of this file's integrity failures. Verified against opentdf/tests' DSPX-4592-java-underflow branch (adds test_tdfs.py::test_chunky_roundtrip, a 5 MiB round-trip that forces a full-default-sized segment): with platform-ref and otdfctl-ref both pointed at this branch and XT_FORCE_SUPPORTS=chunky, js-encrypt -> go-decrypt passes (js omits per-segment sizes on the full-sized segment; go now defaults them back). The one remaining failure in that run, js-encrypt -> java-decrypt, is java-sdk's own pre-existing GMAC-on-empty- segment bug (DSPX-4589), unrelated to this change. Supersedes #3967, which is left open, unmodified, for reference. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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-4590
go-sdk's zip layer disagrees with the other SDKs in a handful of places, and its
manifest reader treats optional per-segment sizes as mandatory. This addresses
all seven findings from the DSPX-4590 investigation.
Finding 1 (interop): writer switched to ZIP64 at 4 GiB instead of 2 GiB
Finalizecompared against^uint32(0), so a payload between 2 GiB and 4 GiBwas written as a zip32 archive with a value in the top half of the unsigned
32-bit range. java-sdk widens those central-directory fields signed, so
deployed Java clients read the size/offset back as a negative number and cannot
open the container. web-sdk always writes ZIP64, java-sdk (since java-sdk#393)
switches at
Integer.MAX_VALUE.maxNonZip64Value = math.MaxInt32inzip_primitives.go, mirroringjava-sdk's
MAX_NON_ZIP64_VALUE.entry.Offset(the local-header offset), which was previously not checked atall — an archive under 2 GiB of payload could still place a later entry's
header past the boundary.
Config.MaxNonZip64Valueplus aWithMaxNonZip64Valueoption (clamped to(0, maxNonZip64Value], so it canonly ever make the writer more eager to use ZIP64). This lets the tests
drive the ZIP64 path with a 1 KiB threshold instead of allocating gigabytes.
Finding 2: ZIP64 extra field parsed positionally
The reader assumed the ZIP64 extended-information field was the first entry in
the extra-field area and that all three values were always present. Per APPNOTE
4.5.3 the values appear in the order original size, compressed size, local
header offset, and each is present only when its central-directory
counterpart holds the
0xFFFFFFFFsentinel. A container whose extra area leadswith, say, an extended-timestamp field (tag
0x5455) was misparsed.parseZip64ExtraFieldnow walks the whole extra area, skips foreign tags,reads values in spec order gated on the sentinels, and rejects a field that
claims to run past the end of the area.
Finding 3: ZIP64 detected from the CD offset alone
NewReaderonly looked atCentralDirectoryOffset == 0xFFFFFFFF. An archivethat overflows the entry count (
0xFFFF) or the central-directory size but notthe offset was read as zip32.
eocdNeedsZip64now checks all three EOCDsentinel fields.
Finding 4: per-entry ZIP64 extra field ignored without a ZIP64 EOCD
A writer may put a ZIP64 extra field on an individual entry while leaving the
EOCD in zip32 form. The reader now consults the extra field whenever the
central-directory header carries a sentinel, independent of the EOCD form.
Finding 5: central-directory cursor could wrap at uint16
nextCDwas advanced with uint16 arithmetic and did not include the filecomment. A 65000-byte filename plus a 600-byte extra field wraps to 110 and
the reader walks into the middle of a header. The advance is now done in
uint64 and includes
FileCommentLength.Finding 6: silent truncation when a value does not fit 32 bits
The zip32 paths narrowed with a bare cast.
checkFitsInCentralDirectorynowreturns a new
ErrFieldOverflowfor the compressed size, uncompressed size,local-header offset, central-directory size/offset and entry count instead of
writing a corrupt archive. (With finding 1 in place this is unreachable in
normal operation; it is a backstop against future callers.)
Finding 7 (correctness): per-segment sizes are optional, and go-sdk read them as zero
manifest.schema.jsonmarkssegmentSizeDefaultandencryptedSegmentSizeDefaultrequired onintegrityInformationbut declares norequired list on
segments/items— a writer may omit a per-segment sizewhenever it equals the default. web-sdk does exactly that for every full-sized
segment, so every web-sdk container larger than one segment (1 MiB) failed to
decrypt in go-sdk: the missing keys unmarshalled to 0.
IntegrityInformation.resolveSegmentSizesnow substitutes the manifest-leveldefaults, and is used in all three places that consumed the raw values
independently:
payloadSizeaccumulation inNewReader/LoadTDF(a wrong totaltruncates
Seekand theReadAtbounds check),WriteToloop,ReadAtloop, including the skip-ahead arithmetic.A segment that resolves to zero or negative is now rejected with a new
ErrSegSizeUnresolved. Previously thelen(readBuf) != seg.EncryptedSizeguard passed vacuously for a zero-length segment and the read failed several
frames later inside the GMAC calculation, with a message that said nothing
about the manifest.
ReadAtalso guardssegmentSizeDefault <= 0before usingit as a divisor.
Tests
sdk/internal/zipstream/zip64_conformance_test.go— hand-assembles raw ziparchives (
buildRawZip) so the reader can be pointed at containers no Gowriter would produce: extra field not first, differing compressed and
uncompressed sizes so the APPNOTE 4.5.3 ordering is actually asserted (a
fixture with equal sizes passes either way), per-entry ZIP64 under a zip32
EOCD, a central-directory file comment, the 65646-byte name+extra case that
wraps to 110 at uint16, ZIP64 implied by the entry count, and a malformed
extra field. Writer side:
TestWriterSwitchesToZip64AtInjectedThresholdusesthe injected 1 KiB threshold,
TestEntryNeedsZip64AtTwoGiBpinsmaxNonZip64Value == math.MaxInt32and covers size / compressed size /offset,
TestCentralDirectoryNarrowingGuardcovers finding 6.sdk/internal/zipstream/fuzz_test.go— two new seeds for thenextCDoverflow and the file-comment case.
sdk/tdf_segment_defaults_test.go— round-trips a 2 MiB payload through amanifest rewritten to omit the per-segment sizes (the root signature covers
the segment hashes, not the JSON encoding, so the rewritten container is
still internally consistent), asserting both the
WriteToand theReadAtpath plus
payloadSize. Verified by fault injection: with the fallbackremoved the test fails with
payloadSize4242 instead of 2101394 and with[tamper detected] tdf: fail to read payload from tdf, matching the symptomsin the ticket.
TestResolveSegmentSizescovers the resolution rules directly.Follow-up required in opentdf/tests (NOT covered by this PR)