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 <katiehockman@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/216677
Run-TryBot: Katie Hockman <katie@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/cryptobyte/asn1.go b/cryptobyte/asn1.go
index 528b9bf..f930f7e 100644
--- a/cryptobyte/asn1.go
+++ b/cryptobyte/asn1.go
@@ -470,7 +470,8 @@
// 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 @@
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 9f6c952..f6bb96d 100644
--- a/cryptobyte/asn1_test.go
+++ b/cryptobyte/asn1_test.go
@@ -31,6 +31,10 @@
{"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 39bf98a..589d297 100644
--- a/cryptobyte/string.go
+++ b/cryptobyte/string.go
@@ -24,7 +24,7 @@
// 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 @@
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 d57a88c..49d6626 100644
--- a/internal/wycheproof/wycheproof_test.go
+++ b/internal/wycheproof/wycheproof_test.go
@@ -17,7 +17,6 @@
"os"
"os/exec"
"path/filepath"
- "runtime"
"testing"
_ "crypto/sha1"
@@ -34,9 +33,6 @@
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
