diff --git a/cmd/erasure-coding.go b/cmd/erasure-coding.go index 3294458e6..da580d8d6 100644 --- a/cmd/erasure-coding.go +++ b/cmd/erasure-coding.go @@ -45,6 +45,17 @@ func NewErasure(ctx context.Context, dataBlocks, parityBlocks int, blockSize int return e, reedsolomon.ErrInvShardNum } + // blockSize reaches here from FileInfo.Erasure, i.e. from xl.meta, which a + // peer can write, and FileInfo.IsValid() does not check it. A coder with a + // non-positive block size cannot encode or decode anything, so refusing to + // build one loses nothing -- but building one leaves every division by + // e.blockSize downstream (ShardFileSize, ShardFileOffset, and the whole of + // erasure-decode.go) as an integer divide-by-zero. Reject at the single + // point where every Erasure value in the process is constructed. + if blockSize <= 0 { + return e, errInvalidArgument + } + if dataBlocks+parityBlocks > 256 { return e, reedsolomon.ErrMaxShardNum } @@ -125,6 +136,13 @@ func (e *Erasure) ShardFileSize(totalLength int64) int64 { if totalLength == -1 { return -1 } + // NewErasure validates dataBlocks and parityBlocks but not blockSize, and + // the values reach it from FileInfo.Erasure - i.e. from xl.meta, which a + // peer can write. Mirrors the guard on ErasureInfo.ShardFileSize; without + // it a zero block size is an integer divide-by-zero here instead. + if e.blockSize <= 0 || e.dataBlocks <= 0 { + return 0 + } numShards := totalLength / e.blockSize lastBlockSize := totalLength % e.blockSize lastShardSize := ceilFrac(lastBlockSize, int64(e.dataBlocks)) diff --git a/cmd/erasure-metadata.go b/cmd/erasure-metadata.go index e0ef25e4b..47cfe5d29 100644 --- a/cmd/erasure-metadata.go +++ b/cmd/erasure-metadata.go @@ -58,6 +58,14 @@ func (e ErasureInfo) ShardFileSize(totalLength int64) int64 { if totalLength == -1 { return -1 } + // ErasureInfo can arrive zero-valued from an untrusted internode payload: + // CheckParts and VerifyFile hand a wire-supplied FileInfo straight here. + // A zero BlockSize would panic with an integer divide-by-zero, and + // CheckParts evaluates this inside xioutil.WithDeadline - a bare goroutine + // whose panic no recover() can reach, taking the whole process down. + if e.BlockSize <= 0 || e.DataBlocks <= 0 { + return 0 + } numShards := totalLength / e.BlockSize lastBlockSize := totalLength % e.BlockSize lastShardSize := ceilFrac(lastBlockSize, int64(e.DataBlocks)) diff --git a/cmd/erasure-server-pool-rebalance.go b/cmd/erasure-server-pool-rebalance.go index c037f0ddc..e3349ee1e 100644 --- a/cmd/erasure-server-pool-rebalance.go +++ b/cmd/erasure-server-pool-rebalance.go @@ -69,7 +69,11 @@ func (rs *rebalanceStats) update(bucket string, fi FileInfo) { rs.NumVersions++ onDiskSz := int64(0) - if !fi.Deleted { + // DataBlocks comes from xl.meta and only fi.Deleted is checked above, so a + // zero here is an integer divide-by-zero rather than a bad statistic. This + // path does not build an Erasure, so NewErasure's validation does not cover + // it; leave the size at zero for metadata that cannot describe a layout. + if !fi.Deleted && fi.Erasure.DataBlocks > 0 { onDiskSz = fi.Size * int64(fi.Erasure.DataBlocks+fi.Erasure.ParityBlocks) / int64(fi.Erasure.DataBlocks) } rs.Bytes += uint64(onDiskSz) diff --git a/cmd/storage-rest-traversal_test.go b/cmd/storage-rest-traversal_test.go index 49607c408..7f613134d 100644 --- a/cmd/storage-rest-traversal_test.go +++ b/cmd/storage-rest-traversal_test.go @@ -523,3 +523,99 @@ func TestCheckPartsMalformedErasure(t *testing.T) { } } +// TestNegativePartSizeNeverPersists covers the one defect in this family that +// survives the request that created it. +// +// A malicious peer can write metadata carrying a negative part size through +// WriteMetadata or RenameData. AddVersion used to persist PartSizes verbatim, +// after which a *local* heal - which does not pass through the wire guards - +// reads it back, derives a zero expected shard size, and reports every part +// intact. The poison stays on disk and the cluster reports itself healthy. +// +// Both ends are covered: the write funnel refuses to persist it, and the +// verification sinks refuse metadata already on disk. +func TestNegativePartSizeNeverPersists(t *testing.T) { + badFI := FileInfo{ + Volume: "foo", Name: "obj", ModTime: UTCNow(), + VersionID: "00000000-0000-0000-0000-0000000000aa", + Parts: []ObjectPartInfo{{Number: 1, Size: -2}}, + Erasure: ErasureInfo{ + DataBlocks: 2, ParityBlocks: 2, BlockSize: blockSizeV2, + Index: 1, Distribution: []int{1, 2, 3, 4}, + }, + } + if !badFI.IsValid() { + t.Fatal("test bug: the counterexample must satisfy FileInfo.IsValid()") + } + + // The write funnel every version write passes through. + var meta xlMetaV2 + if err := meta.AddVersion(badFI); !errors.Is(err, errFileCorrupt) { + t.Errorf("xlMetaV2.AddVersion persisted a negative part size: got %v, want %v", err, errFileCorrupt) + } + + // The verification sinks, reached by local heals that bypass the wire guards. + restClient := newStorageRESTHTTPServerClient(t) + drive := globalLocalSetDrives[0][0][0].Endpoint().Path + mustWrite(t, filepath.Join(drive, "foo", "poisoned", "part.1"), "truncated") + + storage := globalLocalSetDrives[0][0][0] + if _, err := storage.CheckParts(t.Context(), "foo", "poisoned", badFI); !errors.Is(err, errFileCorrupt) { + t.Errorf("local CheckParts accepted a negative part size: got %v, want %v", err, errFileCorrupt) + } + if _, err := storage.VerifyFile(t.Context(), "foo", "poisoned", badFI); !errors.Is(err, errFileCorrupt) { + t.Errorf("local VerifyFile accepted a negative part size: got %v, want %v", err, errFileCorrupt) + } + + // And still refused over the wire. + if _, err := restClient.CheckParts(t.Context(), "foo", "poisoned", badFI); !errors.Is(err, errFileCorrupt) { + t.Errorf("remote CheckParts accepted a negative part size: got %v, want %v", err, errFileCorrupt) + } +} +// TestShardFileSizeZeroErasure pins the arithmetic guard at the sink, which is +// what actually protects every caller. +// +// Both ShardFileSize methods are covered. They are separate implementations on +// separate types -- ErasureInfo (metadata, reached by CheckParts/VerifyFile) +// and Erasure (the coder, reached by the object layer via NewErasure, which +// validates dataBlocks and parityBlocks but not blockSize) -- and guarding one +// leaves the other divisible by zero. +func TestShardFileSizeZeroErasure(t *testing.T) { + for _, e := range []ErasureInfo{ + {}, + {DataBlocks: 4}, + {BlockSize: blockSizeV2}, + {BlockSize: -1, DataBlocks: -1}, + } { + if got := e.ShardFileSize(1024); got < 0 { + t.Errorf("ShardFileSize(%+v) = %d, want a non-negative size", e, got) + } + if got := e.ShardSize(); got < 0 { + t.Errorf("ShardSize(%+v) = %d, want a non-negative size", e, got) + } + } + + // The coder variant. NewErasure must refuse a non-positive block size at + // construction: guarding ShardFileSize alone would leave ShardFileOffset + // and every division in erasure-decode.go dividing by zero, since they all + // use e.blockSize directly. Erasure is only ever built here, so this single + // point covers all of them. + for _, block := range []int64{0, -1} { + if _, err := NewErasure(t.Context(), 4, 2, block); err == nil { + t.Errorf("NewErasure accepted blockSize=%d; every downstream division by "+ + "e.blockSize then divides by zero", block) + } + } + + // A sane coder still works, and its arithmetic stays non-negative. + coder, err := NewErasure(t.Context(), 4, 2, blockSizeV2) + if err != nil { + t.Fatalf("NewErasure rejected a legitimate configuration: %v", err) + } + if got := coder.ShardFileSize(1024); got < 0 { + t.Errorf("Erasure.ShardFileSize = %d, want non-negative", got) + } + if got := coder.ShardFileOffset(0, 1024, 4096); got < 0 { + t.Errorf("Erasure.ShardFileOffset = %d, want non-negative", got) + } +} diff --git a/cmd/xl-storage-format-v2.go b/cmd/xl-storage-format-v2.go index 7d26b13e5..f3615e187 100644 --- a/cmd/xl-storage-format-v2.go +++ b/cmd/xl-storage-format-v2.go @@ -1590,6 +1590,13 @@ func (x *xlMetaV2) UpdateObjectVersion(fi FileInfo) error { // AddVersion adds a new version func (x *xlMetaV2) AddVersion(fi FileInfo) error { + // Refuse to persist metadata no shard size can be derived from. This is the + // single funnel every version write passes through, so rejecting here keeps + // the poison off disk rather than relying on every reader to cope with it. + if fi.HasNegativePartSize() { + return errFileCorrupt + } + if fi.VersionID == "" { // this means versioning is not yet // enabled or suspend i.e all versions diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 2937b47b9..f3a6231c3 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -2423,6 +2423,12 @@ func (s *xlStorage) CheckParts(ctx context.Context, volume string, path string, return nil, err } + // Already-persisted metadata can carry this even though the boundary now + // refuses it, so the check belongs here too, not only at the wire edge. + if fi.HasNegativePartSize() { + return nil, errFileCorrupt + } + resp := CheckPartsResp{ // By default, all results have an unknown status Results: make([]int, len(fi.Parts)), @@ -3126,6 +3132,12 @@ func (s *xlStorage) VerifyFile(ctx context.Context, volume, path string, fi File } } + // See CheckParts: metadata already on disk can carry this even though the + // boundary now refuses it. + if fi.HasNegativePartSize() { + return nil, errFileCorrupt + } + resp := CheckPartsResp{ // By default, the result is unknown per part Results: make([]int, len(fi.Parts)),