[release-branch.go1.12-security] internal/x/crypto/cryptobyte: import security fix...

    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>
x/crypto/cryptobyte is used in crypto/x509 for parsing certificates.
Malformed certificates might cause a panic during parsing on 32-bit
architectures (like arm and 386).

Change-Id: I3c619af508bacff84023be4d5a7c4992c2f20a56
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/647483
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

diff --git a/src/internal/x/crypto/cryptobyte/asn1.go b/src/internal/x/crypto/cryptobyte/asn1.go
index 2d40680ddd3b7f0e3f0885275fb4afee898dbfab..758ac3a649699a57ebe69c4bbbe278b9eb6770e5 100644 (file)
--- a/src/internal/x/crypto/cryptobyte/asn1.go
+++ b/src/internal/x/crypto/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/src/internal/x/crypto/cryptobyte/asn1_test.go b/src/internal/x/crypto/cryptobyte/asn1_test.go
index ca28e3bcfb2565b3c67461265964fe809e88853e..f90d768edb1bea376698e0cb49ecfe4dff659093 100644 (file)
--- a/src/internal/x/crypto/cryptobyte/asn1_test.go
+++ b/src/internal/x/crypto/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/src/internal/x/crypto/cryptobyte/string.go b/src/internal/x/crypto/cryptobyte/string.go
index bd2ed2e207c6e0c9f898063ba5351aa2ae2c26f4..a3ecf638282fd466e0f514d5626f05dabfc450d6 100644 (file)
--- a/src/internal/x/crypto/cryptobyte/string.go
+++ b/src/internal/x/crypto/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