From 429ed7762bde20091659b7c3b3e2beddd1b4068c Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Thu, 3 Sep 2026 12:46:21 -0400 Subject: [PATCH 1/3] fix(sdk): DSPX-4590 default per-segment sizes when a writer omits them segmentSize and encryptedSegmentSize are optional per-segment overrides: manifest.schema.json requires only the integrityInformation defaults, and web-sdk omits the per-segment keys whenever they equal the default, so every web-sdk container over one segment failed to decrypt in go-sdk. Fall back to the manifest defaults in the payload-size computation, WriteTo and ReadAt. Rebased onto #3933 (map ReadAt plaintext offsets from cumulative segment sizes), which rewrote ReadAt's segment lookup from a uniform DefaultSegmentSize stride to a walk over each segment's actual plaintext/ciphertext size -- necessary for the non-uniform segments sdk/experimental/tdf can emit. Reconciling the two surfaced a further bug: resolveSegmentSizes treated a per-segment size of 0 as "omitted, use the default" independently for Size and EncryptedSize, but JSON can't distinguish an omitted key from an explicit 0, and go-sdk's own CreateTDF already writes segmentSize: 0 for the sole segment of an empty-payload TDF (no omitempty on the field). That made an empty TDF round-trip to the wrong payloadSize. resolveSegmentSizes now resolves EncryptedSize first -- its zero value is never ambiguous, since ciphertext can never legitimately be zero bytes -- and disambiguates a zero Size by comparing the resolved EncryptedSize against DefaultEncryptedSegSize rather than assuming Size and EncryptedSize are only ever omitted together. Checking go-sdk, java-sdk and web-sdk's actual manifest-writing source confirmed go-sdk and java-sdk always set both fields together (so a joint-zero assumption happened to hold for them), but web-sdk's lib/tdf3/src/tdf.ts decides whether to omit segmentSize and encryptedSegmentSize with two independent equals-the-default comparisons, not one joint check -- so a joint-zero- only version would have mis-resolved a segment where only one of the two happened to be omitted. The corrected comparison needs no assumption about the cipher's per-segment overhead (nonce/tag size stays out of manifest.go entirely): the overhead is constant across every segment in one manifest, so if the resolved EncryptedSize equals its default, the plaintext size must too, regardless of what that overhead number actually is. Also gives calculateSignature's too-short-ciphertext-for-GMAC error (previously a bare, unclassified error) a proper ErrTampered-wrapped sentinel, consistent with the rest of this file's integrity failures. Verified against opentdf/tests' DSPX-4592-java-underflow branch (adds test_tdfs.py::test_chunky_roundtrip, a 5 MiB round-trip that forces a full-default-sized segment): with platform-ref and otdfctl-ref both pointed at this branch and XT_FORCE_SUPPORTS=chunky, js-encrypt -> go-decrypt passes (js omits per-segment sizes on the full-sized segment; go now defaults them back). The one remaining failure in that run, js-encrypt -> java-decrypt, is java-sdk's own pre-existing GMAC-on-empty- segment bug (DSPX-4589), unrelated to this change. Note on #3933 standalone: without this fix, #3933's cumulative-walk ReadAt uses seg.Size directly, so an omitted (0) per-segment size stalls the plaintext cursor and desyncs the ciphertext offset for every segment after it. Reading a web-sdk multi-segment file then fails with a misleading "tamper detected: failed integrity check on segment hash" instead of main's current (also broken, but at least consistent) "fail to create gmac signature". #3933 should not be merged or relied on standalone for real multi-segment interop until this lands on top of it. The zip64/ZIP64-conformance findings originally bundled with this change (findings 1-6 of the DSPX-4590 investigation) now live in a separate PR stacked on top of this one, since they are independent of the segment- size defaulting fixed here. Signed-off-by: Dave Mihalcik --- sdk/manifest.go | 52 ++++++ sdk/tdf.go | 49 ++++-- sdk/tdf_readat_test.go | 17 +- sdk/tdf_segment_defaults_test.go | 284 +++++++++++++++++++++++++++++++ sdk/tdferrors.go | 2 + 5 files changed, 381 insertions(+), 23 deletions(-) create mode 100644 sdk/tdf_segment_defaults_test.go diff --git a/sdk/manifest.go b/sdk/manifest.go index fc3034fddc..4407b0fddc 100644 --- a/sdk/manifest.go +++ b/sdk/manifest.go @@ -1,5 +1,20 @@ 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 -- deciding whether to omit Size and EncryptedSize independently of +// each other, not as a pair. JSON can't distinguish an omitted key from an +// explicit 0, though, and a segment legitimately can hold zero plaintext +// bytes -- go-sdk's own CreateTDF writes segmentSize: 0 for the sole +// segment of an empty-payload TDF. See IntegrityInformation. +// resolveSegmentSizes for how the two are told apart. type Segment struct { Hash string `json:"hash"` Size int64 `json:"segmentSize"` @@ -19,6 +34,43 @@ type IntegrityInformation struct { Segments []Segment `json:"segments"` } +// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg, +// substituting the manifest-level default for whichever field the writer +// omitted. +// +// EncryptedSize is never ambiguous on its own: ciphertext is never +// legitimately zero-length (there is always at least a nonce and a tag), so +// a raw 0 always means the key was left out because it equals +// DefaultEncryptedSegSize. +// +// Size is ambiguous on its own -- web-sdk decides whether to omit Size and +// EncryptedSize independently (two separate equals-the-default checks, not +// one joint check), and JSON can't tell an omitted key from an explicit 0. +// But the per-segment cipher overhead is constant across every segment in +// one manifest, so whether the resolved EncryptedSize equals the default +// tells us, without knowing that overhead, whether the plaintext size does +// too: if it does, a stated Size of 0 was omitted and really is +// DefaultSegmentSize; if it doesn't, the writer would not have omitted a +// Size equal to the default, so 0 is literal -- a genuinely empty segment, +// which go-sdk's own CreateTDF produces for an empty-payload TDF. +func (i IntegrityInformation) resolveSegmentSizes(seg Segment) (int64, int64, error) { + encryptedSize := seg.EncryptedSize + if encryptedSize == 0 { + encryptedSize = i.DefaultEncryptedSegSize + } + + size := seg.Size + if size == 0 && encryptedSize == i.DefaultEncryptedSegSize { + size = i.DefaultSegmentSize + } + + 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 d8cb8fdcad..57e8463982 100644 --- a/sdk/tdf.go +++ b/sdk/tdf.go @@ -879,7 +879,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{ @@ -956,18 +963,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 } @@ -1006,9 +1018,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 @@ -1056,6 +1068,11 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen var segStart int64 // plaintext offset of seg startIndex := int64(-1) // offset of the request within decryptedBuf for _, seg := range r.manifest.Segments { + segSize, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) + if err != nil { + return 0, err + } + // Segment.Size positions every plaintext offset derived below -- // including for the segments this request skips over -- but nothing // authenticates it: the root signature aggregates only Segment.Hash. @@ -1065,18 +1082,18 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen // is the per-segment form of the check doPayloadKeyUnwrap already // applies to the manifest defaults. Deriving Size from EncryptedSize // rather than the reverse keeps the arithmetic from overflowing. - if seg.EncryptedSize < gcmIvSize+aesBlockSize || seg.Size != seg.EncryptedSize-(gcmIvSize+aesBlockSize) { + if encryptedSegSize < gcmIvSize+aesBlockSize || segSize != encryptedSegSize-(gcmIvSize+aesBlockSize) { return 0, fmt.Errorf("%w: segment declares size %d with encrypted size %d", - ErrSegSizeMismatch, seg.Size, seg.EncryptedSize) + ErrSegSizeMismatch, segSize, encryptedSegSize) } - segEnd := segStart + seg.Size + segEnd := segStart + segSize // Wholly before the request. The comparison is <= rather than < so // that a request starting exactly on a segment boundary, or a // zero-length request, does not pull in the preceding segment. if segEnd <= offset { - payloadReadOffset += seg.EncryptedSize + payloadReadOffset += encryptedSegSize segStart = segEnd continue } @@ -1090,12 +1107,12 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen startIndex = offset - segStart } - 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 } @@ -1128,7 +1145,7 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen return 0, errWriteFailed } - payloadReadOffset += seg.EncryptedSize + payloadReadOffset += encryptedSegSize segStart = segEnd } @@ -1553,7 +1570,7 @@ func calculateSignature(data []byte, secret []byte, alg IntegrityAlgorithm, isLe return string(hmac), nil } if kGMACPayloadLength > len(data) { - return "", errors.New("fail to create gmac signature") + return "", fmt.Errorf("%w: ciphertext length=%d", ErrGMACSignatureFailed, len(data)) } if isLegacyTDF { diff --git a/sdk/tdf_readat_test.go b/sdk/tdf_readat_test.go index 383c85d0d9..0ea1f0569b 100644 --- a/sdk/tdf_readat_test.go +++ b/sdk/tdf_readat_test.go @@ -227,21 +227,24 @@ func TestReaderReadAtNonUniformEdges(t *testing.T) { // its own to catch it. func TestReaderReadAtDeclaredSizeMismatch(t *testing.T) { for _, tc := range []struct { - name string - mutate func(segments []Segment) + name string + mutate func(segments []Segment) + wantErr error }{ // Understating the first segment shifts every later segment down by // five bytes. The read below starts past that segment, so it is skipped // and never decrypted. - {"understated", func(segments []Segment) { segments[0].Size = 5 }}, - {"overstated", func(segments []Segment) { segments[0].Size = 40 }}, + {"understated", func(segments []Segment) { segments[0].Size = 5 }, ErrSegSizeMismatch}, + {"overstated", func(segments []Segment) { segments[0].Size = 40 }, ErrSegSizeMismatch}, // Sizes that sum back to something plausible: payloadSize is the sum of // every Size, so a pair that overflows to a small positive total gets - // past the range check on offset and reaches the segment walk. + // past the range check on offset and reaches the segment walk. A + // negative declared Size is caught by resolveSegmentSizes itself, + // before the arithmetic consistency check below it ever runs. {"negative", func(segments []Segment) { segments[0].Size = math.MinInt64 + 1 segments[1].Size = math.MinInt64 + 7 - }}, + }, ErrSegSizeUnresolved}, } { t.Run(tc.name, func(t *testing.T) { reader, _ := newNonUniformReader(t, []int{10, 10, 10}) @@ -258,7 +261,7 @@ func TestReaderReadAtDeclaredSizeMismatch(t *testing.T) { // reader that trusted Size would report a full 20 bytes of shifted // plaintext rather than an error. n, err := reader.ReadAt(make([]byte, 20), 5) - require.ErrorIs(t, err, ErrSegSizeMismatch) + require.ErrorIs(t, err, tc.wantErr) assert.Zero(t, n) }) } diff --git a/sdk/tdf_segment_defaults_test.go b/sdk/tdf_segment_defaults_test.go new file mode 100644 index 0000000000..5395cfae55 --- /dev/null +++ b/sdk/tdf_segment_defaults_test.go @@ -0,0 +1,284 @@ +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 asserts that a TDF with +// per-segment sizes omitted -- legal per manifest.schema.json whenever they +// equal the manifest-level defaults, and what web-sdk emits for every +// full-width segment -- still decrypts correctly via both WriteTo and +// ReadAt. +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) + }) +} + +// zeroLenOKReader wraps an empty *bytes.Reader so a zero-length Read reports +// (0, nil) rather than (0, io.EOF). bytes.Reader reports EOF on any Read once +// exhausted, including a zero-length one, which trips CreateTDFContext's +// segment-read loop for a genuinely empty payload before segment sizing is +// ever reached -- a separate, pre-existing quirk this test works around +// rather than exercises. +type zeroLenOKReader struct { + *bytes.Reader +} + +func (r zeroLenOKReader) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + return r.Reader.Read(p) +} + +// Test_EmptyPayloadRoundTrip asserts that an empty-payload TDF -- whose sole +// segment has segmentSize: 0 in the manifest (no omitempty on the field), +// indistinguishable on the wire from a segment that omitted the key to +// inherit a non-zero manifest-level default -- round-trips to a payloadSize +// of 0, not the default segment size. +func (s *TDFSuite) Test_EmptyPayloadRoundTrip() { + kasInfoList := make([]KASInfo, len(s.kases)) + for i, ki := range s.kases { + kasInfoList[i] = ki.KASInfo + kasInfoList[i].PublicKey = "" + } + kasInfoList[0].Default = true + + tdfBuf := &bytes.Buffer{} + _, err := s.sdk.CreateTDF(tdfBuf, zeroLenOKReader{bytes.NewReader(nil)}, WithKasInformation(kasInfoList...)) + s.Require().NoError(err) + + r, err := s.sdk.LoadTDF(bytes.NewReader(tdfBuf.Bytes())) + s.Require().NoError(err) + s.Require().Equal(int64(0), r.payloadSize) + + decrypted := &bytes.Buffer{} + n, err := io.Copy(decrypted, r) + s.Require().NoError(err) + s.Require().Equal(int64(0), n) +} + +// TestResolveSegmentSizes covers the fallback rules directly: EncryptedSize +// defaults whenever it is zero, Size's ambiguous zero is resolved by +// comparing the (possibly already-defaulted) EncryptedSize against its own +// default rather than by whether Size and EncryptedSize were omitted +// together, and a legitimate zero-length segment is preserved rather than +// defaulted. +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, + }, + { + // web-sdk decides whether to omit Size and EncryptedSize + // independently of each other -- an explicit Size with an + // omitted EncryptedSize is unusual but not itself contradictory, + // so Size is trusted as given and EncryptedSize falls back to + // the default on its own. + name: "Size explicit, EncryptedSize omitted, independently", + integrity: defaults, + segment: Segment{Size: 7}, + wantSize: 7, + wantEncryptedSize: 1052, + }, + { + name: "explicit zero-length segment is legal", + integrity: defaults, + segment: Segment{Size: 0, EncryptedSize: 28}, + wantSize: 0, + wantEncryptedSize: 28, + }, + { + // Size omitted (0) while EncryptedSize is given explicitly as + // exactly the default: the disambiguation compares EncryptedSize + // against DefaultEncryptedSegSize, not against seg.EncryptedSize + // being zero, so this resolves the same as full omission would. + name: "Size omitted, EncryptedSize explicit but equal to its default", + integrity: defaults, + segment: Segment{Size: 0, EncryptedSize: 1052}, + wantSize: 1024, + 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..95d8ebf38a 100644 --- a/sdk/tdferrors.go +++ b/sdk/tdferrors.go @@ -15,7 +15,9 @@ 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) + ErrGMACSignatureFailed = fmt.Errorf("[%w] tdf: ciphertext too short for a gmac signature", ErrTampered) ErrTDFPayloadReadFail = fmt.Errorf("[%w] tdf: fail to read payload from tdf", ErrTampered) ErrTDFPayloadInvalidOffset = fmt.Errorf("[%w] sdk.Reader.ReadAt: negative offset", ErrTampered) ErrRootSignatureFailure = fmt.Errorf("[%w] tdf: issue verifying root signature", ErrTampered) From 45c3196776d7058633c48f3d38f8a23b2ef1d0b4 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Fri, 4 Sep 2026 14:32:44 -0400 Subject: [PATCH 2/3] chore: simplify comments Signed-off-by: Dave Mihalcik --- sdk/manifest.go | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/sdk/manifest.go b/sdk/manifest.go index 4407b0fddc..1497c758e4 100644 --- a/sdk/manifest.go +++ b/sdk/manifest.go @@ -4,17 +4,10 @@ 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 -- deciding whether to omit Size and EncryptedSize independently of -// each other, not as a pair. JSON can't distinguish an omitted key from an -// explicit 0, though, and a segment legitimately can hold zero plaintext -// bytes -- go-sdk's own CreateTDF writes segmentSize: 0 for the sole -// segment of an empty-payload TDF. See IntegrityInformation. -// resolveSegmentSizes for how the two are told apart. +// Size and EncryptedSize are optional in the wire format. +// If absent, use the default sizes. +// Since our JSON parser doesn't distinguish an omitted key from an +// explicit 0, always check both (EncryptedSize is never 0). type Segment struct { Hash string `json:"hash"` Size int64 `json:"segmentSize"` @@ -34,7 +27,7 @@ type IntegrityInformation struct { Segments []Segment `json:"segments"` } -// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg, +// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg in bytes, // substituting the manifest-level default for whichever field the writer // omitted. // @@ -43,16 +36,9 @@ type IntegrityInformation struct { // a raw 0 always means the key was left out because it equals // DefaultEncryptedSegSize. // -// Size is ambiguous on its own -- web-sdk decides whether to omit Size and -// EncryptedSize independently (two separate equals-the-default checks, not -// one joint check), and JSON can't tell an omitted key from an explicit 0. -// But the per-segment cipher overhead is constant across every segment in -// one manifest, so whether the resolved EncryptedSize equals the default -// tells us, without knowing that overhead, whether the plaintext size does -// too: if it does, a stated Size of 0 was omitted and really is -// DefaultSegmentSize; if it doesn't, the writer would not have omitted a -// Size equal to the default, so 0 is literal -- a genuinely empty segment, -// which go-sdk's own CreateTDF produces for an empty-payload TDF. +// Size is ambiguous on its own. For example, web-sdk decides emits Size and +// EncryptedSize only when they are not the default size (128 and 128+28 for AES-GCM-256). +// This determines the correct plaintext and ciphertext based on that understanding. func (i IntegrityInformation) resolveSegmentSizes(seg Segment) (int64, int64, error) { encryptedSize := seg.EncryptedSize if encryptedSize == 0 { From 4004a61fa173971dc2cb5ac460656470f1d9a066 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Tue, 8 Sep 2026 10:29:58 -0400 Subject: [PATCH 3/3] fix(sdk): validate segment size invariant in WriteTo WriteTo trusted resolveSegmentSizes' declared Size without checking it against EncryptedSize, unlike ReadAt's existing guard. A manifest with a Size that disagrees with EncryptedSize could let decryptedDataOffset run ahead of the actual decrypted length, panicking on writeBuf[offset:] once a later segment landed mid-request. Add the same invariant check ReadAt already performs, and move it into resolveSegmentSizes itself so all three call sites (the payload-size sum in LoadTDF, WriteTo, and ReadAt) enforce it identically instead of duplicating the same four-line check twice. This closes a gap where LoadTDF's payload-size computation did not validate the invariant at all -- a manifest inconsistent enough for ReadAt/WriteTo to reject could still "successfully" load with a wrong payloadSize, only failing later at read time. Manifest-level defaults are now checked the same way as explicit per-segment fields, so tampering with the defaults themselves (reachable once per-segment fields are omitted) is caught too. Also: - Fixes resolveSegmentSizes' doc comment, which cited a bogus example default (128 bytes, matching neither go-sdk's nor web-sdk's actual segment-size defaults) and had a grammar error. - Fixes the Segment doc comment, which claimed "EncryptedSize is never 0" while the code three lines below explicitly checks for that case; reworded to clarify it means the wire-level value is never legitimately zero, not that the Go field itself never reads as 0. - Reworks ErrSegSizeUnresolved's message, which said "missing from manifest" but also fires for a present, negative size. - Adds WriteTo coverage to TestReaderReadAtDeclaredSizeMismatch: the invariant check added here had no test exercising it via WriteTo. - Adds Test_TamperedManifestDefaultsRejected, an end-to-end test tampering the manifest-level defaults (rather than any per-segment field) on a TDF with omitted per-segment sizes. - Adds TestCalculateSignatureGMACShortCiphertext, covering the ErrGMACSignatureFailed path introduced two commits back, which had no test. - Adds two more resolveSegmentSizes table cases covering inconsistent explicit sizes and inconsistent manifest-level defaults. Signed-off-by: Dave Mihalcik --- sdk/manifest.go | 36 +++++-- sdk/tdf.go | 27 +++-- sdk/tdf_helpers_test.go | 17 ++++ sdk/tdf_readat_test.go | 43 +++++--- sdk/tdf_segment_defaults_test.go | 164 +++++++++++++++++++++++-------- sdk/tdferrors.go | 2 +- 6 files changed, 210 insertions(+), 79 deletions(-) diff --git a/sdk/manifest.go b/sdk/manifest.go index 1497c758e4..1109cf096b 100644 --- a/sdk/manifest.go +++ b/sdk/manifest.go @@ -4,10 +4,12 @@ import "fmt" // Segment describes one chunk of the payload. // -// Size and EncryptedSize are optional in the wire format. -// If absent, use the default sizes. -// Since our JSON parser doesn't distinguish an omitted key from an -// explicit 0, always check both (EncryptedSize is never 0). +// Size and EncryptedSize are optional in the wire format: a writer may omit +// either key whenever its value equals the corresponding manifest-level +// default. Since JSON can't distinguish an omitted key from an explicit 0, +// a zero EncryptedSize always means "omitted" -- ciphertext is never +// legitimately zero bytes -- but a zero Size is ambiguous; see +// resolveSegmentSizes for how it's disambiguated. type Segment struct { Hash string `json:"hash"` Size int64 `json:"segmentSize"` @@ -27,18 +29,28 @@ type IntegrityInformation struct { Segments []Segment `json:"segments"` } -// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg in bytes, -// substituting the manifest-level default for whichever field the writer -// omitted. +// resolveSegmentSizes returns the plaintext and ciphertext sizes of seg in +// bytes, substituting the manifest-level default for whichever field the +// writer omitted, and validates that the resolved pair is internally +// consistent. // // EncryptedSize is never ambiguous on its own: ciphertext is never // legitimately zero-length (there is always at least a nonce and a tag), so // a raw 0 always means the key was left out because it equals // DefaultEncryptedSegSize. // -// Size is ambiguous on its own. For example, web-sdk decides emits Size and -// EncryptedSize only when they are not the default size (128 and 128+28 for AES-GCM-256). -// This determines the correct plaintext and ciphertext based on that understanding. +// Size is ambiguous on its own: web-sdk, for example, omits Size and +// EncryptedSize independently of each other, each time its own value equals +// the manifest-level default -- so a zero Size doesn't necessarily mean +// EncryptedSize was omitted too. Disambiguate by comparing the resolved +// EncryptedSize against its own default instead. +// +// AES-GCM frames every segment with a fixed-size nonce and tag, so the +// plaintext size is pinned by the ciphertext size regardless of which +// fields the manifest declared explicitly. A resolved pair that disagrees +// with that framing is rejected here -- whether the inconsistency came from +// the per-segment fields or the manifest-level defaults -- rather than left +// for a caller to discover downstream. func (i IntegrityInformation) resolveSegmentSizes(seg Segment) (int64, int64, error) { encryptedSize := seg.EncryptedSize if encryptedSize == 0 { @@ -54,6 +66,10 @@ func (i IntegrityInformation) resolveSegmentSizes(seg Segment) (int64, int64, er return 0, 0, fmt.Errorf("%w: segmentSize=%d encryptedSegmentSize=%d", ErrSegSizeUnresolved, size, encryptedSize) } + if encryptedSize < gcmIvSize+aesBlockSize || size != encryptedSize-(gcmIvSize+aesBlockSize) { + return 0, 0, fmt.Errorf("%w: segment declares size %d with encrypted size %d", ErrSegSizeMismatch, size, encryptedSize) + } + return size, encryptedSize, nil } diff --git a/sdk/tdf.go b/sdk/tdf.go index 57e8463982..6cdcfa31b4 100644 --- a/sdk/tdf.go +++ b/sdk/tdf.go @@ -963,6 +963,11 @@ func (r *Reader) WriteTo(writer io.Writer) (int64, error) { var payloadReadOffset int64 var decryptedDataOffset int64 for _, seg := range r.manifest.Segments { + // resolveSegmentSizes rejects a declared Size that disagrees with + // EncryptedSize; without that check here too, decryptedDataOffset + // could run ahead of the actual decrypted length, panicking on + // writeBuf[offset:] below once a later segment's slice runs shorter + // than expected. segSize, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) if err != nil { return totalBytes, err @@ -1068,23 +1073,17 @@ func (r *Reader) ReadAt(buf []byte, offset int64) (int, error) { //nolint:funlen var segStart int64 // plaintext offset of seg startIndex := int64(-1) // offset of the request within decryptedBuf for _, seg := range r.manifest.Segments { - segSize, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) - if err != nil { - return 0, err - } - // Segment.Size positions every plaintext offset derived below -- // including for the segments this request skips over -- but nothing // authenticates it: the root signature aggregates only Segment.Hash. - // AES-GCM frames each segment with a fixed-size nonce and tag, so the - // plaintext size is pinned by the ciphertext size, and ReadPayload - // below checks EncryptedSize against the bytes actually present. This - // is the per-segment form of the check doPayloadKeyUnwrap already - // applies to the manifest defaults. Deriving Size from EncryptedSize - // rather than the reverse keeps the arithmetic from overflowing. - if encryptedSegSize < gcmIvSize+aesBlockSize || segSize != encryptedSegSize-(gcmIvSize+aesBlockSize) { - return 0, fmt.Errorf("%w: segment declares size %d with encrypted size %d", - ErrSegSizeMismatch, segSize, encryptedSegSize) + // resolveSegmentSizes pins the plaintext size to the ciphertext size + // (AES-GCM frames each segment with a fixed-size nonce and tag), and + // ReadPayload below checks EncryptedSize against the bytes actually + // present. This is the per-segment form of the check + // doPayloadKeyUnwrap already applies to the manifest defaults. + segSize, encryptedSegSize, err := r.manifest.resolveSegmentSizes(seg) + if err != nil { + return 0, err } segEnd := segStart + segSize diff --git a/sdk/tdf_helpers_test.go b/sdk/tdf_helpers_test.go index eaf9a5095f..da0e43f064 100644 --- a/sdk/tdf_helpers_test.go +++ b/sdk/tdf_helpers_test.go @@ -43,6 +43,23 @@ func TestIntegrityAlgorithmStringMatchesCalculateSignature(t *testing.T) { } } +// GMAC signs by returning the ciphertext's trailing kGMACPayloadLength bytes +// verbatim; a ciphertext shorter than that has no tag to return and must be +// rejected rather than silently truncated or padded. +func TestCalculateSignatureGMACShortCiphertext(t *testing.T) { + key := make([]byte, kKeySize) + _, err := rand.Read(key) + require.NoError(t, err) + + data := make([]byte, kGMACPayloadLength-1) + _, err = rand.Read(data) + require.NoError(t, err) + + _, err = calculateSignature(data, key, GMAC, false) + require.ErrorIs(t, err, ErrGMACSignatureFailed) + require.ErrorIs(t, err, ErrTampered) +} + func TestCreatePolicyBinding(t *testing.T) { symKey := make([]byte, kKeySize) _, err := rand.Read(symKey) diff --git a/sdk/tdf_readat_test.go b/sdk/tdf_readat_test.go index 0ea1f0569b..4d44022839 100644 --- a/sdk/tdf_readat_test.go +++ b/sdk/tdf_readat_test.go @@ -217,14 +217,17 @@ func TestReaderReadAtNonUniformEdges(t *testing.T) { // TestReaderReadAtDeclaredSizeMismatch checks that a manifest whose declared // segment sizes disagree with the AES-GCM framing is rejected rather than -// trusted. +// trusted, on both ReadAt and WriteTo. // // Nothing authenticates Segment.Size: the root signature aggregates only each -// segment's Hash, and the schema types the size as a bare number. ReadAt derives -// every plaintext offset from those sizes, so an altered Size shifts the mapping -// -- and because a segment before the requested offset is skipped rather than -// decrypted, checking the length that comes back from Decrypt is not enough on -// its own to catch it. +// segment's Hash, and the schema types the size as a bare number. ReadAt and +// WriteTo both derive plaintext offsets from those sizes, so an altered Size +// shifts the mapping -- and because a segment before the requested offset is +// skipped rather than decrypted, checking the length that comes back from +// Decrypt is not enough on its own to catch it. For WriteTo specifically, the +// same drift would otherwise let decryptedDataOffset run ahead of the actual +// decrypted length, panicking on writeBuf[offset:] once a later segment's +// slice comes up short. func TestReaderReadAtDeclaredSizeMismatch(t *testing.T) { for _, tc := range []struct { name string @@ -250,19 +253,33 @@ func TestReaderReadAtDeclaredSizeMismatch(t *testing.T) { reader, _ := newNonUniformReader(t, []int{10, 10, 10}) tc.mutate(reader.manifest.Segments) - // Mirror what LoadTDF derives from the tampered manifest. + // LoadTDF would itself reject this manifest via resolveSegmentSizes + // before a Reader ever exists, so payloadSize is set by hand here to + // unit-test ReadAt/WriteTo against the tampered manifest directly, + // bypassing that earlier rejection. var payloadSize int64 for _, seg := range reader.manifest.Segments { payloadSize += seg.Size } reader.payloadSize = payloadSize - // The request spans the tampered segment and the one after it, so a - // reader that trusted Size would report a full 20 bytes of shifted - // plaintext rather than an error. - n, err := reader.ReadAt(make([]byte, 20), 5) - require.ErrorIs(t, err, tc.wantErr) - assert.Zero(t, n) + t.Run("ReadAt", func(t *testing.T) { + // The request spans the tampered segment and the one after it, so a + // reader that trusted Size would report a full 20 bytes of shifted + // plaintext rather than an error. + n, err := reader.ReadAt(make([]byte, 20), 5) + require.ErrorIs(t, err, tc.wantErr) + assert.Zero(t, n) + }) + + t.Run("WriteTo", func(t *testing.T) { + // The tampered segment is first, so WriteTo hits the same error on + // its very first iteration, before writing any bytes. + var out bytes.Buffer + n, err := reader.WriteTo(&out) + require.ErrorIs(t, err, tc.wantErr) + assert.Zero(t, n) + }) }) } } diff --git a/sdk/tdf_segment_defaults_test.go b/sdk/tdf_segment_defaults_test.go index 5395cfae55..6b2adaec16 100644 --- a/sdk/tdf_segment_defaults_test.go +++ b/sdk/tdf_segment_defaults_test.go @@ -19,16 +19,16 @@ import ( // 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. +// rewriteManifest rewrites a TDF's integrityInformation via mutate, leaving +// the payload and the ciphertext segment hashes untouched, and returns the +// rewritten archive. // // 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) { +// covers the segment hashes, not the JSON encoding, so mutating these fields +// leaves a container that is still internally consistent on the wire -- +// exactly the situation go-sdk has to cope with, whether the mutation comes +// from a writer omitting defaulted fields or from tampering. +func (s *TDFSuite) rewriteManifest(tdfBytes []byte, mutate func(integrityInfo map[string]any)) []byte { s.T().Helper() zipReader, err := zipstream.NewReader(bytes.NewReader(tdfBytes)) @@ -49,25 +49,8 @@ func (s *TDFSuite) stripDefaultSegmentSizes(tdfBytes []byte) ([]byte, int) { 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++ - } + mutate(integrityInfo) rewritten, err := json.Marshal(manifest) s.Require().NoError(err) @@ -86,7 +69,39 @@ func (s *TDFSuite) stripDefaultSegmentSizes(tdfBytes []byte) ([]byte, int) { s.Require().NoError(err) out.Write(final) - return out.Bytes(), stripped + return out.Bytes() +} + +// 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. +func (s *TDFSuite) stripDefaultSegmentSizes(tdfBytes []byte) ([]byte, int) { + s.T().Helper() + + stripped := 0 + rewritten := s.rewriteManifest(tdfBytes, func(integrityInfo map[string]any) { + 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) + + 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++ + } + }) + + return rewritten, stripped } // Test_SegmentSizesOmittedFallBackToDefaults asserts that a TDF with @@ -149,12 +164,57 @@ func (s *TDFSuite) Test_SegmentSizesOmittedFallBackToDefaults() { }) } +// Test_TamperedManifestDefaultsRejected asserts that a TDF whose per-segment +// sizes were omitted (as web-sdk emits) is rejected -- not silently +// misdecrypted -- when the manifest-level defaults it falls back to are +// internally inconsistent with AES-GCM's fixed nonce+tag overhead. This +// covers tampering (or a buggy writer) that targets the defaults themselves +// rather than any individual segment's fields. +func (s *TDFSuite) Test_TamperedManifestDefaultsRejected() { + kasInfoList := make([]KASInfo, len(s.kases)) + for i, ki := range s.kases { + kasInfoList[i] = ki.KASInfo + kasInfoList[i].PublicKey = "" + } + kasInfoList[0].Default = true + + plaintext := make([]byte, webSDKSegmentSize) + for i := range plaintext { + plaintext[i] = byte(i % 251) + } + + 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(1, stripped, "fixture should have one default-sized segment to strip") + + // Inflate the plaintext default by one byte relative to the ciphertext + // default, so the pair no longer differs by exactly the AES-GCM + // nonce+tag overhead -- with every per-segment field already omitted, + // this is the only place left for the inconsistency to live. + tdfBytes = s.rewriteManifest(tdfBytes, func(integrityInfo map[string]any) { + defaultSize, ok := integrityInfo["segmentSizeDefault"].(float64) + s.Require().True(ok) + integrityInfo["segmentSizeDefault"] = defaultSize + 1 + }) + + _, err = s.sdk.LoadTDF(bytes.NewReader(tdfBytes)) + s.Require().ErrorIs(err, ErrSegSizeMismatch) +} + // zeroLenOKReader wraps an empty *bytes.Reader so a zero-length Read reports // (0, nil) rather than (0, io.EOF). bytes.Reader reports EOF on any Read once // exhausted, including a zero-length one, which trips CreateTDFContext's // segment-read loop for a genuinely empty payload before segment sizing is // ever reached -- a separate, pre-existing quirk this test works around -// rather than exercises. +// rather than exercises. In practice this means SDK.CreateTDF cannot itself +// encrypt a genuinely empty io.Reader today (e.g. bytes.NewReader(nil)); that +// gap is unrelated to segment-size defaulting and is not fixed here. type zeroLenOKReader struct { *bytes.Reader } @@ -197,8 +257,10 @@ func (s *TDFSuite) Test_EmptyPayloadRoundTrip() { // defaults whenever it is zero, Size's ambiguous zero is resolved by // comparing the (possibly already-defaulted) EncryptedSize against its own // default rather than by whether Size and EncryptedSize were omitted -// together, and a legitimate zero-length segment is preserved rather than -// defaulted. +// together, a legitimate zero-length segment is preserved rather than +// defaulted, and a resolved pair that disagrees with AES-GCM's fixed framing +// is rejected -- whether the inconsistency comes from explicit per-segment +// fields or from the manifest-level defaults themselves. func TestResolveSegmentSizes(t *testing.T) { defaults := IntegrityInformation{ DefaultSegmentSize: 1024, @@ -211,7 +273,7 @@ func TestResolveSegmentSizes(t *testing.T) { segment Segment wantSize int64 wantEncryptedSize int64 - wantErr bool + wantErrIs error }{ { name: "explicit sizes win", @@ -229,14 +291,14 @@ func TestResolveSegmentSizes(t *testing.T) { }, { // web-sdk decides whether to omit Size and EncryptedSize - // independently of each other -- an explicit Size with an - // omitted EncryptedSize is unusual but not itself contradictory, - // so Size is trusted as given and EncryptedSize falls back to - // the default on its own. + // independently of each other -- an explicit, physically-consistent + // Size with an omitted EncryptedSize is unusual but not itself + // contradictory, so Size is trusted as given and EncryptedSize + // falls back to the default on its own. name: "Size explicit, EncryptedSize omitted, independently", integrity: defaults, - segment: Segment{Size: 7}, - wantSize: 7, + segment: Segment{Size: 1024}, + wantSize: 1024, wantEncryptedSize: 1052, }, { @@ -261,19 +323,39 @@ func TestResolveSegmentSizes(t *testing.T) { name: "no value and no default is an error", integrity: IntegrityInformation{}, segment: Segment{}, - wantErr: true, + wantErrIs: ErrSegSizeUnresolved, }, { name: "negative default is an error", integrity: IntegrityInformation{DefaultSegmentSize: -1, DefaultEncryptedSegSize: -1}, segment: Segment{}, - wantErr: true, + wantErrIs: ErrSegSizeUnresolved, + }, + { + // An explicit Size that doesn't match the AES-GCM framing implied + // by EncryptedSize (whether EncryptedSize is explicit or defaulted) + // is rejected here rather than left for a caller to discover + // downstream. + name: "inconsistent explicit sizes are rejected", + integrity: defaults, + segment: Segment{Size: 7, EncryptedSize: 1052}, + wantErrIs: ErrSegSizeMismatch, + }, + { + // The same consistency check applies to the manifest-level + // defaults themselves, not just explicit per-segment fields -- a + // manifest whose defaults were tampered with is caught the same + // way, even though every per-segment field is omitted. + name: "inconsistent manifest-level defaults are rejected", + integrity: IntegrityInformation{DefaultSegmentSize: 999, DefaultEncryptedSegSize: 1052}, + segment: Segment{}, + wantErrIs: ErrSegSizeMismatch, }, } { t.Run(tc.name, func(t *testing.T) { size, encryptedSize, err := tc.integrity.resolveSegmentSizes(tc.segment) - if tc.wantErr { - require.ErrorIs(t, err, ErrSegSizeUnresolved) + if tc.wantErrIs != nil { + require.ErrorIs(t, err, tc.wantErrIs) return } require.NoError(t, err) diff --git a/sdk/tdferrors.go b/sdk/tdferrors.go index 95d8ebf38a..428b6dbbb2 100644 --- a/sdk/tdferrors.go +++ b/sdk/tdferrors.go @@ -15,7 +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) + ErrSegSizeUnresolved = fmt.Errorf("[%w] tdf: segment size invalid or missing from manifest, with no default to fall back on", ErrTampered) ErrSegSigValidation = fmt.Errorf("[%w] tdf: failed integrity check on segment hash", ErrTampered) ErrGMACSignatureFailed = fmt.Errorf("[%w] tdf: ciphertext too short for a gmac signature", ErrTampered) ErrTDFPayloadReadFail = fmt.Errorf("[%w] tdf: fail to read payload from tdf", ErrTampered)