From 100a510097e600ee542e1ca2c3162ef136cb9bdc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michael=20Mur=C3=A9?= <michael.mure@consensys.net>
Date: Wed, 9 Oct 2024 18:38:35 +0200
Subject: [PATCH] pkg/container: harden the CAR file round-trip with fuzzing

---
 pkg/container/car.go      | 14 ++++++++++++--
 pkg/container/car_test.go | 36 +++++++++++++++++++++++++++++++-----
 pkg/container/writer.go   |  4 ++--
 3 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/pkg/container/car.go b/pkg/container/car.go
index ee6d18c..ed39a96 100644
--- a/pkg/container/car.go
+++ b/pkg/container/car.go
@@ -37,7 +37,7 @@ type carBlock struct {
 // writeCar writes a CARv1 file containing the blocks from the iterator.
 // If no roots are provided, a single EmptyCid is used as root to make the file
 // spec compliant.
-func writeCar(w io.Writer, roots []cid.Cid, blocks iter.Seq[carBlock]) error {
+func writeCar(w io.Writer, roots []cid.Cid, blocks iter.Seq2[carBlock, error]) error {
 	if len(roots) == 0 {
 		roots = []cid.Cid{EmptyCid}
 	}
@@ -54,7 +54,10 @@ func writeCar(w io.Writer, roots []cid.Cid, blocks iter.Seq[carBlock]) error {
 		return err
 	}
 
-	for block := range blocks {
+	for block, err := range blocks {
+		if err != nil {
+			return err
+		}
 		err = ldWrite(w, block.c.Bytes(), block.data)
 		if err != nil {
 			return err
@@ -144,6 +147,9 @@ func ldRead(r *bufio.Reader) ([]byte, error) {
 		}
 		return nil, err
 	}
+	if l == 0 {
+		return nil, fmt.Errorf("invalid zero size section")
+	}
 
 	if l > uint64(maxAllowedSectionSize) { // Don't OOM
 		return nil, fmt.Errorf("malformed car; header is bigger than MaxAllowedSectionSize")
@@ -151,6 +157,10 @@ func ldRead(r *bufio.Reader) ([]byte, error) {
 
 	buf := make([]byte, l)
 	if _, err := io.ReadFull(r, buf); err != nil {
+		if err == io.EOF {
+			// we should be able to read the promised bytes, this is not normal
+			return nil, io.ErrUnexpectedEOF
+		}
 		return nil, err
 	}
 
diff --git a/pkg/container/car_test.go b/pkg/container/car_test.go
index 9b0ef31..80f4b5d 100644
--- a/pkg/container/car_test.go
+++ b/pkg/container/car_test.go
@@ -26,9 +26,9 @@ func TestCarRoundTrip(t *testing.T) {
 
 	buf := bytes.NewBuffer(nil)
 
-	err = writeCar(buf, roots, func(yield func(carBlock) bool) {
+	err = writeCar(buf, roots, func(yield func(carBlock, error) bool) {
 		for _, blk := range blks {
-			if !yield(blk) {
+			if !yield(blk, nil) {
 				return
 			}
 		}
@@ -39,14 +39,40 @@ func TestCarRoundTrip(t *testing.T) {
 	require.Equal(t, original, buf.Bytes())
 }
 
-func FuzzCarRead(f *testing.F) {
+func FuzzCarRoundTrip(f *testing.F) {
 	example, err := os.ReadFile("testdata/sample-v1.car")
 	require.NoError(f, err)
 
 	f.Add(example)
 
 	f.Fuzz(func(t *testing.T, data []byte) {
-		_, _, _ = readCar(bytes.NewReader(data))
-		// only looking for panics
+		roots, blocksIter, err := readCar(bytes.NewReader(data))
+		if err != nil {
+			// skip invalid binary
+			t.Skip()
+		}
+
+		// reading all the blocks, which force reading and verifying the full file
+		var blocks []carBlock
+		for block, err := range blocksIter {
+			if err != nil {
+				// error reading, invalid data
+				t.Skip()
+			}
+			blocks = append(blocks, block)
+		}
+
+		var buf bytes.Buffer
+		err = writeCar(&buf, roots, func(yield func(carBlock, error) bool) {
+			for _, blk := range blocks {
+				if !yield(blk, nil) {
+					return
+				}
+			}
+		})
+		require.NoError(t, err)
+
+		// test if the round-trip produce a byte-equal CAR
+		require.Equal(t, data, buf.Bytes())
 	})
 }
diff --git a/pkg/container/writer.go b/pkg/container/writer.go
index cec6675..310fa8a 100644
--- a/pkg/container/writer.go
+++ b/pkg/container/writer.go
@@ -27,9 +27,9 @@ func (ctn Writer) AddSealed(cid cid.Cid, data []byte) {
 }
 
 func (ctn Writer) ToCar(w io.Writer) error {
-	return writeCar(w, nil, func(yield func(carBlock) bool) {
+	return writeCar(w, nil, func(yield func(carBlock, error) bool) {
 		for c, bytes := range ctn {
-			if !yield(carBlock{c: c, data: bytes}) {
+			if !yield(carBlock{c: c, data: bytes}, nil) {
 				return
 			}
 		}