From 01d583c13447ce10264ad311e3436a0edc29c702 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 and per-segment size defaults 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. --- 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 | 421 ++++++++++++++++++ sdk/internal/zipstream/zip_headers.go | 1 + sdk/internal/zipstream/zip_primitives.go | 90 +++- sdk/manifest.go | 38 ++ sdk/tdf.go | 48 +- sdk/tdf_segment_defaults_test.go | 215 +++++++++ sdk/tdferrors.go | 1 + 11 files changed, 1016 insertions(+), 77 deletions(-) create mode 100644 sdk/internal/zipstream/zip64_conformance_test.go create mode 100644 sdk/tdf_segment_defaults_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..91e80f50f3 --- /dev/null +++ b/sdk/internal/zipstream/zip64_conformance_test.go @@ -0,0 +1,421 @@ +// 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 covers finding 2: APPNOTE 4.5.3 orders the +// ZIP64 values 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 covers finding 4: the ZIP64 field is not +// required to 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 covers the other half of finding 4: +// a ZIP64 extra field on an entry in an archive whose EOCD is not 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 covers the first half of finding 5: +// a comment on one entry must 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 covers the second half of finding +// 5: the three uint16 lengths must be widened before they are summed. Here +// they total 65646, which wraps to 110 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 covers finding 3: the entry count is +// its own ZIP64 trigger, and it 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 covers finding 1. The +// production switch point 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 covers finding 6: a value that cannot +// fit the 32-bit field has to fail loudly instead of being 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 } diff --git a/sdk/manifest.go b/sdk/manifest.go index fc3034fddc..a25b085617 100644 --- a/sdk/manifest.go +++ b/sdk/manifest.go @@ -1,5 +1,17 @@ package sdk +import "fmt" + +// Segment describes one chunk of the payload. +// +// Size and EncryptedSize are optional in the wire format: +// manifest.schema.json marks segmentSizeDefault and +// encryptedSegmentSizeDefault required on integrityInformation but declares +// no required list on segments/items, so a writer may omit a per-segment +// size whenever it equals the manifest-level default. web-sdk does exactly +// that for every full-sized segment. An omitted key unmarshals to 0, which +// is not a legal segment size, so 0 means "absent, use the default" -- see +// IntegrityInformation.resolveSegmentSizes. type Segment struct { Hash string `json:"hash"` Size int64 `json:"segmentSize"` @@ -19,6 +31,32 @@ type IntegrityInformation struct { Segments []Segment `json:"segments"` } +// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg, +// substituting the manifest-level defaults for values the writer omitted. +// +// Both must come out positive. A segment of length zero is not something a +// writer can legitimately describe, and letting one through makes the +// caller's `len(readBuf) != encryptedSize` check pass vacuously -- the read +// then fails several frames later inside the GMAC signature calculation, +// where the message says nothing about the manifest. +func (i IntegrityInformation) resolveSegmentSizes(seg Segment) (int64, int64, error) { + size := seg.Size + if size == 0 { + size = i.DefaultSegmentSize + } + + encryptedSize := seg.EncryptedSize + if encryptedSize == 0 { + encryptedSize = i.DefaultEncryptedSegSize + } + + if size <= 0 || encryptedSize <= 0 { + return 0, 0, fmt.Errorf("%w: segmentSize=%d encryptedSegmentSize=%d", ErrSegSizeUnresolved, size, encryptedSize) + } + + return size, encryptedSize, nil +} + type KeyAccess struct { KeyType string `json:"type"` KasURL string `json:"url"` diff --git a/sdk/tdf.go b/sdk/tdf.go index ce42099603..098fb8c658 100644 --- a/sdk/tdf.go +++ b/sdk/tdf.go @@ -907,7 +907,14 @@ func (s SDK) LoadTDF(reader io.ReadSeeker, opts ...TDFReaderOption) (*Reader, er var payloadSize int64 for _, seg := range manifestObj.Segments { - payloadSize += seg.Size + // Sizes the writer left to the manifest-level default have to be + // filled in here too: without it the payload looks shorter than it + // is, and every read bounded by payloadSize comes up short. + size, _, err := manifestObj.resolveSegmentSizes(seg) + if err != nil { + return nil, err + } + payloadSize += size } return &Reader{ @@ -984,18 +991,23 @@ func (r *Reader) WriteTo(writer io.Writer) (int64, error) { var payloadReadOffset int64 var decryptedDataOffset int64 for _, seg := range r.manifest.Segments { - if decryptedDataOffset+seg.Size < r.cursor { - decryptedDataOffset += seg.Size - payloadReadOffset += seg.EncryptedSize + segSize, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) + if err != nil { + return totalBytes, err + } + + if decryptedDataOffset+segSize < r.cursor { + decryptedDataOffset += segSize + payloadReadOffset += encryptedSegSize continue } - readBuf, err := r.tdfReader.ReadPayload(payloadReadOffset, seg.EncryptedSize) + readBuf, err := r.tdfReader.ReadPayload(payloadReadOffset, encryptedSegSize) if err != nil { return totalBytes, fmt.Errorf("TDFReader.ReadPayload failed: %w", err) } - if int64(len(readBuf)) != seg.EncryptedSize { + if int64(len(readBuf)) != encryptedSegSize { return totalBytes, ErrSegSizeMismatch } @@ -1034,9 +1046,9 @@ func (r *Reader) WriteTo(writer io.Writer) (int64, error) { return totalBytes, errWriteFailed } - payloadReadOffset += seg.EncryptedSize + payloadReadOffset += encryptedSegSize r.cursor += int64(n) - decryptedDataOffset += seg.Size + decryptedDataOffset += segSize } return totalBytes, nil @@ -1059,7 +1071,14 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen return 0, ErrTDFPayloadInvalidOffset } + // segmentSizeDefault is required by the manifest schema, and the index + // arithmetic below divides by it; a manifest that omits it would panic + // rather than report the malformed input. defaultSegmentSize := r.manifest.DefaultSegmentSize + if defaultSegmentSize <= 0 { + return 0, fmt.Errorf("%w: segmentSizeDefault=%d", ErrSegSizeUnresolved, defaultSegmentSize) + } + start := offset / defaultSegmentSize end := (offset + int64(len(buf)) + defaultSegmentSize - 1) / defaultSegmentSize // rounds up @@ -1082,17 +1101,22 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen break } + _, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) + if err != nil { + return 0, err + } + if firstSegment > int64(index) { - payloadReadOffset += seg.EncryptedSize + payloadReadOffset += encryptedSegSize continue } - readBuf, err := r.tdfReader.ReadPayload(payloadReadOffset, seg.EncryptedSize) + readBuf, err := r.tdfReader.ReadPayload(payloadReadOffset, encryptedSegSize) if err != nil { return 0, fmt.Errorf("TDFReader.ReadPayload failed: %w", err) } - if int64(len(readBuf)) != seg.EncryptedSize { + if int64(len(readBuf)) != encryptedSegSize { return 0, ErrSegSizeMismatch } @@ -1125,7 +1149,7 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen return 0, errWriteFailed } - payloadReadOffset += seg.EncryptedSize + payloadReadOffset += encryptedSegSize } var err error diff --git a/sdk/tdf_segment_defaults_test.go b/sdk/tdf_segment_defaults_test.go new file mode 100644 index 0000000000..5c8e6f0537 --- /dev/null +++ b/sdk/tdf_segment_defaults_test.go @@ -0,0 +1,215 @@ +package sdk + +import ( + "bytes" + "context" + "encoding/json" + "hash/crc32" + "io" + "testing" + + "github.com/opentdf/platform/sdk/internal/zipstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// webSDKSegmentSize is web-sdk's DEFAULT_SEGMENT_SIZE. It is the point at +// which a web-sdk container first contains a segment whose size equals the +// manifest-level default, and therefore the point at which web-sdk starts +// omitting the per-segment sizes. +const webSDKSegmentSize = 1024 * 1024 + +// stripDefaultSegmentSizes rewrites a TDF so that every segment whose sizes +// match the manifest-level defaults carries neither segmentSize nor +// encryptedSegmentSize, reproducing what web-sdk emits. It returns the +// rewritten archive and the number of segments it stripped. +// +// The manifest is only re-serialized, never re-signed: the root signature +// covers the segment hashes, not the JSON encoding, so dropping these keys +// leaves a container that is still internally consistent -- exactly the +// situation go-sdk has to cope with. +func (s *TDFSuite) stripDefaultSegmentSizes(tdfBytes []byte) ([]byte, int) { + s.T().Helper() + + zipReader, err := zipstream.NewReader(bytes.NewReader(tdfBytes)) + s.Require().NoError(err) + + manifestBytes, err := zipReader.ReadAllFileData(zipstream.TDFManifestFileName, 10*oneMB) + s.Require().NoError(err) + + payloadSize, err := zipReader.ReadFileSize(zipstream.TDFPayloadFileName) + s.Require().NoError(err) + payload, err := zipReader.ReadFileData(zipstream.TDFPayloadFileName, 0, payloadSize) + s.Require().NoError(err) + + var manifest map[string]any + s.Require().NoError(json.Unmarshal(manifestBytes, &manifest)) + + encryptionInfo, ok := manifest["encryptionInformation"].(map[string]any) + s.Require().True(ok) + integrityInfo, ok := encryptionInfo["integrityInformation"].(map[string]any) + s.Require().True(ok) + segments, ok := integrityInfo["segments"].([]any) + s.Require().True(ok) + + defaultSize, ok := integrityInfo["segmentSizeDefault"].(float64) + s.Require().True(ok) + defaultEncryptedSize, ok := integrityInfo["encryptedSegmentSizeDefault"].(float64) + s.Require().True(ok) + + stripped := 0 + for _, raw := range segments { + segment, isObject := raw.(map[string]any) + s.Require().True(isObject) + if segment["segmentSize"] != defaultSize || segment["encryptedSegmentSize"] != defaultEncryptedSize { + continue + } + delete(segment, "segmentSize") + delete(segment, "encryptedSegmentSize") + stripped++ + } + + rewritten, err := json.Marshal(manifest) + s.Require().NoError(err) + + ctx := context.Background() + writer := zipstream.NewSegmentTDFWriter(1) + defer func() { s.Require().NoError(writer.Close()) }() + + out := &bytes.Buffer{} + header, err := writer.WriteSegment(ctx, 0, uint64(len(payload)), crc32.ChecksumIEEE(payload)) + s.Require().NoError(err) + out.Write(header) + out.Write(payload) + + final, err := writer.Finalize(ctx, rewritten) + s.Require().NoError(err) + out.Write(final) + + return out.Bytes(), stripped +} + +// Test_SegmentSizesOmittedFallBackToDefaults covers DSPX-4590 finding 7. +// +// web-sdk omits segmentSize/encryptedSegmentSize whenever they equal the +// manifest-level defaults, which the schema permits. go-sdk used to read the +// absent keys as zero, so every web-sdk container over 1 MiB -- the first +// size at which a full-width segment appears -- failed to decrypt. +func (s *TDFSuite) Test_SegmentSizesOmittedFallBackToDefaults() { + // Two full segments plus a partial one, so the fixture covers both the + // omitted and the explicitly-sized case. + plaintext := make([]byte, 2*webSDKSegmentSize+4242) + for i := range plaintext { + plaintext[i] = byte(i % 251) + } + + kasInfoList := make([]KASInfo, len(s.kases)) + for i, ki := range s.kases { + kasInfoList[i] = ki.KASInfo + kasInfoList[i].PublicKey = "" + } + kasInfoList[0].Default = true + + original := &bytes.Buffer{} + _, err := s.sdk.CreateTDF(original, bytes.NewReader(plaintext), + WithKasInformation(kasInfoList...), + WithSegmentSize(webSDKSegmentSize), + ) + s.Require().NoError(err) + + tdfBytes, stripped := s.stripDefaultSegmentSizes(original.Bytes()) + s.Require().Equal(2, stripped, "fixture should have two default-sized segments to strip") + + s.Run("WriteTo", func() { + r, err := s.sdk.LoadTDF(bytes.NewReader(tdfBytes)) + s.Require().NoError(err) + + // payloadSize has to account for the omitted segments too; + // otherwise Seek and the ReadAt bounds check both truncate. + s.Require().Equal(int64(len(plaintext)), r.payloadSize) + + decrypted := &bytes.Buffer{} + n, err := io.Copy(decrypted, r) + s.Require().NoError(err) + s.Require().Equal(int64(len(plaintext)), n) + s.Require().Equal(plaintext, decrypted.Bytes()) + }) + + s.Run("ReadAt", func() { + r, err := s.sdk.LoadTDF(bytes.NewReader(tdfBytes)) + s.Require().NoError(err) + + // Start inside the second segment so the read has to skip a + // segment whose size was omitted before it decrypts one. + const offset = webSDKSegmentSize + 100 + buf := make([]byte, 4096) + n, err := r.ReadAt(buf, offset) + s.Require().NoError(err) + s.Require().Equal(len(buf), n) + s.Require().Equal(plaintext[offset:offset+int64(len(buf))], buf) + }) +} + +// TestResolveSegmentSizes covers the fallback rules directly, including the +// zero-length segment that used to slip past the read-length check and fail +// later inside the GMAC calculation. +func TestResolveSegmentSizes(t *testing.T) { + defaults := IntegrityInformation{ + DefaultSegmentSize: 1024, + DefaultEncryptedSegSize: 1052, + } + + for _, tc := range []struct { + name string + integrity IntegrityInformation + segment Segment + wantSize int64 + wantEncryptedSize int64 + wantErr bool + }{ + { + name: "explicit sizes win", + integrity: defaults, + segment: Segment{Size: 7, EncryptedSize: 35}, + wantSize: 7, + wantEncryptedSize: 35, + }, + { + name: "both omitted fall back", + integrity: defaults, + segment: Segment{}, + wantSize: 1024, + wantEncryptedSize: 1052, + }, + { + name: "one omitted falls back", + integrity: defaults, + segment: Segment{Size: 7}, + wantSize: 7, + wantEncryptedSize: 1052, + }, + { + name: "no value and no default is an error", + integrity: IntegrityInformation{}, + segment: Segment{}, + wantErr: true, + }, + { + name: "negative default is an error", + integrity: IntegrityInformation{DefaultSegmentSize: -1, DefaultEncryptedSegSize: -1}, + segment: Segment{}, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + size, encryptedSize, err := tc.integrity.resolveSegmentSizes(tc.segment) + if tc.wantErr { + require.ErrorIs(t, err, ErrSegSizeUnresolved) + return + } + require.NoError(t, err) + assert.Equal(t, tc.wantSize, size) + assert.Equal(t, tc.wantEncryptedSize, encryptedSize) + }) + } +} diff --git a/sdk/tdferrors.go b/sdk/tdferrors.go index 37a69a23ac..5e260a4056 100644 --- a/sdk/tdferrors.go +++ b/sdk/tdferrors.go @@ -15,6 +15,7 @@ var ( ErrTampered = errors.New("tamper detected") ErrRootSigValidation = fmt.Errorf("[%w] tdf: failed integrity check on root signature", ErrTampered) ErrSegSizeMismatch = fmt.Errorf("[%w] tdf: mismatch encrypted segment size in manifest", ErrTampered) + ErrSegSizeUnresolved = fmt.Errorf("[%w] tdf: segment size missing from manifest with no default to fall back on", ErrTampered) ErrSegSigValidation = fmt.Errorf("[%w] tdf: failed integrity check on segment hash", ErrTampered) ErrTDFPayloadReadFail = fmt.Errorf("[%w] tdf: fail to read payload from tdf", ErrTampered) ErrTDFPayloadInvalidOffset = fmt.Errorf("[%w] sdk.Reader.ReadAt: negative offset", ErrTampered)