encoding/unicode: correctly handle single-byte UTF-16 inputs (and harden transform.String)

If a single byte is passed to a UTF-16 decoder
with atEOF set, it should not ask for more src
with ErrShortSrc but return an error. Also harden
transform.String not to enter an infinite loop if a
Transformer does return ErrShortSrc with atEOF true.

Fixes #39491
Fixes CVE-2020-14040

Change-Id: If8d2a9bca4eb9b4270c98a4967d356082043e17e
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/768667
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Reviewed-on: https://go-review.googlesource.com/c/text/+/238238
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

encoding/unicode/unicode.go

diff --git a/encoding/unicode/unicode.go b/encoding/unicode/unicode.go
index f2e576d..dd99ad1 100644
--- a/encoding/unicode/unicode.go
+++ b/encoding/unicode/unicode.go
@@ -368,16 +368,13 @@
 }
 
 func (u *utf16Decoder) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) {
+	if len(src) < 2 && atEOF && u.current.bomPolicy&requireBOM != 0 {
+		return 0, 0, ErrMissingBOM
+	}
 	if len(src) == 0 {
-		if atEOF && u.current.bomPolicy&requireBOM != 0 {
-			return 0, 0, ErrMissingBOM
-		}
 		return 0, 0, nil
 	}
-	if u.current.bomPolicy&acceptBOM != 0 {
-		if len(src) < 2 {
-			return 0, 0, transform.ErrShortSrc
-		}
+	if len(src) >= 2 && u.current.bomPolicy&acceptBOM != 0 {
 		switch {
 		case src[0] == 0xfe && src[1] == 0xff:
 			u.current.endianness = BigEndian

encoding/unicode/unicode_test.go

diff --git a/encoding/unicode/unicode_test.go b/encoding/unicode/unicode_test.go
index 02520c9..9611ba5 100644
--- a/encoding/unicode/unicode_test.go
+++ b/encoding/unicode/unicode_test.go
@@ -114,6 +114,19 @@
 		err:     ErrMissingBOM,
 		t:       utf16BEEB.NewDecoder(),
 	}, {
+		desc:    "utf-16 dec: Fail on single byte missing BOM when required",
+		src:     "\x00",
+		sizeDst: 4,
+		t:       utf16BEEB.NewDecoder(),
+		err:     ErrMissingBOM,
+	}, {
+		desc:    "utf-16 dec: Fail on short src missing BOM when required",
+		src:     "\x00",
+		notEOF:  true,
+		sizeDst: 4,
+		t:       utf16BEEB.NewDecoder(),
+		err:     transform.ErrShortSrc,
+	}, {
 		desc:    "utf-16 dec: SHOULD interpret text as big-endian when BOM not present (RFC 2781:4.3)",
 		src:     "\xD8\x08\xDF\x45\x00\x3D\x00\x52\x00\x61",
 		sizeDst: 100,
@@ -121,6 +134,20 @@
 		nSrc:    10,
 		t:       utf16BEUB.NewDecoder(),
 	}, {
+		desc:    "utf-16 dec: incorrect UTF-16: odd bytes",
+		src:     "\x00",
+		sizeDst: 100,
+		want:    "\uFFFD",
+		nSrc:    1,
+		t:       utf16BEUB.NewDecoder(),
+	}, {
+		desc:    "utf-16 dec: Fail on incorrect UTF-16: short source odd bytes",
+		src:     "\x00",
+		notEOF:  true,
+		sizeDst: 100,
+		t:       utf16BEUB.NewDecoder(),
+		err:     transform.ErrShortSrc,
+	}, {
 		// This is an error according to RFC 2781. But errors in RFC 2781 are
 		// open to interpretations, so I guess this is fine.
 		desc:    "utf-16le dec: incorrect BOM is an error (RFC 2781:4.1)",
@@ -273,16 +300,23 @@
 		t:       utf16LEUB.NewDecoder(),
 	}}
 	for i, tc := range testCases {
-		b := make([]byte, tc.sizeDst)
-		nDst, nSrc, err := tc.t.Transform(b, []byte(tc.src), !tc.notEOF)
-		if err != tc.err {
-			t.Errorf("%d:%s: error was %v; want %v", i, tc.desc, err, tc.err)
-		}
-		if got := string(b[:nDst]); got != tc.want {
-			t.Errorf("%d:%s: result was %q: want %q", i, tc.desc, got, tc.want)
-		}
-		if nSrc != tc.nSrc {
-			t.Errorf("%d:%s: nSrc was %d; want %d", i, tc.desc, nSrc, tc.nSrc)
+		for j := 0; j < 2; j++ {
+			b := make([]byte, tc.sizeDst)
+			nDst, nSrc, err := tc.t.Transform(b, []byte(tc.src), !tc.notEOF)
+			if err != tc.err {
+				t.Errorf("%d:%s: error was %v; want %v", i, tc.desc, err, tc.err)
+			}
+			if got := string(b[:nDst]); got != tc.want {
+				t.Errorf("%d:%s: result was %q: want %q", i, tc.desc, got, tc.want)
+			}
+			if nSrc != tc.nSrc {
+				t.Errorf("%d:%s: nSrc was %d; want %d", i, tc.desc, nSrc, tc.nSrc)
+			}
+			// Since Transform is stateful, run failures again
+			// to ensure that the same error occurs a second time.
+			if err == nil {
+				break
+			}
 		}
 	}
 }

transform/transform.go

diff --git a/transform/transform.go b/transform/transform.go
index 520b9ad..48ec64b 100644
--- a/transform/transform.go
+++ b/transform/transform.go
@@ -648,7 +648,8 @@
 	// Transform the remaining input, growing dst and src buffers as necessary.
 	for {
 		n := copy(src, s[pSrc:])
-		nDst, nSrc, err := t.Transform(dst[pDst:], src[:n], pSrc+n == len(s))
+		atEOF := pSrc+n == len(s)
+		nDst, nSrc, err := t.Transform(dst[pDst:], src[:n], atEOF)
 		pDst += nDst
 		pSrc += nSrc
 
@@ -659,6 +660,9 @@
 				dst = grow(dst, pDst)
 			}
 		} else if err == ErrShortSrc {
+			if atEOF {
+				return string(dst[:pDst]), pSrc, err
+			}
 			if nSrc == 0 {
 				src = grow(src, 0)
 			}

transform/transform_test.go

diff --git a/transform/transform_test.go b/transform/transform_test.go
index 771633d..273abfa 100644
--- a/transform/transform_test.go
+++ b/transform/transform_test.go
@@ -1315,3 +1315,26 @@
 	aaa = strings.Repeat("a", 4096)
 	AAA = strings.Repeat("A", 4096)
 )
+
+type badTransformer struct{}
+
+func (bt badTransformer) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) {
+	return 0, 0, ErrShortSrc
+}
+
+func (bt badTransformer) Reset() {}
+
+func TestBadTransformer(t *testing.T) {
+	bt := badTransformer{}
+	if _, _, err := String(bt, "aaa"); err != ErrShortSrc {
+		t.Errorf("String expected ErrShortSrc, got nil")
+	}
+	if _, _, err := Bytes(bt, []byte("aaa")); err != ErrShortSrc {
+		t.Errorf("Bytes expected ErrShortSrc, got nil")
+	}
+	r := NewReader(bytes.NewReader([]byte("aaa")), bt)
+	var bytes []byte
+	if _, err := r.Read(bytes); err != ErrShortSrc {
+		t.Errorf("NewReader Read expected ErrShortSrc, got nil")
+	}
+}