From: Emmanuel T Odeke <emmanuel@orijtech.com>
Date: Sun, 7 Mar 2021 07:14:21 +0000 (-0800)
Subject: encoding/binary: limit bytes read by Uvarint to <= 10
X-Git-Tag: go1.17beta1~1238
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=aafad20b617ee63d58fcd4f6e0d98fe27760678c;p=gostls13.git

encoding/binary: limit bytes read by Uvarint to <= 10

Limits the number of bytes that can be consumed by Uvarint
to MaxVarintLen64 (10) to avoid wasted computations.
With this change, if Uvarint reads more than MaxVarintLen64
bytes, it'll return the erroring byte count of n=-(MaxVarintLen64+1)
which is -11, as per the function signature.

Updated some tests to reflect the new change in expectations of n
when the number of bytes to be read exceeds the limits..

Fixes #41185

Change-Id: Ie346457b1ddb0214b60c72e81128e24d604d083d
Reviewed-on: https://go-review.googlesource.com/c/go/+/299531
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
---

diff --git a/src/encoding/binary/varint.go b/src/encoding/binary/varint.go
index 1fa325dec7..8fe20b5c45 100644
--- a/src/encoding/binary/varint.go
+++ b/src/encoding/binary/varint.go
@@ -61,8 +61,13 @@ func Uvarint(buf []byte) (uint64, int) {
 	var x uint64
 	var s uint
 	for i, b := range buf {
+		if i == MaxVarintLen64 {
+			// Catch byte reads past MaxVarintLen64.
+			// See issue https://golang.org/issues/41185
+			return 0, -(i + 1) // overflow
+		}
 		if b < 0x80 {
-			if i >= MaxVarintLen64 || i == MaxVarintLen64-1 && b > 1 {
+			if i == MaxVarintLen64-1 && b > 1 {
 				return 0, -(i + 1) // overflow
 			}
 			return x | uint64(b)<<s, i + 1
diff --git a/src/encoding/binary/varint_test.go b/src/encoding/binary/varint_test.go
index 6ef4c99505..d025a67538 100644
--- a/src/encoding/binary/varint_test.go
+++ b/src/encoding/binary/varint_test.go
@@ -7,6 +7,7 @@ package binary
 import (
 	"bytes"
 	"io"
+	"math"
 	"testing"
 )
 
@@ -121,10 +122,66 @@ func TestBufferTooSmall(t *testing.T) {
 	}
 }
 
+// Ensure that we catch overflows of bytes going past MaxVarintLen64.
+// See issue https://golang.org/issues/41185
+func TestBufferTooBigWithOverflow(t *testing.T) {
+	tests := []struct {
+		in        []byte
+		name      string
+		wantN     int
+		wantValue uint64
+	}{
+		{
+			name: "invalid: 1000 bytes",
+			in: func() []byte {
+				b := make([]byte, 1000)
+				for i := range b {
+					b[i] = 0xff
+				}
+				b[999] = 0
+				return b
+			}(),
+			wantN:     -11,
+			wantValue: 0,
+		},
+		{
+			name:      "valid: math.MaxUint64-40",
+			in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01},
+			wantValue: math.MaxUint64 - 40,
+			wantN:     10,
+		},
+		{
+			name:      "invalid: with more than MaxVarintLen64 bytes",
+			in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x01},
+			wantN:     -11,
+			wantValue: 0,
+		},
+		{
+			name:      "invalid: 10th byte",
+			in:        []byte{0xd7, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
+			wantN:     -10,
+			wantValue: 0,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			value, n := Uvarint(tt.in)
+			if g, w := n, tt.wantN; g != w {
+				t.Errorf("bytes returned=%d, want=%d", g, w)
+			}
+			if g, w := value, tt.wantValue; g != w {
+				t.Errorf("value=%d, want=%d", g, w)
+			}
+		})
+	}
+}
+
 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)
+		t.Errorf("Uvarint(% X): got x = %d, n = %d; want 0, %d", buf, x, n, n0)
 	}
 
 	r := bytes.NewReader(buf)
@@ -140,8 +197,8 @@ func testOverflow(t *testing.T, buf []byte, x0 uint64, n0 int, err0 error) {
 
 func TestOverflow(t *testing.T) {
 	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
+	testOverflow(t, []byte{0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x1, 0, 0}, 0, -11, overflow)
+	testOverflow(t, []byte{0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 1<<64-1, -11, overflow) // 11 bytes, should overflow
 }
 
 func TestNonCanonicalZero(t *testing.T) {