fix(storage): reject unusable erasure metadata at every sink

Malformed erasure layouts can divide by zero, while negative part sizes collapse expected shard sizes to zero and make truncated data appear healthy. Boundary validation alone is insufficient because poisoned metadata may already exist on disk or arrive through local heal paths.

Reject non-positive block sizes at the sole Erasure constructor, guard the metadata arithmetic helpers and rebalance calculation, refuse negative part sizes before persistence, and make CheckParts and VerifyFile reject previously stored poison. Tests cover both shard-size implementations, construction, persistence, local verification, and the wire boundary.

Co-authored-by: ChatGPT <noreply@openai.com>
Co-authored-by: Claude <noreply@anthropic.com>

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))

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))

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)

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)
+	}
+}

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

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)),