From 69ecbb4d6d5dab05e49161c6e77ea40a030884e1 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Wed, 22 Jan 2020 16:59:53 -0500 Subject: [PATCH] cryptobyte: fix panic due to malformed ASN.1 inputs on 32-bit archs 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 Reviewed-by: Adam Langley Reviewed-on: https://go-review.googlesource.com/c/crypto/+/216677 Run-TryBot: Katie Hockman Reviewed-by: Dmitri Shuralyov Reviewed-by: Filippo Valsorda TryBot-Result: Gobot Gobot --- cryptobyte/asn1.go | 5 +++-- cryptobyte/asn1_test.go | 4 ++++ cryptobyte/string.go | 7 +------ internal/wycheproof/wycheproof_test.go | 4 ---- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/cryptobyte/asn1.go b/cryptobyte/asn1.go index 528b9bff..f930f7e5 100644 --- 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 index 9f6c952a..f6bb96d5 100644 --- 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 index 39bf98ae..589d297e 100644 --- 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 diff --git a/internal/wycheproof/wycheproof_test.go b/internal/wycheproof/wycheproof_test.go index d57a88c2..49d6626c 100644 --- a/internal/wycheproof/wycheproof_test.go +++ b/internal/wycheproof/wycheproof_test.go @@ -17,7 +17,6 @@ import ( "os" "os/exec" "path/filepath" - "runtime" "testing" _ "crypto/sha1" @@ -34,9 +33,6 @@ func TestMain(m *testing.M) { log.Printf("skipping test because 'go' command is unavailable: %v", err) os.Exit(0) } - if runtime.GOARCH == "386" || runtime.GOARCH == "arm" { - os.Exit(0) // skip tests - } // Download the JSON test files from github.com/google/wycheproof // using `go mod download -json` so the cached source of the testdata