[release-branch.go1.13] cryptobyte: fix panic due to malformed ASN.1 inputs on 32-bit archs

FiloSottile · FiloSottile · commit 8b5121be2f68 · 2020-01-24T22:56:46.000Z
When int is 32 bits wide (on 32-bit architectures like 386 and arm), an overflow could occur, causing a panic, due to malformed ASN.1 being passed to any of the ASN1 methods of String. Tested on linux/386 and darwin/amd64. This fixes CVE-2020-7919 and was found thanks to the Project Wycheproof test vectors. Change-Id: I8c9696a8bfad1b40ec877cd740dba3467d66ab54 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/645211 Reviewed-by: Katie Hockman <katiehockman@google.com> Reviewed-by: Adam Langley <agl@google.com> Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/647740
diff --git a/cryptobyte/asn1.go b/cryptobyte/asn1.go
@@ -470,7 +470,8 @@ func (s *String) ReadASN1GeneralizedTime(out *time.Time) bool {
 // It reports whether the read was successful.
 func (s *String) ReadASN1BitString(out *encoding_asn1.BitString) bool {
 	var bytes String
-	if !s.ReadASN1(&bytes, asn1.BIT_STRING) || len(bytes) == 0 {
+	if !s.ReadASN1(&bytes, asn1.BIT_STRING) || len(bytes) == 0 ||
+		len(bytes)*8/8 != len(bytes) {
 		return false
 	}
 
@@ -740,7 +741,7 @@ func (s *String) readASN1(out *String, outTag *asn1.Tag, skipHeader bool) bool {
 		length = headerLen + len32
 	}
 
-	if uint32(int(length)) != length || !s.ReadBytes((*[]byte)(out), int(length)) {
+	if int(length) < 0 || !s.ReadBytes((*[]byte)(out), int(length)) {
 		return false
 	}
 	if skipHeader && !out.Skip(int(headerLen)) {
diff --git a/cryptobyte/asn1_test.go b/cryptobyte/asn1_test.go
@@ -31,6 +31,10 @@ var readASN1TestData = []readASN1Test{
 	{"non-minimal length", append([]byte{0x30, 0x82, 0, 0x80}, make([]byte, 0x80)...), 0x30, false, nil},
 	{"invalid tag", []byte{0xa1, 3, 0x4, 1, 1}, 31, false, nil},
 	{"high tag", []byte{0x1f, 0x81, 0x80, 0x01, 2, 1, 2}, 0xff /* actually 0x4001, but tag is uint8 */, false, nil},
+	{"2**31 - 1 length", []byte{0x30, 0x84, 0x7f, 0xff, 0xff, 0xff}, 0x30, false, nil},
+	{"2**32 - 1 length", []byte{0x30, 0x84, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil},
+	{"2**63 - 1 length", []byte{0x30, 0x88, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil},
+	{"2**64 - 1 length", []byte{0x30, 0x88, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil},
 }
 
 func TestReadASN1(t *testing.T) {
diff --git a/cryptobyte/string.go b/cryptobyte/string.go
@@ -24,7 +24,7 @@ type String []byte
 // read advances a String by n bytes and returns them. If less than n bytes
 // remain, it returns nil.
 func (s *String) read(n int) []byte {
-	if len(*s) < n {
+	if len(*s) < n || n < 0 {
 		return nil
 	}
 	v := (*s)[:n]
@@ -105,11 +105,6 @@ func (s *String) readLengthPrefixed(lenLen int, outChild *String) bool {
 		length = length << 8
 		length = length | uint32(b)
 	}
-	if int(length) < 0 {
-		// This currently cannot overflow because we read uint24 at most, but check
-		// anyway in case that changes in the future.
-		return false
-	}
 	v := s.read(int(length))
 	if v == nil {
 		return false

Original file line number	Diff line number	Diff line change
`@@ -470,7 +470,8 @@ func (s String) ReadASN1GeneralizedTime(out time.Time) bool {`
`470`	`470`	`// It reports whether the read was successful.`
`471`	`471`	`func (s String) ReadASN1BitString(out encoding_asn1.BitString) bool {`
`472`	`472`	`var bytes String`
`473`		`- if !s.ReadASN1(&bytes, asn1.BIT_STRING) \|\| len(bytes) == 0 {`
	`473`	`+ if !s.ReadASN1(&bytes, asn1.BIT_STRING) \|\| len(bytes) == 0 \|\|`
	`474`	`+ len(bytes)*8/8 != len(bytes) {`
`474`	`475`	`return false`
`475`	`476`	`}`
`476`	`477`
`@@ -740,7 +741,7 @@ func (s String) readASN1(out String, outTag *asn1.Tag, skipHeader bool) bool {`
`740`	`741`	`length = headerLen + len32`
`741`	`742`	`}`
`742`	`743`
`743`		`- if uint32(int(length)) != length \|\| !s.ReadBytes((*[]byte)(out), int(length)) {`
	`744`	`+ if int(length) < 0 \|\| !s.ReadBytes((*[]byte)(out), int(length)) {`
`744`	`745`	`return false`
`745`	`746`	`}`
`746`	`747`	`if skipHeader && !out.Skip(int(headerLen)) {`