Skip to content

Commit 01d6d43

Browse files
fix(sdk): DSPX-4590 zip64 conformance
go-sdk's zip layer disagrees with the other SDKs in a handful of places. This addresses findings 1-6 from the DSPX-4590 investigation (finding 7, per-segment size defaults, is the parent PR this one is stacked on). ## Finding 1 (interop): writer switched to ZIP64 at 4 GiB instead of 2 GiB `Finalize` compared against `^uint32(0)`, so a payload between 2 GiB and 4 GiB was written as a zip32 archive with a value in the top half of the unsigned 32-bit range. java-sdk widens those central-directory fields *signed*, so deployed Java clients read the size/offset back as a negative number and cannot open the container. web-sdk always writes ZIP64, java-sdk (since java-sdk#393) switches at `Integer.MAX_VALUE`. - New `maxNonZip64Value = math.MaxInt32` in `zip_primitives.go`, mirroring java-sdk's `MAX_NON_ZIP64_VALUE`. - The rule is applied to the uncompressed size, the compressed size **and** `entry.Offset` (the local-header offset), which was previously not checked at all -- an archive under 2 GiB of payload could still place a later entry's header past the boundary. - The threshold is injectable: `Config.MaxNonZip64Value` plus a `WithMaxNonZip64Value` option (clamped to `(0, maxNonZip64Value]`, so it can only ever make the writer *more* eager to use ZIP64). This lets the tests drive the ZIP64 path with a 1 KiB threshold instead of allocating gigabytes. ## Finding 2: ZIP64 extra field parsed positionally The reader assumed the ZIP64 extended-information field was the first entry in the extra-field area and that all three values were always present. Per APPNOTE 4.5.3 the values appear in the order *original size, compressed size, local header offset*, and each is present **only** when its central-directory counterpart holds the `0xFFFFFFFF` sentinel. A container whose extra area leads with, say, an extended-timestamp field (tag `0x5455`) was misparsed. `parseZip64ExtraField` now walks the whole extra area, skips foreign tags, reads values in spec order gated on the sentinels, and rejects a field that claims to run past the end of the area. ## Finding 3: ZIP64 detected from the CD offset alone `NewReader` only looked at `CentralDirectoryOffset == 0xFFFFFFFF`. An archive that overflows the entry count (`0xFFFF`) or the central-directory size but not the offset was read as zip32. `eocdNeedsZip64` now checks all three EOCD sentinel fields. ## Finding 4: per-entry ZIP64 extra field ignored without a ZIP64 EOCD A writer may put a ZIP64 extra field on an individual entry while leaving the EOCD in zip32 form. The reader now consults the extra field whenever the central-directory header carries a sentinel, independent of the EOCD form. ## Finding 5: central-directory cursor could wrap at uint16 `nextCD` was advanced with uint16 arithmetic and did not include the file comment. A 65000-byte filename plus a 600-byte extra field wraps to 110 and the reader walks into the middle of a header. The advance is now done in uint64 and includes `FileCommentLength`. ## Finding 6: silent truncation when a value does not fit 32 bits The zip32 paths narrowed with a bare cast. `checkFitsInCentralDirectory` now returns a new `ErrFieldOverflow` for the compressed size, uncompressed size, local-header offset, central-directory size/offset and entry count instead of writing a corrupt archive. (With finding 1 in place this is unreachable in normal operation; it is a backstop against future callers.) ## Tests `sdk/internal/zipstream/zip64_conformance_test.go` -- hand-assembles raw zip archives (`buildRawZip`) so the reader can be pointed at containers no Go writer would produce: extra field not first, differing compressed and uncompressed sizes so the APPNOTE 4.5.3 ordering is actually asserted (a fixture with equal sizes passes either way), per-entry ZIP64 under a zip32 EOCD, a central-directory file comment, the 65646-byte name+extra case that wraps to 110 at uint16, ZIP64 implied by the entry count, and a malformed extra field. Writer side: `TestWriterSwitchesToZip64AtInjectedThreshold` uses the injected 1 KiB threshold, `TestEntryNeedsZip64AtTwoGiB` pins `maxNonZip64Value == math.MaxInt32` and covers size / compressed size / offset, `TestCentralDirectoryNarrowingGuard` covers finding 6. `sdk/internal/zipstream/fuzz_test.go` -- two new seeds for the `nextCD` overflow and the file-comment case. ## Follow-up required in opentdf/tests (NOT covered by this PR) > The xtest cell `test_tdfs.py::test_chunky_roundtrip` currently **SKIPS** for > go, because `xtest/sdk/go/cli.sh` answers no to `supports chunky`. That shim > lives in the `opentdf/tests` repo, so merging this PR does **not** flip it -- > the go column will stay skipped and the interop regression will stay > invisible in CI. When this fix ships in a release, someone needs to > version-gate the `chunky)` case in `xtest/sdk/go/cli.sh` so it reports > support at or above that version. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
1 parent 128c23a commit 01d6d43

7 files changed

Lines changed: 728 additions & 65 deletions

File tree

sdk/internal/zipstream/fuzz_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package zipstream
55
import (
66
"bytes"
77
"encoding/base64"
8+
"strings"
89
"testing"
910

1011
"github.com/stretchr/testify/assert"
@@ -130,6 +131,24 @@ func FuzzReader(f *testing.F) {
130131
"AAAAAAAAAwLnBheWxvYWRQSwECLQAtAAgAAAB9dS8xAuiuLAIE///tBQAADwAAAAAAAAAAA" +
131132
"AAAAABWAAAAMC5tYW5pZmVzdC5qc29uUEsFBgAAAAACAAIAdAAAAJUFAAAAAA=="))
132133

134+
// A central directory file comment: the entry after it only parses if
135+
// nextCD accounts for FileCommentLength.
136+
f.Add(buildRawZip(f, []rawZipEntry{
137+
{name: "0.payload", data: []byte("payload bytes"), comment: "central directory file comment"},
138+
{name: "0.manifest.json", data: []byte(`{"m":1}`)},
139+
}, false))
140+
141+
// Filename plus extra field plus header size sums to 65646, which wraps
142+
// to 110 if the addition happens at uint16 width.
143+
f.Add(buildRawZip(f, []rawZipEntry{
144+
{
145+
name: strings.Repeat("n", 65000),
146+
data: []byte("payload bytes"),
147+
extraPrefix: bytes.Repeat([]byte{0}, 600),
148+
},
149+
{name: "0.manifest.json", data: []byte(`{"m":1}`)},
150+
}, false))
151+
133152
f.Fuzz(func(t *testing.T, data []byte) {
134153
reader, err := NewReader(bytes.NewReader(data))
135154
if err != nil {

sdk/internal/zipstream/reader.go

Lines changed: 157 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -72,15 +72,19 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) {
7272
}
7373

7474
// check if zip is zip64 or zip32 format
75+
//
76+
// Any of the three EOCD fields that ZIP64 can overflow may carry the
77+
// sentinel independently: the entry count (two bytes wide, so its
78+
// sentinel is 0xFFFF), the central directory size, and the central
79+
// directory offset. An archive with more than 65534 entries needs ZIP64
80+
// for the count alone while its central directory still starts below
81+
// 4 GiB, so keying off the offset by itself misses it.
7582
var entryCount uint64
7683
var centralDirectoryStart uint64
77-
isZip64 := false
78-
if endOfCDRecord.CentralDirectoryOffset != zip64MagicVal { //nolint:nestif // pkzip is complicated
84+
if !eocdNeedsZip64(endOfCDRecord) { //nolint:nestif // pkzip is complicated
7985
entryCount = uint64(endOfCDRecord.NumberOfCDRecordEntries)
8086
centralDirectoryStart = uint64(endOfCDRecord.CentralDirectoryOffset)
8187
} else {
82-
isZip64 = true
83-
8488
// read zip64 end of central directory locator
8589
_, err := readSeeker.Seek(-(endOfCDRecordSize + zip64EndOfCDRecordLocatorSize), io.SeekEnd)
8690
if err != nil {
@@ -144,52 +148,10 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) {
144148
return reader, fmt.Errorf("binary.Read failed: %w", err)
145149
}
146150

147-
offset := uint64(cdFileHeader.LocalHeaderOffset)
148-
bytesToRead := uint64(cdFileHeader.CompressedSize)
149-
150-
if isZip64 { //nolint:nestif // pkzip is complicated
151-
// read Zip64 Extended Information extra field id
152-
headerTag := uint16(0)
153-
err = binary.Read(readSeeker, binary.LittleEndian, &headerTag)
154-
if err != nil {
155-
return reader, fmt.Errorf("binary.Read failed: %w", err)
156-
}
157-
158-
// read Zip64 Extended Information Extra Field Block Size
159-
blockSize := uint16(0)
160-
err = binary.Read(readSeeker, binary.LittleEndian, &blockSize)
161-
if err != nil {
162-
return reader, fmt.Errorf("binary.Read failed: %w", err)
163-
}
164-
165-
if headerTag == zip64ExternalID {
166-
if cdFileHeader.CompressedSize == zip64MagicVal {
167-
compressedSize := uint64(0)
168-
err = binary.Read(readSeeker, binary.LittleEndian, &compressedSize)
169-
if err != nil {
170-
return reader, fmt.Errorf("binary.Read failed: %w", err)
171-
}
172-
173-
bytesToRead = compressedSize
174-
}
175-
176-
if cdFileHeader.UncompressedSize == zip64MagicVal {
177-
uncompressedSize := uint64(0)
178-
err = binary.Read(readSeeker, binary.LittleEndian, &uncompressedSize)
179-
if err != nil {
180-
return reader, fmt.Errorf("binary.Read failed: %w", err)
181-
}
182-
}
183-
184-
if cdFileHeader.LocalHeaderOffset == zip64MagicVal {
185-
localHeaderOffset := uint64(0)
186-
err = binary.Read(readSeeker, binary.LittleEndian, &localHeaderOffset)
187-
if err != nil {
188-
return reader, fmt.Errorf("binary.Read failed: %w", err)
189-
}
190-
offset = localHeaderOffset
191-
}
192-
}
151+
// readSeeker is now positioned at this entry's extra-field area.
152+
offset, bytesToRead, err := resolveEntryLocation(readSeeker, cdFileHeader)
153+
if err != nil {
154+
return reader, err
193155
}
194156

195157
// Read each file
@@ -214,12 +176,156 @@ func NewReader(readSeeker io.ReadSeeker) (Reader, error) {
214176

215177
reader.fileEntries[string(fileNameByteArray)] = zipFileEntry
216178

217-
nextCD += uint64(cdFileHeader.ExtraFieldLength + cdFileHeader.FilenameLength + cdFileHeaderSize)
179+
// Widen every term before summing: all three header lengths are
180+
// uint16, so adding them at their declared width wraps at 65536 and
181+
// lands the next seek inside the current entry. The file comment is
182+
// part of the record too -- omitting it desyncs every entry after
183+
// the first one that carries a comment.
184+
nextCD += uint64(cdFileHeaderSize) +
185+
uint64(cdFileHeader.FilenameLength) +
186+
uint64(cdFileHeader.ExtraFieldLength) +
187+
uint64(cdFileHeader.FileCommentLength)
218188
}
219189

220190
return reader, nil
221191
}
222192

193+
// resolveEntryLocation returns the local header offset and the number of
194+
// stored bytes for a central directory entry, reading the ZIP64 extended
195+
// information extra field when the 32-bit fields carry the sentinel.
196+
//
197+
// That field is a property of the entry, not of the archive: APPNOTE permits
198+
// one on an entry whose EOCD is not ZIP64, so the lookup is driven off this
199+
// entry's own sentinel values rather than an archive-wide flag. The reader
200+
// must be positioned at the start of the entry's extra-field area, and the
201+
// area is only consumed when the entry declares one -- reading
202+
// unconditionally would eat the bytes of whatever record follows.
203+
func resolveEntryLocation(readSeeker io.Reader, cdFileHeader CDFileHeader) (uint64, uint64, error) {
204+
offset := uint64(cdFileHeader.LocalHeaderOffset)
205+
bytesToRead := uint64(cdFileHeader.CompressedSize)
206+
207+
if cdFileHeader.ExtraFieldLength == 0 || !cdHeaderHasZip64Sentinel(cdFileHeader) {
208+
return offset, bytesToRead, nil
209+
}
210+
211+
extraFields := make([]byte, cdFileHeader.ExtraFieldLength)
212+
if _, err := io.ReadFull(readSeeker, extraFields); err != nil {
213+
return 0, 0, fmt.Errorf("io.ReadFull failed: %w", err)
214+
}
215+
216+
zip64, err := parseZip64ExtraField(extraFields, cdFileHeader)
217+
if err != nil {
218+
return 0, 0, err
219+
}
220+
221+
if zip64.found {
222+
if cdFileHeader.CompressedSize == zip64MagicVal {
223+
bytesToRead = zip64.compressedSize
224+
}
225+
if cdFileHeader.LocalHeaderOffset == zip64MagicVal {
226+
offset = zip64.localHeaderOffset
227+
}
228+
}
229+
230+
return offset, bytesToRead, nil
231+
}
232+
233+
// eocdNeedsZip64 reports whether the end of central directory record defers
234+
// any of its fields to the ZIP64 end of central directory record. Note the
235+
// entry count is two bytes wide, so it uses a 16-bit sentinel.
236+
func eocdNeedsZip64(eocd EndOfCDRecord) bool {
237+
return eocd.CentralDirectoryOffset == zip64MagicVal ||
238+
eocd.SizeOfCentralDirectory == zip64MagicVal ||
239+
eocd.NumberOfCDRecordEntries == zip64MagicVal16
240+
}
241+
242+
// cdHeaderHasZip64Sentinel reports whether any central directory field of
243+
// this entry defers its value to a ZIP64 extended information extra field.
244+
func cdHeaderHasZip64Sentinel(h CDFileHeader) bool {
245+
return h.CompressedSize == zip64MagicVal ||
246+
h.UncompressedSize == zip64MagicVal ||
247+
h.LocalHeaderOffset == zip64MagicVal
248+
}
249+
250+
// zip64ExtraValues holds the values a ZIP64 Extended Information extra
251+
// field supplies for one central directory entry. Only the fields whose
252+
// central directory counterpart carried the sentinel are populated.
253+
type zip64ExtraValues struct {
254+
found bool
255+
compressedSize uint64
256+
localHeaderOffset uint64
257+
}
258+
259+
// parseZip64ExtraField walks the whole extra-field area of a central
260+
// directory entry looking for the ZIP64 Extended Information field
261+
// (0x0001). The field is not required to come first -- a Unix timestamp or
262+
// NTFS field frequently precedes it -- so the area has to be iterated
263+
// rather than probed at its head.
264+
//
265+
// Within the field the values appear in APPNOTE 4.5.3 order: original
266+
// (uncompressed) size, compressed size, then local header offset. Each is
267+
// present only when the matching central directory field holds the
268+
// 0xFFFFFFFF sentinel, so the uncompressed size has to be stepped over even
269+
// though nothing here consumes it -- reading the compressed size first
270+
// would hand back the wrong value for any entry where the two differ.
271+
func parseZip64ExtraField(extraFields []byte, h CDFileHeader) (zip64ExtraValues, error) {
272+
var values zip64ExtraValues
273+
274+
for pos := 0; pos+extraFieldHeaderSize <= len(extraFields); {
275+
tag := binary.LittleEndian.Uint16(extraFields[pos:])
276+
size := int(binary.LittleEndian.Uint16(extraFields[pos+2:]))
277+
pos += extraFieldHeaderSize
278+
279+
if size > len(extraFields)-pos {
280+
// A field claiming to run past the end of the area is
281+
// malformed; there is nothing sane to resync to.
282+
return values, errZipFormat
283+
}
284+
285+
if tag != zip64ExternalID {
286+
pos += size
287+
continue
288+
}
289+
290+
body := extraFields[pos : pos+size]
291+
bodyPos := 0
292+
read := func() (uint64, bool) {
293+
const uint64Size = 8
294+
if bodyPos+uint64Size > len(body) {
295+
return 0, false
296+
}
297+
v := binary.LittleEndian.Uint64(body[bodyPos:])
298+
bodyPos += uint64Size
299+
return v, true
300+
}
301+
302+
if h.UncompressedSize == zip64MagicVal {
303+
if _, ok := read(); !ok {
304+
return values, errZipFormat
305+
}
306+
}
307+
if h.CompressedSize == zip64MagicVal {
308+
v, ok := read()
309+
if !ok {
310+
return values, errZipFormat
311+
}
312+
values.compressedSize = v
313+
}
314+
if h.LocalHeaderOffset == zip64MagicVal {
315+
v, ok := read()
316+
if !ok {
317+
return values, errZipFormat
318+
}
319+
values.localHeaderOffset = v
320+
}
321+
322+
values.found = true
323+
return values, nil
324+
}
325+
326+
return values, nil
327+
}
328+
223329
// ReadFileData Read data from file of given length of size.
224330
func (reader Reader) ReadFileData(filename string, index int64, length int64) ([]byte, error) {
225331
fileNameEntry, ok := reader.fileEntries[filename]

sdk/internal/zipstream/segment_writer.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,13 @@ func NewSegmentTDFWriter(expectedSegments int, opts ...Option) SegmentWriter {
3333

3434
base := newBaseWriter(cfg)
3535

36+
centralDir := NewCentralDirectory()
37+
centralDir.MaxNonZip64Value = cfg.MaxNonZip64Value
38+
3639
return &segmentWriter{
3740
baseWriter: base,
3841
metadata: NewSegmentMetadata(expectedSegments, cfg.Now),
39-
centralDir: NewCentralDirectory(),
42+
centralDir: centralDir,
4043
payloadEntry: &FileEntry{
4144
Name: TDFPayloadFileName,
4245
Offset: 0,
@@ -191,11 +194,12 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte,
191194
// Total payload size = header + all data (no data descriptor in this calculation)
192195
totalPayloadSize := headerSize + sw.payloadEntry.CompressedSize
193196

194-
// Decide whether payload descriptor must be ZIP64
195-
const max32 = ^uint32(0)
197+
// Decide whether payload descriptor must be ZIP64. The switch point is
198+
// 2 GiB rather than 4 GiB: see maxNonZip64Value.
199+
maxNonZip64 := sw.config.MaxNonZip64Value
196200
needZip64ForPayload := sw.config.Zip64 == Zip64Always ||
197-
sw.payloadEntry.Size > uint64(max32) ||
198-
sw.payloadEntry.CompressedSize > uint64(max32)
201+
sw.payloadEntry.Size > maxNonZip64 ||
202+
sw.payloadEntry.CompressedSize > maxNonZip64
199203

200204
// 1. Write data descriptor for payload (fail if Zip64Never but required)
201205
if sw.config.Zip64 == Zip64Never && needZip64ForPayload {
@@ -230,7 +234,10 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte,
230234
// 5. Write central directory
231235
sw.centralDir.Offset = totalPayloadSize + uint64(buffer.Len())
232236
// Decide if ZIP64 is needed for central directory/EOCD based on offset or forced mode
233-
needZip64ForCD := needZip64ForPayload || sw.config.Zip64 == Zip64Always || sw.centralDir.Offset > uint64(max32) || len(sw.centralDir.Entries) > int(^uint16(0))
237+
needZip64ForCD := needZip64ForPayload ||
238+
sw.config.Zip64 == Zip64Always ||
239+
sw.centralDir.Offset > maxNonZip64 ||
240+
len(sw.centralDir.Entries) >= zip64MagicVal16
234241
if sw.config.Zip64 == Zip64Never && needZip64ForCD {
235242
return nil, &Error{Op: "finalize", Type: "segment", Err: ErrZip64Required}
236243
}

sdk/internal/zipstream/writer.go

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ var (
6868
ErrNoSegmentZero = errors.New("segment 0 missing; it carries the payload local file header")
6969
ErrInvalidSize = errors.New("invalid size")
7070
ErrZip64Required = errors.New("ZIP64 required but disabled (Zip64Never)")
71+
ErrFieldOverflow = errors.New("value too large for zip field")
7172
)
7273

7374
// Config holds configuration options for writers
@@ -79,6 +80,10 @@ type Config struct {
7980
// Defaults to time.Now; tests inject a pinned clock for
8081
// deterministic ZIP output.
8182
Now func() time.Time
83+
// MaxNonZip64Value is the largest size or offset written into a 32-bit
84+
// zip field before the archive switches to ZIP64. Zero means
85+
// maxNonZip64Value (2 GiB - 1). See WithMaxNonZip64Value.
86+
MaxNonZip64Value uint64
8287
}
8388

8489
// Option is a functional option for configuring writers
@@ -130,13 +135,27 @@ func WithClock(now func() time.Time) Option {
130135
}
131136
}
132137

138+
// WithMaxNonZip64Value lowers the point at which the writer switches to
139+
// ZIP64. This exists as a test seam -- mirroring java-sdk's injectable
140+
// MAX_NON_ZIP64_VALUE -- so the ZIP64 path can be exercised without
141+
// materializing a 2 GiB payload. Production callers should leave it alone;
142+
// zero or a value above the default is ignored.
143+
func WithMaxNonZip64Value(maxValue uint64) Option {
144+
return func(c *Config) {
145+
if maxValue > 0 && maxValue <= maxNonZip64Value {
146+
c.MaxNonZip64Value = maxValue
147+
}
148+
}
149+
}
150+
133151
// defaultConfig returns default configuration
134152
func defaultConfig() *Config {
135153
return &Config{
136-
Zip64: Zip64Auto,
137-
MaxSegments: defaultMaxSegments,
138-
EnableLogging: false,
139-
Now: time.Now,
154+
Zip64: Zip64Auto,
155+
MaxSegments: defaultMaxSegments,
156+
EnableLogging: false,
157+
Now: time.Now,
158+
MaxNonZip64Value: maxNonZip64Value,
140159
}
141160
}
142161

@@ -154,6 +173,12 @@ func applyOptions(opts []Option) *Config {
154173
if cfg.Now == nil {
155174
cfg.Now = time.Now
156175
}
176+
// Same defence for the ZIP64 switch point: an option is free to zero
177+
// the exported field, and zero would mean "switch to ZIP64 for
178+
// everything" to the comparisons in Finalize.
179+
if cfg.MaxNonZip64Value == 0 {
180+
cfg.MaxNonZip64Value = maxNonZip64Value
181+
}
157182
return cfg
158183
}
159184

0 commit comments

Comments
 (0)