[release-branch.go1.14-security] encoding/binary: read at most MaxVarintLen64 bytes...

This CL ensures that ReadUvarint consumes only a limited
amount of input (instead of an unbounded amount).

On some inputs, ReadUvarint could read an arbitrary number
of bytes before deciding to return an overflow error.
After this CL, ReadUvarint returns that same overflow
error sooner, after reading at most MaxVarintLen64 bytes.

Fix authored by Robert Griesemer and Filippo Valsorda.

Thanks to Diederik Loerakker, Jonny Rhea, Raúl Kripalani,
and Preston Van Loon for reporting this.

Fixes CVE-2020-16845

Change-Id: Ie0cb15972f14c38b7cf7af84c45c4ce54909bb8f
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/812099
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/812326

diff --git a/src/encoding/binary/varint.go b/src/encoding/binary/varint.go
index bcb8ac9a459c5c20ceefe3e9ad9f69e5177b9c83..38af61075c860ef6b97b883bc6be2508a1c914f1 100644 (file)
--- a/src/encoding/binary/varint.go
+++ b/src/encoding/binary/varint.go
@@ -106,13 +106,13 @@ var overflow = errors.New("binary: varint overflows a 64-bit integer")
 func ReadUvarint(r io.ByteReader) (uint64, error) {
        var x uint64
        var s uint
-       for i := 0; ; i++ {
+       for i := 0; i < MaxVarintLen64; i++ {
                b, err := r.ReadByte()
                if err != nil {
                        return x, err
                }
                if b < 0x80 {
-                       if i > 9 || i == 9 && b > 1 {
+                       if i == 9 && b > 1 {
                                return x, overflow
                        }
                        return x | uint64(b)<<s, nil
@@ -120,6 +120,7 @@ func ReadUvarint(r io.ByteReader) (uint64, error) {
                x |= uint64(b&0x7f) << s
                s += 7
        }
+       return x, overflow
 }
 
 // ReadVarint reads an encoded signed integer from r and returns it as an int64.

diff --git a/src/encoding/binary/varint_test.go b/src/encoding/binary/varint_test.go
index ca411ecbd65e8180d2bfcb0c819a561328d66f06..6ef4c9950525a45c56eb7a11c5bb2ca443693b09 100644 (file)
--- a/src/encoding/binary/varint_test.go
+++ b/src/encoding/binary/varint_test.go
@@ -121,21 +121,27 @@ func TestBufferTooSmall(t *testing.T) {
        }
 }
 
-func testOverflow(t *testing.T, buf []byte, n0 int, err0 error) {
+func testOverflow(t *testing.T, buf []byte, x0 uint64, n0 int, err0 error) {
        x, n := Uvarint(buf)
        if x != 0 || n != n0 {
                t.Errorf("Uvarint(%v): got x = %d, n = %d; want 0, %d", buf, x, n, n0)
        }
 
-       x, err := ReadUvarint(bytes.NewReader(buf))
-       if x != 0 || err != err0 {
-               t.Errorf("ReadUvarint(%v): got x = %d, err = %s; want 0, %s", buf, x, err, err0)
+       r := bytes.NewReader(buf)
+       len := r.Len()
+       x, err := ReadUvarint(r)
+       if x != x0 || err != err0 {
+               t.Errorf("ReadUvarint(%v): got x = %d, err = %s; want %d, %s", buf, x, err, x0, err0)
+       }
+       if read := len - r.Len(); read > MaxVarintLen64 {
+               t.Errorf("ReadUvarint(%v): read more than MaxVarintLen64 bytes, got %d", buf, read)
        }
 }
 
 func TestOverflow(t *testing.T) {
-       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x2}, -10, overflow)
-       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x1, 0, 0}, -13, overflow)
+       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x2}, 0, -10, overflow)
+       testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x1, 0, 0}, 0, -13, overflow)
+       testOverflow(t, []byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 1<<64-1, 0, overflow) // 11 bytes, should overflow
 }
 
 func TestNonCanonicalZero(t *testing.T) {