Fix unbounded inflate parsing binary fragments

gitdiff/apply_binary.go

@@ -49,11 +49,15 @@ func (a *BinaryApplier) ApplyFragment(f *BinaryFragment) error {
 
 	switch f.Method {
 	case BinaryPatchLiteral:
-		if _, err := a.dst.Write(f.Data); err != nil {
+		if _, err := io.Copy(a.dst, f.Data()); err != nil {
 			return applyError(err)
 		}
 	case BinaryPatchDelta:
-		if err := applyBinaryDeltaFragment(a.dst, a.src, f.Data); err != nil {
+		data, err := io.ReadAll(f.Data())
+		if err != nil {
+			return applyError(err)
+		}
+		if err := applyBinaryDeltaFragment(a.dst, a.src, data); err != nil {
 			return applyError(err)
 		}
 	default:

gitdiff/binary.go

@@ -2,10 +2,7 @@ package gitdiff
 
 import (
 	"bytes"
-	"compress/zlib"
-	"fmt"
 	"io"
-	"io/ioutil"
 	"strconv"
 	"strings"
 )
@@ -93,6 +90,9 @@ func (p *parser) ParseBinaryFragmentHeader() (*BinaryFragment, error) {
 		nerr := err.(*strconv.NumError)
 		return nil, p.Errorf(0, "binary patch: invalid size: %v", nerr.Err)
 	}
+	if frag.Size < 0 {
+		return nil, p.Errorf(0, "binary patch: invalid size: %d", frag.Size)
+	}
 
 	if err := p.Next(); err != nil && err != io.EOF {
 		return nil, err
@@ -152,35 +152,11 @@ func (p *parser) ParseBinaryChunk(frag *BinaryFragment) error {
 			return err
 		}
 	}
-
-	if err := inflateBinaryChunk(frag, &data); err != nil {
-		return p.Errorf(0, "binary patch: %v", err)
-	}
+	frag.RawData = data.Bytes()
 
 	// consume the empty line that ended the fragment
 	if err := p.Next(); err != nil && err != io.EOF {
 		return err
 	}
 	return nil
 }
-
-func inflateBinaryChunk(frag *BinaryFragment, r io.Reader) error {
-	zr, err := zlib.NewReader(r)
-	if err != nil {
-		return err
-	}
-
-	data, err := ioutil.ReadAll(zr)
-	if err != nil {
-		return err
-	}
-	if err := zr.Close(); err != nil {
-		return err
-	}
-
-	if int64(len(data)) != frag.Size {
-		return fmt.Errorf("%d byte fragment inflated to %d", frag.Size, len(data))
-	}
-	frag.Data = data
-	return nil
-}

gitdiff/binary_reader.go

@@ -0,0 +1,56 @@
+package gitdiff
+
+import (
+	"bytes"
+	"compress/zlib"
+	"fmt"
+	"io"
+)
+
+// binaryReader reads the compressed raw data in a binary fragment,
+// decompressing it and checking it against the expected size. Clients should
+// read until it returns an error or io.EOF.
+type binaryReader struct {
+	r    *io.LimitedReader
+	c    io.Closer
+	size int64
+	raw  []byte
+}
+
+func newBinaryReader(f *BinaryFragment) io.Reader {
+	return &binaryReader{
+		raw:  f.RawData,
+		size: f.Size,
+	}
+}
+
+func (r *binaryReader) Read(p []byte) (int, error) {
+	// Defer initialization of the zlib reader so that we report any header
+	// errors from Read() instead of when creating the binaryReader. This
+	// allows clients to use io.ReadAll() directly with BinaryFragment.Data().
+	if r.r == nil {
+		zr, err := zlib.NewReader(bytes.NewReader(r.raw))
+		if err != nil {
+			return 0, err
+		}
+		r.r = &io.LimitedReader{R: zr, N: r.size + 1}
+		r.c = zr
+	}
+
+	n, err := r.r.Read(p)
+	if err == io.EOF {
+		// If we reached the "end", first check that we read the correct amount
+		// of data. On an exact read, the limit reader has one byte remaining.
+		switch {
+		case r.r.N > 1:
+			return n, fmt.Errorf("incorrect inflated size: expected %d bytes, received %d", r.size, r.size-(r.r.N-1))
+		case r.r.N < 1:
+			return n, fmt.Errorf("incorrect inflated size: expected %d bytes, received more", r.size)
+		}
+		// Then check that the zlib checksum is valid by closing the reader
+		if err := r.c.Close(); err != nil {
+			return n, err
+		}
+	}
+	return n, err
+}

gitdiff/binary_reader_test.go

@@ -0,0 +1,88 @@
+package gitdiff
+
+import (
+	"bytes"
+	"compress/zlib"
+	"encoding/binary"
+	"io"
+	"strings"
+	"testing"
+)
+
+func TestBinaryReader(t *testing.T) {
+	tests := map[string]struct {
+		Fragment BinaryFragment
+		Data     []byte
+		Err      string
+	}{
+		"fromCompressed": {
+			Fragment: BinaryFragment{
+				Size:    40,
+				RawData: deflateBinaryData(fib(10, binary.BigEndian)),
+			},
+			Data: fib(10, binary.BigEndian),
+		},
+		"fromCompressedGit": {
+			Fragment: BinaryFragment{
+				Size: 40,
+				RawData: []byte{
+					// Literal data as compressed and base85 encoded by Git
+					0x78, 0x01, 0x63, 0x60, 0x60, 0x60, 0x64, 0x80, 0x60, 0x26, 0x20,
+					0xcd, 0x0c, 0xc4, 0xac, 0x40, 0xcc, 0x01, 0xc4, 0xbc, 0x40, 0x2c,
+					0x0a, 0xc4, 0x4a, 0x40, 0x6c, 0x0e, 0x00, 0x04, 0x2b, 0x00, 0x90,
+				},
+			},
+			Data: fib(10, binary.BigEndian),
+		},
+		"invalidCompression": {
+			Fragment: BinaryFragment{
+				Size:    40,
+				RawData: fib(10, binary.BigEndian),
+			},
+			Err: "zlib",
+		},
+		"incorrectSizeOver": {
+			Fragment: BinaryFragment{
+				Size:    16,
+				RawData: deflateBinaryData(fib(10, binary.BigEndian)),
+			},
+			Err: "expected 16 bytes, received more",
+		},
+		"incorrectSizeUnder": {
+			Fragment: BinaryFragment{
+				Size:    16,
+				RawData: deflateBinaryData(fib(2, binary.BigEndian)),
+			},
+			Err: "expected 16 bytes, received 8",
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			data, err := io.ReadAll(test.Fragment.Data())
+			if test.Err == "" && err != nil {
+				t.Fatalf("unexpected data error: %v", err)
+			}
+			if test.Err != "" {
+				if err == nil {
+					t.Fatal("expected data error, but got nil")
+				}
+				if !strings.Contains(err.Error(), test.Err) {
+					t.Fatalf("incorrect data error: %q is not in %q", test.Err, err.Error())
+				}
+			} else if !bytes.Equal(test.Data, data) {
+				t.Errorf("incorrect data\nexpected: %+v (len=%d)\n  actual: %+v (len=%d)", test.Data, len(test.Data), data, len(data))
+			}
+		})
+	}
+}
+
+func deflateBinaryData(data []byte) []byte {
+	var b bytes.Buffer
+
+	zw := zlib.NewWriter(&b)
+	_, _ = zw.Write(data)
+	_ = zw.Close()
+
+	return b.Bytes()
+}

gitdiff/binary_test.go

@@ -1,6 +1,7 @@
 package gitdiff
 
 import (
+	"bytes"
 	"encoding/binary"
 	"io"
 	"reflect"
@@ -122,23 +123,29 @@ func TestParseBinaryFragmentHeader(t *testing.T) {
 }
 
 func TestParseBinaryChunk(t *testing.T) {
+	// NOTE: input data in this test is encoded correctly but is not valid
+	// binary patch content because it is not compressed with zlib. This is not
+	// relevant for the logic under test, but means tests must not call Data()
+	// on the parsed fragments.
+
 	tests := map[string]struct {
 		Input    string
 		Fragment BinaryFragment
 		Output   []byte
 		Err      string
 	}{
 		"singleline": {
-			Input: "TcmZQzU|?i`U?w2V48*Je09XJG\n\n",
+			Input: "T0000100001000020000300005\n\n",
 			Fragment: BinaryFragment{
 				Size: 20,
 			},
 			Output: fib(5, binary.BigEndian),
 		},
 		"multiline": {
-			Input: "zcmZQzU|?i`U?w2V48*KJ%mKu_Kr9NxN<eH5#F0Qe0f=7$l~*z_FeL$%-)3N7vt?l5\n" +
-				"zl3-vE2xVZ9%4J~CI>f->s?WfX|B-=Vs{#X~svra7Ekg#T|4s}nH;WnAZ)|1Y*`&cB\n" +
-				"s(sh?X(Uz6L^!Ou&aF*u`J!eibJifSrv0z>$Q%Hd(^HIJ<Y?5`S0gT5UE&u=k\n\n",
+			Input: "z0000100001000020000300005000080000D0000L0000Y0000t000140001x0002#\n" +
+				"z0004b0007F000Bq000I(000UY000nG000_o001h&002cV003|C006Zh00AWt00G)D\n" +
+				"z00RF)00h}{00-E$01UDy02GSd03kgE05!+r09OR(0F2DZ0OQfH0dSsq0#tA*1H}%a\n" +
+				"D1{r?K\n\n",
 			Fragment: BinaryFragment{
 				Size: 160,
 			},
@@ -161,24 +168,13 @@ func TestParseBinaryChunk(t *testing.T) {
 			Err:   "incorrect byte count",
 		},
 		"invalidEncoding": {
-			Input: "TcmZQzU|?i'U?w2V48*Je09XJG\n",
+			Input: "T00001000010\"0020000300005\n\n",
 			Err:   "invalid base85 byte",
 		},
 		"noTrailingEmptyLine": {
-			Input: "TcmZQzU|?i`U?w2V48*Je09XJG\n",
+			Input: "T0000100001000020000300005\n",
 			Err:   "unexpected EOF",
 		},
-		"invalidCompression": {
-			Input: "F007GV%KiWV\n\n",
-			Err:   "zlib",
-		},
-		"incorrectSize": {
-			Input: "TcmZQzU|?i`U?w2V48*Je09XJG\n\n",
-			Fragment: BinaryFragment{
-				Size: 16,
-			},
-			Err: "16 byte fragment inflated to 20",
-		},
 	}
 
 	for name, test := range tests {
@@ -196,14 +192,19 @@ func TestParseBinaryChunk(t *testing.T) {
 			if err != nil {
 				t.Fatalf("unexpected error parsing binary chunk: %v", err)
 			}
-			if !reflect.DeepEqual(test.Output, frag.Data) {
-				t.Errorf("incorrect binary chunk\nexpected: %+v\n  actual: %+v", test.Output, frag.Data)
+			if !bytes.Equal(test.Output, frag.RawData) {
+				t.Errorf("incorrect binary chunk\nexpected: %+v (len=%d)\n  actual: %+v (len=%d)", test.Output, len(test.Output), frag.RawData, len(frag.RawData))
 			}
 		})
 	}
 }
 
 func TestParseBinaryFragments(t *testing.T) {
+	// NOTE: input data in this test is encoded correctly but is not valid
+	// binary patch content because it is not compressed with zlib. This is not
+	// relevant for the logic under test, but means tests must not call Data()
+	// on the parsed fragments.
+
 	tests := map[string]struct {
 		Input string
 		File  File
@@ -216,35 +217,35 @@ func TestParseBinaryFragments(t *testing.T) {
 		"dataWithReverse": {
 			Input: `GIT binary patch
 literal 40
-gcmZQzU|?i` + "`" + `U?w2V48*KJ%mKu_Kr9NxN<eH500b)lkN^Mx
+n001h&002cV003|C006Zh00AWt00G)D00RF)00h}{00-E$01UDy
 
-literal 0
-HcmV?d00001
+literal 4
+D00001
 
 `,
 			Binary: true,
 			Fragment: &BinaryFragment{
-				Method: BinaryPatchLiteral,
-				Size:   40,
-				Data:   fib(10, binary.BigEndian),
+				Method:  BinaryPatchLiteral,
+				Size:    40,
+				RawData: fibSlice(20, 30, binary.BigEndian),
 			},
 			ReverseFragment: &BinaryFragment{
-				Method: BinaryPatchLiteral,
-				Size:   0,
-				Data:   []byte{},
+				Method:  BinaryPatchLiteral,
+				Size:    4,
+				RawData: fib(1, binary.BigEndian),
 			},
 		},
 		"dataWithoutReverse": {
 			Input: `GIT binary patch
 literal 40
-gcmZQzU|?i` + "`" + `U?w2V48*KJ%mKu_Kr9NxN<eH500b)lkN^Mx
+n001h&002cV003|C006Zh00AWt00G)D00RF)00h}{00-E$01UDy
 
 `,
 			Binary: true,
 			Fragment: &BinaryFragment{
-				Method: BinaryPatchLiteral,
-				Size:   40,
-				Data:   fib(10, binary.BigEndian),
+				Method:  BinaryPatchLiteral,
+				Size:    40,
+				RawData: fibSlice(20, 30, binary.BigEndian),
 			},
 		},
 		"noData": {
@@ -273,10 +274,10 @@ TcmZQzU|?i'U?w2V48*Je09XJG
 		"invalidReverseData": {
 			Input: `GIT binary patch
 literal 20
-TcmZQzU|?i` + "`" + `U?w2V48*Je09XJG
+T00001000010"0020000300005
 
-literal 0
-zcmV?d00001
+literal 4
+z00001
 
 `,
 			Err: true,
@@ -311,6 +312,11 @@ zcmV?d00001
 	}
 }
 
+func fibSlice(start, end int, ord binary.ByteOrder) []byte {
+	buf := fib(end, ord)
+	return buf[4*start:]
+}
+
 func fib(n int, ord binary.ByteOrder) []byte {
 	buf := make([]byte, 4*n)
 	for i := 0; i < len(buf); i += 4 {