From 2e0d5ecab4ff4e0ccd8eae9a27a4829f95292d89 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 3 Sep 2026 12:46:21 -0400 Subject: [PATCH] fix(sdk): DSPX-4590 zip64 conformance 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 --- sdk/internal/zipstream/fuzz_test.go | 19 + sdk/internal/zipstream/reader.go | 208 ++++++--- sdk/internal/zipstream/segment_writer.go | 19 +- sdk/internal/zipstream/writer.go | 33 +- .../zipstream/zip64_conformance_test.go | 423 ++++++++++++++++++ sdk/internal/zipstream/zip_headers.go | 1 + sdk/internal/zipstream/zip_primitives.go | 90 +++- 7 files changed, 728 insertions(+), 65 deletions(-) create mode 100644 sdk/internal/zipstream/zip64_conformance_test.go diff --git a/sdk/internal/zipstream/fuzz_test.go b/sdk/internal/zipstream/fuzz_test.go index fc362bbdeb..3f3d1d131e 100644 --- a/sdk/internal/zipstream/fuzz_test.go +++ b/sdk/internal/zipstream/fuzz_test.go @@ -5,6 +5,7 @@ package zipstream import ( "bytes" "encoding/base64" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -130,6 +131,24 @@ func FuzzReader(f *testing.F) { "AAAAAAAAAwLnBheWxvYWRQSwECLQAtAAgAAAB9dS8xAuiuLAIE///tBQAADwAAAAAAAAAAA" + "AAAAABWAAAAMC5tYW5pZmVzdC5qc29uUEsFBgAAAAACAAIAdAAAAJUFAAAAAA==")) + // A central directory file comment: the entry after it only parses if + // nextCD accounts for FileCommentLength. + f.Add(buildRawZip(f, []rawZipEntry{ + {name: "0.payload", data: []byte("payload bytes"), comment: "central directory file comment"}, + {name: "0.manifest.json", data: []byte(`{"m":1}`)}, + }, false)) + + // Filename plus extra field plus header size sums to 65646, which wraps + // to 110 if the addition happens at uint16 width. + f.Add(buildRawZip(f, []rawZipEntry{ + { + name: strings.Repeat("n", 65000), + data: []byte("payload bytes"), + extraPrefix: bytes.Repeat([]byte{0}, 600), + }, + {name: "0.manifest.json", data: []byte(`{"m":1}`)}, + }, false)) + f.Fuzz(func(t *testing.T, data []byte) { reader, err := NewReader(bytes.NewReader(data)) if err != nil { diff --git a/sdk/internal/zipstream/reader.go b/sdk/internal/zipstream/reader.go index cd2e04b815..e988681c4f 100644 --- a/sdk/internal/zipstream/reader.go +++ b/sdk/internal/zipstream/reader.go @@ -72,15 +72,19 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) { } // check if zip is zip64 or zip32 format + // + // Any of the three EOCD fields that ZIP64 can overflow may carry the + // sentinel independently: the entry count (two bytes wide, so its + // sentinel is 0xFFFF), the central directory size, and the central + // directory offset. An archive with more than 65534 entries needs ZIP64 + // for the count alone while its central directory still starts below + // 4 GiB, so keying off the offset by itself misses it. var entryCount uint64 var centralDirectoryStart uint64 - isZip64 := false - if endOfCDRecord.CentralDirectoryOffset != zip64MagicVal { //nolint:nestif // pkzip is complicated + if !eocdNeedsZip64(endOfCDRecord) { //nolint:nestif // pkzip is complicated entryCount = uint64(endOfCDRecord.NumberOfCDRecordEntries) centralDirectoryStart = uint64(endOfCDRecord.CentralDirectoryOffset) } else { - isZip64 = true - // read zip64 end of central directory locator _, err := readSeeker.Seek(-(endOfCDRecordSize + zip64EndOfCDRecordLocatorSize), io.SeekEnd) if err != nil { @@ -144,52 +148,10 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) { return reader, fmt.Errorf("binary.Read failed: %w", err) } - offset := uint64(cdFileHeader.LocalHeaderOffset) - bytesToRead := uint64(cdFileHeader.CompressedSize) - - if isZip64 { //nolint:nestif // pkzip is complicated - // read Zip64 Extended Information extra field id - headerTag := uint16(0) - err = binary.Read(readSeeker, binary.LittleEndian, &headerTag) - if err != nil { - return reader, fmt.Errorf("binary.Read failed: %w", err) - } - - // read Zip64 Extended Information Extra Field Block Size - blockSize := uint16(0) - err = binary.Read(readSeeker, binary.LittleEndian, &blockSize) - if err != nil { - return reader, fmt.Errorf("binary.Read failed: %w", err) - } - - if headerTag == zip64ExternalID { - if cdFileHeader.CompressedSize == zip64MagicVal { - compressedSize := uint64(0) - err = binary.Read(readSeeker, binary.LittleEndian, &compressedSize) - if err != nil { - return reader, fmt.Errorf("binary.Read failed: %w", err) - } - - bytesToRead = compressedSize - } - - if cdFileHeader.UncompressedSize == zip64MagicVal { - uncompressedSize := uint64(0) - err = binary.Read(readSeeker, binary.LittleEndian, &uncompressedSize) - if err != nil { - return reader, fmt.Errorf("binary.Read failed: %w", err) - } - } - - if cdFileHeader.LocalHeaderOffset == zip64MagicVal { - localHeaderOffset := uint64(0) - err = binary.Read(readSeeker, binary.LittleEndian, &localHeaderOffset) - if err != nil { - return reader, fmt.Errorf("binary.Read failed: %w", err) - } - offset = localHeaderOffset - } - } + // readSeeker is now positioned at this entry's extra-field area. + offset, bytesToRead, err := resolveEntryLocation(readSeeker, cdFileHeader) + if err != nil { + return reader, err } // Read each file @@ -214,12 +176,156 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) { reader.fileEntries[string(fileNameByteArray)] = zipFileEntry - nextCD += uint64(cdFileHeader.ExtraFieldLength + cdFileHeader.FilenameLength + cdFileHeaderSize) + // Widen every term before summing: all three header lengths are + // uint16, so adding them at their declared width wraps at 65536 and + // lands the next seek inside the current entry. The file comment is + // part of the record too -- omitting it desyncs every entry after + // the first one that carries a comment. + nextCD += uint64(cdFileHeaderSize) + + uint64(cdFileHeader.FilenameLength) + + uint64(cdFileHeader.ExtraFieldLength) + + uint64(cdFileHeader.FileCommentLength) } return reader, nil } +// resolveEntryLocation returns the local header offset and the number of +// stored bytes for a central directory entry, reading the ZIP64 extended +// information extra field when the 32-bit fields carry the sentinel. +// +// That field is a property of the entry, not of the archive: APPNOTE permits +// one on an entry whose EOCD is not ZIP64, so the lookup is driven off this +// entry's own sentinel values rather than an archive-wide flag. The reader +// must be positioned at the start of the entry's extra-field area, and the +// area is only consumed when the entry declares one -- reading +// unconditionally would eat the bytes of whatever record follows. +func resolveEntryLocation(readSeeker io.Reader, cdFileHeader CDFileHeader) (uint64, uint64, error) { + offset := uint64(cdFileHeader.LocalHeaderOffset) + bytesToRead := uint64(cdFileHeader.CompressedSize) + + if cdFileHeader.ExtraFieldLength == 0 || !cdHeaderHasZip64Sentinel(cdFileHeader) { + return offset, bytesToRead, nil + } + + extraFields := make([]byte, cdFileHeader.ExtraFieldLength) + if _, err := io.ReadFull(readSeeker, extraFields); err != nil { + return 0, 0, fmt.Errorf("io.ReadFull failed: %w", err) + } + + zip64, err := parseZip64ExtraField(extraFields, cdFileHeader) + if err != nil { + return 0, 0, err + } + + if zip64.found { + if cdFileHeader.CompressedSize == zip64MagicVal { + bytesToRead = zip64.compressedSize + } + if cdFileHeader.LocalHeaderOffset == zip64MagicVal { + offset = zip64.localHeaderOffset + } + } + + return offset, bytesToRead, nil +} + +// eocdNeedsZip64 reports whether the end of central directory record defers +// any of its fields to the ZIP64 end of central directory record. Note the +// entry count is two bytes wide, so it uses a 16-bit sentinel. +func eocdNeedsZip64(eocd EndOfCDRecord) bool { + return eocd.CentralDirectoryOffset == zip64MagicVal || + eocd.SizeOfCentralDirectory == zip64MagicVal || + eocd.NumberOfCDRecordEntries == zip64MagicVal16 +} + +// cdHeaderHasZip64Sentinel reports whether any central directory field of +// this entry defers its value to a ZIP64 extended information extra field. +func cdHeaderHasZip64Sentinel(h CDFileHeader) bool { + return h.CompressedSize == zip64MagicVal || + h.UncompressedSize == zip64MagicVal || + h.LocalHeaderOffset == zip64MagicVal +} + +// zip64ExtraValues holds the values a ZIP64 Extended Information extra +// field supplies for one central directory entry. Only the fields whose +// central directory counterpart carried the sentinel are populated. +type zip64ExtraValues struct { + found bool + compressedSize uint64 + localHeaderOffset uint64 +} + +// parseZip64ExtraField walks the whole extra-field area of a central +// directory entry looking for the ZIP64 Extended Information field +// (0x0001). The field is not required to come first -- a Unix timestamp or +// NTFS field frequently precedes it -- so the area has to be iterated +// rather than probed at its head. +// +// Within the field the values appear in APPNOTE 4.5.3 order: original +// (uncompressed) size, compressed size, then local header offset. Each is +// present only when the matching central directory field holds the +// 0xFFFFFFFF sentinel, so the uncompressed size has to be stepped over even +// though nothing here consumes it -- reading the compressed size first +// would hand back the wrong value for any entry where the two differ. +func parseZip64ExtraField(extraFields []byte, h CDFileHeader) (zip64ExtraValues, error) { + var values zip64ExtraValues + + for pos := 0; pos+extraFieldHeaderSize <= len(extraFields); { + tag := binary.LittleEndian.Uint16(extraFields[pos:]) + size := int(binary.LittleEndian.Uint16(extraFields[pos+2:])) + pos += extraFieldHeaderSize + + if size > len(extraFields)-pos { + // A field claiming to run past the end of the area is + // malformed; there is nothing sane to resync to. + return values, errZipFormat + } + + if tag != zip64ExternalID { + pos += size + continue + } + + body := extraFields[pos : pos+size] + bodyPos := 0 + read := func() (uint64, bool) { + const uint64Size = 8 + if bodyPos+uint64Size > len(body) { + return 0, false + } + v := binary.LittleEndian.Uint64(body[bodyPos:]) + bodyPos += uint64Size + return v, true + } + + if h.UncompressedSize == zip64MagicVal { + if _, ok := read(); !ok { + return values, errZipFormat + } + } + if h.CompressedSize == zip64MagicVal { + v, ok := read() + if !ok { + return values, errZipFormat + } + values.compressedSize = v + } + if h.LocalHeaderOffset == zip64MagicVal { + v, ok := read() + if !ok { + return values, errZipFormat + } + values.localHeaderOffset = v + } + + values.found = true + return values, nil + } + + return values, nil +} + // ReadFileData Read data from file of given length of size. func (reader Reader) ReadFileData(filename string, index int64, length int64) ([]byte, error) { fileNameEntry, ok := reader.fileEntries[filename] diff --git a/sdk/internal/zipstream/segment_writer.go b/sdk/internal/zipstream/segment_writer.go index 18b35b62ce..34b3da57ac 100644 --- a/sdk/internal/zipstream/segment_writer.go +++ b/sdk/internal/zipstream/segment_writer.go @@ -33,10 +33,13 @@ func NewSegmentTDFWriter(expectedSegments int, opts ...Option) SegmentWriter { base := newBaseWriter(cfg) + centralDir := NewCentralDirectory() + centralDir.MaxNonZip64Value = cfg.MaxNonZip64Value + return &segmentWriter{ baseWriter: base, metadata: NewSegmentMetadata(expectedSegments, cfg.Now), - centralDir: NewCentralDirectory(), + centralDir: centralDir, payloadEntry: &FileEntry{ Name: TDFPayloadFileName, Offset: 0, @@ -191,11 +194,12 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, // Total payload size = header + all data (no data descriptor in this calculation) totalPayloadSize := headerSize + sw.payloadEntry.CompressedSize - // Decide whether payload descriptor must be ZIP64 - const max32 = ^uint32(0) + // Decide whether payload descriptor must be ZIP64. The switch point is + // 2 GiB rather than 4 GiB: see maxNonZip64Value. + maxNonZip64 := sw.config.MaxNonZip64Value needZip64ForPayload := sw.config.Zip64 == Zip64Always || - sw.payloadEntry.Size > uint64(max32) || - sw.payloadEntry.CompressedSize > uint64(max32) + sw.payloadEntry.Size > maxNonZip64 || + sw.payloadEntry.CompressedSize > maxNonZip64 // 1. Write data descriptor for payload (fail if Zip64Never but required) if sw.config.Zip64 == Zip64Never && needZip64ForPayload { @@ -230,7 +234,10 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, // 5. Write central directory sw.centralDir.Offset = totalPayloadSize + uint64(buffer.Len()) // Decide if ZIP64 is needed for central directory/EOCD based on offset or forced mode - needZip64ForCD := needZip64ForPayload || sw.config.Zip64 == Zip64Always || sw.centralDir.Offset > uint64(max32) || len(sw.centralDir.Entries) > int(^uint16(0)) + needZip64ForCD := needZip64ForPayload || + sw.config.Zip64 == Zip64Always || + sw.centralDir.Offset > maxNonZip64 || + len(sw.centralDir.Entries) >= zip64MagicVal16 if sw.config.Zip64 == Zip64Never && needZip64ForCD { return nil, &Error{Op: "finalize", Type: "segment", Err: ErrZip64Required} } diff --git a/sdk/internal/zipstream/writer.go b/sdk/internal/zipstream/writer.go index 774e1c9488..3763fedc91 100644 --- a/sdk/internal/zipstream/writer.go +++ b/sdk/internal/zipstream/writer.go @@ -68,6 +68,7 @@ var ( ErrNoSegmentZero = errors.New("segment 0 missing; it carries the payload local file header") ErrInvalidSize = errors.New("invalid size") ErrZip64Required = errors.New("ZIP64 required but disabled (Zip64Never)") + ErrFieldOverflow = errors.New("value too large for zip field") ) // Config holds configuration options for writers @@ -79,6 +80,10 @@ type Config struct { // Defaults to time.Now; tests inject a pinned clock for // deterministic ZIP output. Now func() time.Time + // MaxNonZip64Value is the largest size or offset written into a 32-bit + // zip field before the archive switches to ZIP64. Zero means + // maxNonZip64Value (2 GiB - 1). See WithMaxNonZip64Value. + MaxNonZip64Value uint64 } // Option is a functional option for configuring writers @@ -130,13 +135,27 @@ func WithClock(now func() time.Time) Option { } } +// WithMaxNonZip64Value lowers the point at which the writer switches to +// ZIP64. This exists as a test seam -- mirroring java-sdk's injectable +// MAX_NON_ZIP64_VALUE -- so the ZIP64 path can be exercised without +// materializing a 2 GiB payload. Production callers should leave it alone; +// zero or a value above the default is ignored. +func WithMaxNonZip64Value(maxValue uint64) Option { + return func(c *Config) { + if maxValue > 0 && maxValue <= maxNonZip64Value { + c.MaxNonZip64Value = maxValue + } + } +} + // defaultConfig returns default configuration func defaultConfig() *Config { return &Config{ - Zip64: Zip64Auto, - MaxSegments: defaultMaxSegments, - EnableLogging: false, - Now: time.Now, + Zip64: Zip64Auto, + MaxSegments: defaultMaxSegments, + EnableLogging: false, + Now: time.Now, + MaxNonZip64Value: maxNonZip64Value, } } @@ -154,6 +173,12 @@ func applyOptions(opts []Option) *Config { if cfg.Now == nil { cfg.Now = time.Now } + // Same defence for the ZIP64 switch point: an option is free to zero + // the exported field, and zero would mean "switch to ZIP64 for + // everything" to the comparisons in Finalize. + if cfg.MaxNonZip64Value == 0 { + cfg.MaxNonZip64Value = maxNonZip64Value + } return cfg } diff --git a/sdk/internal/zipstream/zip64_conformance_test.go b/sdk/internal/zipstream/zip64_conformance_test.go new file mode 100644 index 0000000000..6802ee57ff --- /dev/null +++ b/sdk/internal/zipstream/zip64_conformance_test.go @@ -0,0 +1,423 @@ +// Experimental: This package is EXPERIMENTAL and may change or be removed at any time + +package zipstream + +import ( + "archive/zip" + "bytes" + "encoding/binary" + "hash/crc32" + "math" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// rawZipEntry describes one entry for buildRawZip. The builder deliberately +// bypasses the writers in this package: these fixtures cover archives our own +// writers never emit (differing compressed/uncompressed sizes, file comments, +// extra fields ahead of the ZIP64 one), which is exactly where the reader's +// conformance gaps live. +type rawZipEntry struct { + name string + // data is the stored (compressed) content. + data []byte + // uncompressedSize overrides the declared original size. Zero means + // len(data), i.e. a normal STORED entry. + uncompressedSize uint64 + // comment is the central directory file comment. + comment string + // extraPrefix is written into the extra-field area ahead of the ZIP64 + // field, standing in for a timestamp or NTFS field. + extraPrefix []byte + // zip64 emits the 0xFFFFFFFF sentinels plus a ZIP64 extended + // information extra field for this entry. + zip64 bool +} + +func (e rawZipEntry) uncompressed() uint64 { + if e.uncompressedSize != 0 { + return e.uncompressedSize + } + return uint64(len(e.data)) +} + +// clamp32 narrows a size for a 32-bit header field, substituting the ZIP64 +// sentinel when it does not fit. +func clamp32(v uint64) uint32 { + if v >= zip64MagicVal { + return zip64MagicVal + } + return uint32(v) +} + +// buildRawZip assembles an archive byte-for-byte from entries. +func buildRawZip(t testing.TB, entries []rawZipEntry, zip64EOCD bool) []byte { + t.Helper() + + buf := &bytes.Buffer{} + offsets := make([]uint64, len(entries)) + + for i, e := range entries { + offsets[i] = uint64(buf.Len()) + lfh := LocalFileHeader{ + Signature: fileHeaderSignature, + Version: zipVersion, + Crc32: crc32.ChecksumIEEE(e.data), + CompressedSize: clamp32(uint64(len(e.data))), + UncompressedSize: clamp32(e.uncompressed()), + FilenameLength: uint16(len(e.name)), + } + require.NoError(t, binary.Write(buf, binary.LittleEndian, lfh)) + buf.WriteString(e.name) + buf.Write(e.data) + } + + cdOffset := uint64(buf.Len()) + for i, e := range entries { + extra := &bytes.Buffer{} + extra.Write(e.extraPrefix) + + cdh := CDFileHeader{ + Signature: centralDirectoryHeaderSignature, + VersionCreated: zipVersion, + VersionNeeded: zipVersion, + Crc32: crc32.ChecksumIEEE(e.data), + CompressedSize: clamp32(uint64(len(e.data))), + UncompressedSize: clamp32(e.uncompressed()), + FilenameLength: uint16(len(e.name)), + FileCommentLength: uint16(len(e.comment)), + LocalHeaderOffset: clamp32(offsets[i]), + } + + if e.zip64 { + cdh.CompressedSize = zip64MagicVal + cdh.UncompressedSize = zip64MagicVal + cdh.LocalHeaderOffset = zip64MagicVal + require.NoError(t, binary.Write(extra, binary.LittleEndian, Zip64ExtendedInfoExtraField{ + Signature: zip64ExternalID, + Size: zip64ExtendedInfoExtraFieldSize - extraFieldHeaderSize, + OriginalSize: e.uncompressed(), + CompressedSize: uint64(len(e.data)), + LocalFileHeaderOffset: offsets[i], + })) + } + cdh.ExtraFieldLength = uint16(extra.Len()) + + require.NoError(t, binary.Write(buf, binary.LittleEndian, cdh)) + buf.WriteString(e.name) + buf.Write(extra.Bytes()) + buf.WriteString(e.comment) + } + cdSize := uint64(buf.Len()) - cdOffset + + eocd := EndOfCDRecord{ + Signature: endOfCentralDirectorySignature, + NumberOfCDRecordEntries: uint16(len(entries)), + TotalCDRecordEntries: uint16(len(entries)), + SizeOfCentralDirectory: clamp32(cdSize), + CentralDirectoryOffset: clamp32(cdOffset), + } + + if zip64EOCD { + zip64Start := uint64(buf.Len()) + require.NoError(t, binary.Write(buf, binary.LittleEndian, Zip64EndOfCDRecord{ + Signature: zip64EndOfCDSignature, + RecordSize: zip64EndOfCDRecordSize - zip64RecordHeaderSize, + VersionMadeBy: zipVersion, + VersionToExtract: zipVersion, + NumberOfCDRecordEntries: uint64(len(entries)), + TotalCDRecordEntries: uint64(len(entries)), + CentralDirectorySize: cdSize, + StartingDiskCentralDirectoryOffset: cdOffset, + })) + require.NoError(t, binary.Write(buf, binary.LittleEndian, Zip64EndOfCDRecordLocator{ + Signature: zip64EndOfCDLocatorSignature, + CDOffset: zip64Start, + NumberOfDisks: 1, + })) + + eocd.NumberOfCDRecordEntries = zip64MagicVal16 + eocd.TotalCDRecordEntries = zip64MagicVal16 + eocd.SizeOfCentralDirectory = zip64MagicVal + eocd.CentralDirectoryOffset = zip64MagicVal + } + + require.NoError(t, binary.Write(buf, binary.LittleEndian, eocd)) + return buf.Bytes() +} + +// timestampExtraField is a plausible non-ZIP64 extra field (tag 0x5455, +// "extended timestamp") used to push the ZIP64 field off the head of the +// extra-field area. +func timestampExtraField() []byte { + return []byte{0x55, 0x54, 0x05, 0x00, 0x03, 0x01, 0x02, 0x03, 0x04} +} + +// TestReaderZip64ExtraFieldOrder asserts the reader follows APPNOTE 4.5.3's +// ZIP64 value order -- original size, compressed size, local header offset. +// Reading the compressed size first is invisible for STORED entries where +// the two match, so the fixture makes them differ. +func TestReaderZip64ExtraFieldOrder(t *testing.T) { + payload := []byte("seventeen bytes!!") + require.Len(t, payload, 17) + + data := buildRawZip(t, []rawZipEntry{{ + name: "differing.bin", + data: payload, + uncompressedSize: 99, // deliberately not len(payload) + zip64: true, + }}, true) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + + size, err := reader.ReadFileSize("differing.bin") + require.NoError(t, err) + // 99 here would mean the reader took the original size for the + // compressed one, i.e. read the two values in the wrong order. + assert.Equal(t, int64(len(payload)), size) + + got, err := reader.ReadAllFileData("differing.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, payload, got) +} + +// TestReaderZip64ExtraFieldNotFirst asserts the ZIP64 extra field need not +// head the extra-field area. +func TestReaderZip64ExtraFieldNotFirst(t *testing.T) { + payload := []byte("preceded by a timestamp field") + + data := buildRawZip(t, []rawZipEntry{{ + name: "prefixed.bin", + data: payload, + extraPrefix: timestampExtraField(), + zip64: true, + }}, true) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + + got, err := reader.ReadAllFileData("prefixed.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, payload, got) +} + +// TestReaderPerEntryZip64WithoutZip64EOCD asserts a per-entry ZIP64 extra +// field is honored even when the archive's EOCD is not itself ZIP64. +func TestReaderPerEntryZip64WithoutZip64EOCD(t *testing.T) { + payload := []byte("entry is zip64, archive is not") + + data := buildRawZip(t, []rawZipEntry{{ + name: "lonely.bin", + data: payload, + zip64: true, + }}, false) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + + got, err := reader.ReadAllFileData("lonely.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, payload, got) +} + +// TestReaderCentralDirectoryFileComment asserts a comment on one central +// directory entry does not desync the entries that follow it. +func TestReaderCentralDirectoryFileComment(t *testing.T) { + first := []byte("first entry contents") + second := []byte("second entry contents") + + data := buildRawZip(t, []rawZipEntry{ + {name: "first.bin", data: first, comment: "a central directory file comment"}, + {name: "second.bin", data: second}, + }, false) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + + got, err := reader.ReadAllFileData("second.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, second, got) +} + +// TestReaderCentralDirectoryLengthOverflow asserts the three per-entry +// uint16 lengths (name, extra field, comment) are widened before being +// summed. Here they total 65646, which wraps to 110 if summed at 16 bits. +func TestReaderCentralDirectoryLengthOverflow(t *testing.T) { + const longNameLen = 65000 + + second := []byte("second entry contents") + data := buildRawZip(t, []rawZipEntry{ + { + name: strings.Repeat("n", longNameLen), + data: []byte("first entry contents"), + extraPrefix: bytes.Repeat([]byte{0}, 600), + }, + {name: "second.bin", data: second}, + }, false) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + + got, err := reader.ReadAllFileData("second.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, second, got) +} + +// TestReaderZip64DetectedFromEntryCount asserts the entry count is its own +// ZIP64 trigger, independent of the size/offset sentinels, and that its +// sentinel is two bytes wide. +func TestReaderZip64DetectedFromEntryCount(t *testing.T) { + payload := []byte("counted") + data := buildRawZip(t, []rawZipEntry{{name: "counted.bin", data: payload}}, true) + + // Undo the size/offset sentinels the builder set, leaving only the + // entry-count sentinel to signal ZIP64. + eocdStart := len(data) - endOfCDRecordSize + eocd := EndOfCDRecord{} + require.NoError(t, binary.Read(bytes.NewReader(data[eocdStart:]), binary.LittleEndian, &eocd)) + require.Equal(t, uint32(zip64MagicVal), eocd.CentralDirectoryOffset) + + rewritten := &bytes.Buffer{} + rewritten.Write(data[:eocdStart]) + eocd.SizeOfCentralDirectory = 0 + eocd.CentralDirectoryOffset = 0 + require.NoError(t, binary.Write(rewritten, binary.LittleEndian, eocd)) + + reader, err := NewReader(bytes.NewReader(rewritten.Bytes())) + require.NoError(t, err) + + got, err := reader.ReadAllFileData("counted.bin", oneMB) + require.NoError(t, err) + assert.Equal(t, payload, got) +} + +// TestReaderMalformedExtraFieldRejected checks the walk refuses an extra +// field that claims to run past the end of the area rather than reading +// whatever follows it. +func TestReaderMalformedExtraFieldRejected(t *testing.T) { + // Tag 0x0001, declared body length 0xFFFF, no body. + _, err := parseZip64ExtraField([]byte{0x01, 0x00, 0xFF, 0xFF}, CDFileHeader{ + CompressedSize: zip64MagicVal, + }) + require.ErrorIs(t, err, errZipFormat) +} + +// eocdOf decodes the trailing end of central directory record. +func eocdOf(t *testing.T, data []byte) EndOfCDRecord { + t.Helper() + eocd := EndOfCDRecord{} + require.NoError(t, binary.Read(bytes.NewReader(data[len(data)-endOfCDRecordSize:]), binary.LittleEndian, &eocd)) + require.Equal(t, uint32(endOfCentralDirectorySignature), eocd.Signature) + return eocd +} + +// writeOneSegmentArchive drives the segment writer over a single payload. +func writeOneSegmentArchive(t *testing.T, payload []byte, opts ...Option) []byte { + t.Helper() + + w := NewSegmentTDFWriter(1, opts...) + defer w.Close() + + header, err := w.WriteSegment(t.Context(), 0, uint64(len(payload)), crc32.ChecksumIEEE(payload)) + require.NoError(t, err) + + fin, err := w.Finalize(t.Context(), []byte(`{"m":1}`)) + require.NoError(t, err) + + return buildZip(t, [][]byte{header, payload}, fin) +} + +// TestWriterSwitchesToZip64AtInjectedThreshold asserts the writer switches +// to ZIP64 once an entry's size, compressed size, or offset exceeds the +// configured threshold. The production threshold is 2 GiB, which no unit +// test can reach without allocating a 2 GiB payload, so the threshold is +// lowered instead -- the same seam java-sdk uses. +func TestWriterSwitchesToZip64AtInjectedThreshold(t *testing.T) { + const threshold = 1024 + payload := bytes.Repeat([]byte("z"), threshold+1) + + t.Run("above threshold uses zip64", func(t *testing.T) { + data := writeOneSegmentArchive(t, payload, WithMaxNonZip64Value(threshold)) + + eocd := eocdOf(t, data) + assert.Equal(t, uint32(zip64MagicVal), eocd.CentralDirectoryOffset, + "payload above the threshold should defer the EOCD to ZIP64") + + // The archive still has to be readable, by us and by a stock reader. + zr, err := zip.NewReader(bytes.NewReader(data), int64(len(data))) + require.NoError(t, err) + assert.Len(t, zr.File, 2) + + reader, err := NewReader(bytes.NewReader(data)) + require.NoError(t, err) + got, err := reader.ReadAllFileData(TDFPayloadFileName, oneMB) + require.NoError(t, err) + assert.Equal(t, payload, got) + }) + + t.Run("below threshold stays zip32", func(t *testing.T) { + data := writeOneSegmentArchive(t, payload) // default threshold, 2 GiB + + eocd := eocdOf(t, data) + assert.NotEqual(t, uint32(zip64MagicVal), eocd.CentralDirectoryOffset, + "a kilobyte payload should not need ZIP64") + }) +} + +// TestEntryNeedsZip64AtTwoGiB pins the production switch point without +// materializing an archive of that size. +func TestEntryNeedsZip64AtTwoGiB(t *testing.T) { + require.Equal(t, uint64(math.MaxInt32), uint64(maxNonZip64Value)) + + cd := NewCentralDirectory() + + // Every field is a trigger on its own, offsets included -- the local + // condition must not lean on the central directory offset check in + // Finalize to catch a large offset. + for _, tc := range []struct { + name string + entry FileEntry + want bool + }{ + {"just below", FileEntry{Size: maxNonZip64Value, CompressedSize: maxNonZip64Value}, false}, + {"size above", FileEntry{Size: maxNonZip64Value + 1}, true}, + {"compressed size above", FileEntry{CompressedSize: maxNonZip64Value + 1}, true}, + {"offset above", FileEntry{Offset: maxNonZip64Value + 1}, true}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, cd.entryNeedsZip64(tc.entry)) + }) + } +} + +// TestCentralDirectoryNarrowingGuard asserts a value that cannot fit the +// 32-bit field fails loudly instead of being silently truncated into a +// corrupt archive. +func TestCentralDirectoryNarrowingGuard(t *testing.T) { + t.Run("central directory offset", func(t *testing.T) { + cd := NewCentralDirectory() + cd.AddFile(FileEntry{Name: "small", Size: 1, CompressedSize: 1}) + cd.Offset = uint64(math.MaxUint32) + 1 + + _, err := cd.GenerateBytes(false) + require.ErrorIs(t, err, ErrFieldOverflow) + }) + + t.Run("entry count", func(t *testing.T) { + cd := NewCentralDirectory() + cd.Entries = make([]FileEntry, zip64MagicVal16) + + _, err := cd.GenerateBytes(false) + require.ErrorIs(t, err, ErrFieldOverflow) + }) + + t.Run("value that fits is accepted", func(t *testing.T) { + require.NoError(t, checkFitsInCentralDirectory("size", zip64MagicVal-1)) + // The sentinel itself cannot be written as a literal value. + require.ErrorIs(t, checkFitsInCentralDirectory("size", zip64MagicVal), ErrFieldOverflow) + }) +} diff --git a/sdk/internal/zipstream/zip_headers.go b/sdk/internal/zipstream/zip_headers.go index f6ad2bde76..881164700e 100644 --- a/sdk/internal/zipstream/zip_headers.go +++ b/sdk/internal/zipstream/zip_headers.go @@ -9,6 +9,7 @@ const ( endOfCentralDirectorySignature = 0x06054b50 zip64EndOfCDLocatorSignature = 0x07064b50 zip64MagicVal = 0xFFFFFFFF + zip64MagicVal16 = 0xFFFF // sentinel for the 2-byte EOCD entry counts zip64EndOfCDSignature = 0x06064b50 zip64ExternalID = 0x0001 zipVersion = 0x2D // version 4.5 of the PKZIP specification diff --git a/sdk/internal/zipstream/zip_primitives.go b/sdk/internal/zipstream/zip_primitives.go index b783954e11..b430910bfe 100644 --- a/sdk/internal/zipstream/zip_primitives.go +++ b/sdk/internal/zipstream/zip_primitives.go @@ -5,9 +5,21 @@ package zipstream import ( "bytes" "encoding/binary" + "fmt" + "math" "time" ) +// maxNonZip64Value is the largest value this writer will place in a 32-bit +// ZIP field before switching the entry to ZIP64. The fields are unsigned on +// the wire and would hold up to 0xFFFFFFFF, but readers that widen them with +// a signed read -- every java-sdk released before opentdf/java-sdk#393, and +// those stay in the field indefinitely -- see anything above 2 GiB as +// negative. Switching at Integer.MAX_VALUE instead, matching java-sdk's +// MAX_NON_ZIP64_VALUE, costs 28 bytes per affected entry and keeps archives +// in the 2-4 GiB band readable by those clients. +const maxNonZip64Value = math.MaxInt32 + // Note: CRC32 calculation for the payload is performed using a combine // approach over per-segment CRCs and sizes to avoid buffering segments. @@ -196,6 +208,11 @@ type CentralDirectory struct { Entries []FileEntry // File entries in the archive Offset uint64 // Offset where central directory starts Size uint64 // Size of central directory + // MaxNonZip64Value is the largest value written into a 32-bit field + // before the entry switches to ZIP64. Zero means maxNonZip64Value. + // Lowering it is a test seam: it exercises the ZIP64 path without + // materializing a multi-gigabyte archive. + MaxNonZip64Value uint64 } // NewCentralDirectory creates a new central directory @@ -205,6 +222,18 @@ func NewCentralDirectory() *CentralDirectory { } } +// checkFitsInCentralDirectory guards a narrowing conversion into a 32-bit +// central directory field. The surrounding ZIP64 conditions already keep +// these values in range, so a failure here means one of them is wrong; +// erroring out beats silently emitting a truncated -- and therefore corrupt +// -- archive. +func checkFitsInCentralDirectory(field string, value uint64) error { + if value >= zip64MagicVal { + return fmt.Errorf("%w: %s is %d, which does not fit in a 32-bit zip field", ErrFieldOverflow, field, value) + } + return nil +} + // AddFile adds a file entry to the central directory func (cd *CentralDirectory) AddFile(entry FileEntry) { cd.Entries = append(cd.Entries, entry) @@ -218,7 +247,7 @@ func (cd *CentralDirectory) GenerateBytes(isZip64 bool) ([]byte, error) { cdEntriesSize := uint64(0) for _, entry := range cd.Entries { entrySize := cdFileHeaderSize + uint64(len(entry.Name)) - if isZip64 || entry.Size >= uint64(^uint32(0)) || entry.CompressedSize >= uint64(^uint32(0)) { + if isZip64 || cd.entryNeedsZip64(entry) { entrySize += zip64ExtendedInfoExtraFieldSize } cdEntriesSize += entrySize @@ -242,6 +271,25 @@ func (cd *CentralDirectory) GenerateBytes(isZip64 bool) ([]byte, error) { return buf.Bytes(), nil } +// maxNonZip64 resolves the ZIP64 switch point for this directory. +func (cd *CentralDirectory) maxNonZip64() uint64 { + if cd.MaxNonZip64Value == 0 { + return maxNonZip64Value + } + return cd.MaxNonZip64Value +} + +// entryNeedsZip64 reports whether an entry cannot be described by the 32-bit +// central directory fields alone. Offsets count as well as sizes: an entry +// starting past the switch point needs the ZIP64 extra field even when the +// entry itself is small. +func (cd *CentralDirectory) entryNeedsZip64(entry FileEntry) bool { + maxValue := cd.maxNonZip64() + return entry.Size > maxValue || + entry.CompressedSize > maxValue || + entry.Offset > maxValue +} + // msDosTimeDate encodes t as the (time, date) pair ZIP headers carry. // // The date packs the year as a 7-bit offset from zipBaseYear, so only @@ -277,6 +325,24 @@ func msDosTimeDate(t time.Time) (uint16, uint16) { func (cd *CentralDirectory) writeCDFileHeader(buf *bytes.Buffer, entry FileEntry, isZip64 bool) error { lastModifiedTime, lastModifiedDate := msDosTimeDate(entry.ModTime) + useZip64 := isZip64 || cd.entryNeedsZip64(entry) + if !useZip64 { + // Only the 32-bit path narrows these; the ZIP64 path overwrites + // them with the sentinel below. + for _, f := range []struct { + name string + value uint64 + }{ + {"compressed size of " + entry.Name, entry.CompressedSize}, + {"uncompressed size of " + entry.Name, entry.Size}, + {"local header offset of " + entry.Name, entry.Offset}, + } { + if err := checkFitsInCentralDirectory(f.name, f.value); err != nil { + return err + } + } + } + header := CDFileHeader{ Signature: centralDirectoryHeaderSignature, VersionCreated: zipVersion, @@ -303,7 +369,7 @@ func (cd *CentralDirectory) writeCDFileHeader(buf *bytes.Buffer, entry FileEntry } // Handle ZIP64 if needed - if isZip64 || entry.Size >= uint64(^uint32(0)) || entry.CompressedSize >= uint64(^uint32(0)) { + if useZip64 { header.CompressedSize = zip64MagicVal header.UncompressedSize = zip64MagicVal header.LocalHeaderOffset = zip64MagicVal @@ -373,6 +439,22 @@ func (cd *CentralDirectory) writeEndOfCDRecord(buf *bytes.Buffer, isZip64 bool) } } + if !isZip64 { + // The 32-bit EOCD carries these verbatim. Same reasoning as + // checkFitsInCentralDirectory: fail loudly rather than emit a + // trailer that points somewhere else in the archive. + if err := checkFitsInCentralDirectory("central directory size", cd.Size); err != nil { + return err + } + if err := checkFitsInCentralDirectory("central directory offset", cd.Offset); err != nil { + return err + } + if len(cd.Entries) >= zip64MagicVal16 { + return fmt.Errorf("%w: entry count is %d, which does not fit in a 16-bit zip field", + ErrFieldOverflow, len(cd.Entries)) + } + } + // Write standard end of central directory record endOfCD := EndOfCDRecord{ Signature: endOfCentralDirectorySignature, @@ -387,8 +469,8 @@ func (cd *CentralDirectory) writeEndOfCDRecord(buf *bytes.Buffer, isZip64 bool) // Use ZIP64 values if needed if isZip64 { - endOfCD.NumberOfCDRecordEntries = 0xFFFF - endOfCD.TotalCDRecordEntries = 0xFFFF + endOfCD.NumberOfCDRecordEntries = zip64MagicVal16 + endOfCD.TotalCDRecordEntries = zip64MagicVal16 endOfCD.SizeOfCentralDirectory = zip64MagicVal endOfCD.CentralDirectoryOffset = zip64MagicVal }