[release-branch.go1.10] crypto/x509: parse invalid DNS names and email addresses.

Go 1.10 requires that SANs in certificates are valid. However, a
non-trivial number of (generally non-WebPKI) certificates have invalid
strings in dnsName fields and some have even put those dnsName SANs in
CA certificates.

This change defers validity checking until name constraints are checked.

Fixes #23995, #23711.

Change-Id: I2e0ebb0898c047874a3547226b71e3029333b7f1
Reviewed-on: https://go-review.googlesource.com/96378
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/102783
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

diff --git a/src/crypto/x509/name_constraints_test.go b/src/crypto/x509/name_constraints_test.go
index 14741592030e8d8a7102aa03a06825fd517927eb..40caf035529c9e83e8594f0e385caaba513a0fe7 100644 (file)
--- a/src/crypto/x509/name_constraints_test.go
+++ b/src/crypto/x509/name_constraints_test.go
@@ -11,6 +11,7 @@ import (
        "crypto/rand"
        "crypto/x509/pkix"
        "encoding/asn1"
+       "encoding/hex"
        "encoding/pem"
        "fmt"
        "io/ioutil"
@@ -1482,6 +1483,64 @@ var nameConstraintsTests = []nameConstraintsTest{
                },
                requestedEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
        },
+
+       // An invalid DNS SAN should be detected only at validation time so
+       // that we can process CA certificates in the wild that have invalid SANs.
+       // See https://github.com/golang/go/issues/23995
+
+       // #77: an invalid DNS or mail SAN will not be detected if name constaint
+       // checking is not triggered.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{},
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"dns:this is invalid", "email:this @ is invalid"},
+               },
+       },
+
+       // #78: an invalid DNS SAN will be detected if any name constraint checking
+       // is triggered.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               bad: []string{"uri:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"dns:this is invalid"},
+               },
+               expectedError: "cannot parse dnsName",
+       },
+
+       // #79: an invalid email SAN will be detected if any name constraint
+       // checking is triggered.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               bad: []string{"uri:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"email:this @ is invalid"},
+               },
+               expectedError: "cannot parse rfc822Name",
+       },
 }
 
 func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.PrivateKey, parent *Certificate, parentKey *ecdsa.PrivateKey) (*Certificate, error) {
@@ -1550,6 +1609,13 @@ func makeConstraintsLeafCert(leaf leafSpec, key *ecdsa.PrivateKey, parent *Certi
                        }
                        template.IPAddresses = append(template.IPAddresses, ip)
 
+               case strings.HasPrefix(name, "invalidip:"):
+                       ipBytes, err := hex.DecodeString(name[10:])
+                       if err != nil {
+                               return nil, fmt.Errorf("cannot parse invalid IP: %s", err)
+                       }
+                       template.IPAddresses = append(template.IPAddresses, net.IP(ipBytes))
+
                case strings.HasPrefix(name, "email:"):
                        template.EmailAddresses = append(template.EmailAddresses, name[6:])
 
@@ -2011,12 +2077,13 @@ func TestBadNamesInConstraints(t *testing.T) {
 }
 
 func TestBadNamesInSANs(t *testing.T) {
-       // Bad names in SANs should not parse.
+       // Bad names in URI and IP SANs should not parse. Bad DNS and email SANs
+       // will parse and are tested in name constraint tests at the top of this
+       // file.
        badNames := []string{
-               "dns:foo.com.",
-               "email:abc@foo.com.",
-               "email:foo.com.",
                "uri:https://example.com./dsf",
+               "invalidip:0102",
+               "invalidip:0102030405",
        }
 
        priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)

diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index cd6156c69fbaaf565803c55ae4b8b9b75141ba59..074f56678cf88281ff834a64f29ce9d60c9792cf 100644 (file)
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -640,8 +640,7 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
                                name := string(data)
                                mailbox, ok := parseRFC2821Mailbox(name)
                                if !ok {
-                                       // This certificate should not have parsed.
-                                       return errors.New("x509: internal error: rfc822Name SAN failed to parse")
+                                       return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
                                }
 
                                if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
@@ -653,6 +652,10 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 
                        case nameTypeDNS:
                                name := string(data)
+                               if _, ok := domainToReverseLabels(name); !ok {
+                                       return fmt.Errorf("x509: cannot parse dnsName %q", name)
+                               }
+
                                if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name,
                                        func(parsedName, constraint interface{}) (bool, error) {
                                                return matchDomainConstraint(parsedName.(string), constraint.(string))

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 86d9e82aca45332b4744261de68a56650cee7a69..8c50a0d47416e5f3ae12b3bd78d177df0151afb7 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -706,7 +706,9 @@ type Certificate struct {
        OCSPServer            []string
        IssuingCertificateURL []string
 
-       // Subject Alternate Name values
+       // Subject Alternate Name values. (Note that these values may not be valid
+       // if invalid values were contained within a parsed certificate. For
+       // example, an element of DNSNames may not be a valid DNS domain name.)
        DNSNames       []string
        EmailAddresses []string
        IPAddresses    []net.IP
@@ -1126,17 +1128,9 @@ func parseSANExtension(value []byte) (dnsNames, emailAddresses []string, ipAddre
        err = forEachSAN(value, func(tag int, data []byte) error {
                switch tag {
                case nameTypeEmail:
-                       mailbox := string(data)
-                       if _, ok := parseRFC2821Mailbox(mailbox); !ok {
-                               return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
-                       }
-                       emailAddresses = append(emailAddresses, mailbox)
+                       emailAddresses = append(emailAddresses, string(data))
                case nameTypeDNS:
-                       domain := string(data)
-                       if _, ok := domainToReverseLabels(domain); !ok {
-                               return fmt.Errorf("x509: cannot parse dnsName %q", string(data))
-                       }
-                       dnsNames = append(dnsNames, domain)
+                       dnsNames = append(dnsNames, string(data))
                case nameTypeURI:
                        uri, err := url.Parse(string(data))
                        if err != nil {
@@ -1153,7 +1147,7 @@ func parseSANExtension(value []byte) (dnsNames, emailAddresses []string, ipAddre
                        case net.IPv4len, net.IPv6len:
                                ipAddresses = append(ipAddresses, data)
                        default:
-                               return errors.New("x509: certificate contained IP address of length " + strconv.Itoa(len(data)))
+                               return errors.New("x509: cannot parse IP address of length " + strconv.Itoa(len(data)))
                        }
                }