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)