fix(sdk): DSPX-4590 zip64 conformance - #3981
Draft
dmihalcik-virtru wants to merge 3 commits into
Draft
Conversation
Reader.ReadAt derived every segment's plaintext extent from manifest.DefaultSegmentSize, assuming a uniform stride. The TDF manifest records a per-segment Size and does not require the segments to be equal sized, and sdk/experimental/tdf already emits variable-length segments. Against such a payload the uniform stride selects the wrong segment (wrong bytes returned, no error) or slices past the decrypted buffer. Walk manifest.Segments accumulating seg.Size instead, and return ErrSegSizeMismatch when a segment's declared Size disagrees with what it actually decrypts to rather than panicking on the slice. WriteTo already walked cumulative sizes; the two now agree. SDK.CreateTDF only ever emits uniform segments, so this is not reachable through it -- the new sweep over segment sizes 1/2/7/62/64 is regression coverage and passes before and after. The non-uniform tests build the archive directly on internal/zipstream and fail before this change. Peeled out of the DSPX-2604 stack; it stands on its own. Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
|
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:
|
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. 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 a 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 a 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. Note on #3933 standalone: without this fix, #3933's cumulative-walk ReadAt uses seg.Size directly, so an omitted (0) per-segment size stalls the plaintext cursor and desyncs the ciphertext offset for every segment after it. Reading a web-sdk multi-segment file then fails with a misleading "tamper detected: failed integrity check on segment hash" instead of main's current (also broken, but at least consistent) "fail to create gmac signature". #3933 should not be merged or relied on standalone for real multi-segment interop until this lands on top of it. The zip64/ZIP64-conformance findings originally bundled with this change (findings 1-6 of the DSPX-4590 investigation) now live in a separate PR stacked on top of this one, since they are independent of the segment- size defaulting fixed here. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
go-sdk's zip layer disagrees with the other SDKs in a handful of places. This addresses findings 1-6 from the DSPX-4590 investigation (finding 7, per-segment size defaults, is the parent PR this one is stacked on). ## Finding 1 (interop): writer switched to ZIP64 at 4 GiB instead of 2 GiB `Finalize` compared against `^uint32(0)`, so a payload between 2 GiB and 4 GiB was 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`. - New `maxNonZip64Value = math.MaxInt32` in `zip_primitives.go`, mirroring java-sdk's `MAX_NON_ZIP64_VALUE`. - The rule is applied to the uncompressed size, the compressed size **and** `entry.Offset` (the local-header offset), which was previously not checked at all -- an archive under 2 GiB of payload could still place a later entry's header past the boundary. - The threshold is injectable: `Config.MaxNonZip64Value` plus a `WithMaxNonZip64Value` option (clamped to `(0, maxNonZip64Value]`, so it can only 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 `0xFFFFFFFF` sentinel. A container whose extra area leads with, say, an extended-timestamp field (tag `0x5455`) was misparsed. `parseZip64ExtraField` now 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 `NewReader` only looked at `CentralDirectoryOffset == 0xFFFFFFFF`. An archive that overflows the entry count (`0xFFFF`) or the central-directory size but not the offset was read as zip32. `eocdNeedsZip64` now checks all three EOCD sentinel 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 `nextCD` was advanced with uint16 arithmetic and did not include the file comment. 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. `checkFitsInCentralDirectory` now returns a new `ErrFieldOverflow` for 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.) ## Tests `sdk/internal/zipstream/zip64_conformance_test.go` -- hand-assembles raw zip archives (`buildRawZip`) so the reader can be pointed at containers no Go writer 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: `TestWriterSwitchesToZip64AtInjectedThreshold` uses the injected 1 KiB threshold, `TestEntryNeedsZip64AtTwoGiB` pins `maxNonZip64Value == math.MaxInt32` and covers size / compressed size / offset, `TestCentralDirectoryNarrowingGuard` covers finding 6. `sdk/internal/zipstream/fuzz_test.go` -- two new seeds for the `nextCD` overflow and the file-comment case. ## Follow-up required in opentdf/tests (NOT covered by this PR) > The xtest cell `test_tdfs.py::test_chunky_roundtrip` currently **SKIPS** for > go, because `xtest/sdk/go/cli.sh` answers no to `supports chunky`. That shim > lives in the `opentdf/tests` repo, so merging this PR does **not** flip it -- > the go column will stay skipped and the interop regression will stay > invisible in CI. When this fix ships in a release, someone needs to > version-gate the `chunky)` case in `xtest/sdk/go/cli.sh` so it reports > support at or above that version. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4590-zip64-conformance-only
branch
from
September 4, 2026 16:36
f57c73b to
01d6d43
Compare
Contributor
X-Test Failure Reportopentdf |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
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-4590
Part 3 of a 3-PR stack: #3933 (base) <- #3979 <- this PR.
Based on #3979 (default per-segment sizes when a writer omits them), which
is itself based on #3933. This PR carries findings 1-6 of the DSPX-4590
investigation (go-sdk's ZIP64/APPNOTE conformance issues); finding 7
(per-segment size defaults) is fixed in #3979 below it and is unrelated to
these changes -- the two were originally one PR (#3967) and are split here
so each can be reviewed independently.
go-sdk's zip layer disagrees with the other SDKs in a handful of places,
and this addresses all six.
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 checkedat all -- 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 thetests can 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 first 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.parseZip64ExtraFieldnow walksthe 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 butnot the 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.)
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, 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:
TestWriterSwitchesToZip64AtInjectedThresholduses the injected 1 KiBthreshold,
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.
Follow-up required in opentdf/tests (NOT covered by this PR)
Testing
cd sdk && go test ./... -racemake fmt,make lint(0 new issues)Supersedes the zip64 portion of #3967, which is left open, unmodified, for
reference.