crypto/x509: ignore Common Name when it does not parse as a hostname

The Common Name is used as a hostname when there are no Subject
Alternative Names, but it is not restricted by name constraints. To
protect against a name constraints bypass, we used to require SANs for
constrained chains. See the NameConstraintsWithoutSANs error.

This change ignores the CN when it does not look like a hostname, so we
can avoid returning NameConstraintsWithoutSANs.

This makes it possible to validate certificates with non-hostname CN
against chains that use name constraints to disallow all names, like the
Estonian IDs.

Updates #24151

Change-Id: I798d797990720a01ad9b5a13336756cc472ebf44
Reviewed-on: https://go-review.googlesource.com/123355Reviewed-by: Adam Langley <agl@golang.org>

src/crypto/x509/name_constraints_test.go

@@ -57,6 +57,7 @@ type constraintsSpec struct {
 type leafSpec struct {
 	sans []string
 	ekus []string
+	cn   string
 }
 
 var nameConstraintsTests = []nameConstraintsTest{
@@ -633,7 +634,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #30: without SANs, a certificate is rejected in a constrained chain.
+	// #30: without SANs, a certificate with a CN is rejected in a constrained chain.
 	nameConstraintsTest{
 		roots: []constraintsSpec{
 			constraintsSpec{
@@ -647,9 +648,9 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 		leaf: leafSpec{
 			sans: []string{},
+			cn:   "foo.com",
 		},
 		expectedError: "leaf doesn't have a SAN extension",
-		noOpenSSL:     true, // OpenSSL doesn't require SANs in this case.
 	},
 
 	// #31: IPv6 addresses work in constraints: roots can permit them as
@@ -1580,6 +1581,60 @@ var nameConstraintsTests = []nameConstraintsTest{
 			ekus: []string{"email", "serverAuth"},
 		},
 	},
+
+	// #82: a certificate without SANs and CN is accepted in a constrained chain.
+	nameConstraintsTest{
+		roots: []constraintsSpec{
+			constraintsSpec{
+				ok: []string{"dns:foo.com", "dns:.foo.com"},
+			},
+		},
+		intermediates: [][]constraintsSpec{
+			[]constraintsSpec{
+				constraintsSpec{},
+			},
+		},
+		leaf: leafSpec{
+			sans: []string{},
+		},
+	},
+
+	// #83: a certificate without SANs and with a CN that does not parse as a
+	// hostname is accepted in a constrained chain.
+	nameConstraintsTest{
+		roots: []constraintsSpec{
+			constraintsSpec{
+				ok: []string{"dns:foo.com", "dns:.foo.com"},
+			},
+		},
+		intermediates: [][]constraintsSpec{
+			[]constraintsSpec{
+				constraintsSpec{},
+			},
+		},
+		leaf: leafSpec{
+			sans: []string{},
+			cn:   "foo,bar",
+		},
+	},
+
+	// #84: a certificate with SANs and CN is accepted in a constrained chain.
+	nameConstraintsTest{
+		roots: []constraintsSpec{
+			constraintsSpec{
+				ok: []string{"dns:foo.com", "dns:.foo.com"},
+			},
+		},
+		intermediates: [][]constraintsSpec{
+			[]constraintsSpec{
+				constraintsSpec{},
+			},
+		},
+		leaf: leafSpec{
+			sans: []string{"dns:foo.com"},
+			cn:   "foo.bar",
+		},
+	},
 }
 
 func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.PrivateKey, parent *Certificate, parentKey *ecdsa.PrivateKey) (*Certificate, error) {
@@ -1625,9 +1680,8 @@ func makeConstraintsLeafCert(leaf leafSpec, key *ecdsa.PrivateKey, parent *Certi
 	template := &Certificate{
 		SerialNumber: new(big.Int).SetBytes(serialBytes[:]),
 		Subject: pkix.Name{
-			// Don't set a CommonName because OpenSSL (at least) will try to
-			// match it against name constraints.
 			OrganizationalUnit: []string{"Leaf"},
+			CommonName:         leaf.cn,
 		},
 		NotBefore:             time.Unix(1000, 0),
 		NotAfter:              time.Unix(2000, 0),
@@ -1899,7 +1953,9 @@ func TestConstraintCases(t *testing.T) {
 			t.Fatalf("#%d: cannot create leaf: %s", i, err)
 		}
 
-		if !test.noOpenSSL && testNameConstraintsAgainstOpenSSL {
+		// Skip tests with CommonName set because OpenSSL will try to match it
+		// against name constraints, while we ignore it when it's not hostname-looking.
+		if !test.noOpenSSL && testNameConstraintsAgainstOpenSSL && test.leaf.cn == "" {
 			output, err := testChainAgainstOpenSSL(leafCert, intermediatePool, rootPool)
 			if err == nil && len(test.expectedError) > 0 {
 				t.Errorf("#%d: unexpectedly succeeded against OpenSSL", i)
@@ -1912,7 +1968,7 @@ func TestConstraintCases(t *testing.T) {
 				if _, ok := err.(*exec.ExitError); !ok {
 					t.Errorf("#%d: OpenSSL failed to run: %s", i, err)
 				} else if len(test.expectedError) == 0 {
-					t.Errorf("#%d: OpenSSL unexpectedly failed: %q", i, output)
+					t.Errorf("#%d: OpenSSL unexpectedly failed: %v", i, output)
 					if debugOpenSSLFailure {
 						return
 					}
@@ -1949,7 +2005,7 @@ func TestConstraintCases(t *testing.T) {
 			certAsPEM := func(cert *Certificate) string {
 				var buf bytes.Buffer
 				pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
-				return string(buf.Bytes())
+				return buf.String()
 			}
 			t.Errorf("#%d: root:\n%s", i, certAsPEM(rootPool.certs[0]))
 			t.Errorf("#%d: leaf:\n%s", i, certAsPEM(leafCert))
@@ -2012,7 +2068,7 @@ func testChainAgainstOpenSSL(leaf *Certificate, intermediates, roots *CertPool)
 	cmd.Stderr = &output
 
 	err := cmd.Run()
-	return string(output.Bytes()), err
+	return output.String(), err
 }
 
 var rfc2821Tests = []struct {

src/crypto/x509/verify.go

@@ -6,14 +6,12 @@ package x509
 
 import (
 	"bytes"
-	"encoding/asn1"
 	"errors"
 	"fmt"
 	"net"
 	"net/url"
 	"reflect"
 	"runtime"
-	"strconv"
 	"strings"
 	"time"
 	"unicode/utf8"
@@ -43,7 +41,8 @@ const (
 	NameMismatch
 	// NameConstraintsWithoutSANs results when a leaf certificate doesn't
 	// contain a Subject Alternative Name extension, but a CA certificate
-	// contains name constraints.
+	// contains name constraints, and the Common Name can be interpreted as
+	// a hostname.
 	NameConstraintsWithoutSANs
 	// UnconstrainedName results when a CA certificate contains permitted
 	// name constraints, but leaf certificate contains a name of an
@@ -102,6 +101,12 @@ type HostnameError struct {
 func (h HostnameError) Error() string {
 	c := h.Certificate
 
+	if !c.hasSANExtension() && !validHostname(c.Subject.CommonName) &&
+		matchHostnames(toLowerCaseASCII(c.Subject.CommonName), toLowerCaseASCII(h.Host)) {
+		// This would have validated, if it weren't for the validHostname check on Common Name.
+		return "x509: Common Name is not a valid hostname: " + c.Subject.CommonName
+	}
+
 	var valid string
 	if ip := net.ParseIP(h.Host); ip != nil {
 		// Trying to validate an IP
@@ -115,10 +120,10 @@ func (h HostnameError) Error() string {
 			valid += san.String()
 		}
 	} else {
-		if c.hasSANExtension() {
-			valid = strings.Join(c.DNSNames, ", ")
-		} else {
+		if c.commonNameAsHostname() {
 			valid = c.Subject.CommonName
+		} else {
+			valid = strings.Join(c.DNSNames, ", ")
 		}
 	}
 
@@ -583,17 +588,16 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 		leaf = currentChain[0]
 	}
 
-	if (certType == intermediateCertificate || certType == rootCertificate) && c.hasNameConstraints() {
-		sanExtension, ok := leaf.getSANExtension()
-		if !ok {
-			// This is the deprecated, legacy case of depending on
-			// the CN as a hostname. Chains modern enough to be
-			// using name constraints should not be depending on
-			// CNs.
-			return CertificateInvalidError{c, NameConstraintsWithoutSANs, ""}
-		}
-
-		err := forEachSAN(sanExtension, func(tag int, data []byte) error {
+	checkNameConstraints := (certType == intermediateCertificate || certType == rootCertificate) && c.hasNameConstraints()
+	if checkNameConstraints && leaf.commonNameAsHostname() {
+		// This is the deprecated, legacy case of depending on the commonName as
+		// a hostname. We don't enforce name constraints against the CN, but
+		// VerifyHostname will look for hostnames in there if there are no SANs.
+		// In order to ensure VerifyHostname will not accept an unchecked name,
+		// return an error here.
+		return CertificateInvalidError{c, NameConstraintsWithoutSANs, ""}
+	} else if checkNameConstraints && leaf.hasSANExtension() {
+		err := forEachSAN(leaf.getSANExtension(), func(tag int, data []byte) error {
 			switch tag {
 			case nameTypeEmail:
 				name := string(data)
@@ -692,18 +696,6 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 	return nil
 }
 
-// formatOID formats an ASN.1 OBJECT IDENTIFER in the common, dotted style.
-func formatOID(oid asn1.ObjectIdentifier) string {
-	ret := ""
-	for i, v := range oid {
-		if i > 0 {
-			ret += "."
-		}
-		ret += strconv.Itoa(v)
-	}
-	return ret
-}
-
 // Verify attempts to verify c by building one or more chains from c to a
 // certificate in opts.Roots, using certificates in opts.Intermediates if
 // needed. If successful, it returns one or more chains where the first
@@ -860,6 +852,64 @@ nextIntermediate:
 	return
 }
 
+// validHostname returns whether host is a valid hostname that can be matched or
+// matched against according to RFC 6125 2.2, with some leniency to accomodate
+// legacy values.
+func validHostname(host string) bool {
+	host = strings.TrimSuffix(host, ".")
+
+	if len(host) == 0 {
+		return false
+	}
+
+	for i, part := range strings.Split(host, ".") {
+		if part == "" {
+			// Empty label.
+			return false
+		}
+		if i == 0 && part == "*" {
+			// Only allow full left-most wildcards, as those are the only ones
+			// we match, and matching literal '*' characters is probably never
+			// the expected behavior.
+			continue
+		}
+		for j, c := range part {
+			if 'a' <= c && c <= 'z' {
+				continue
+			}
+			if '0' <= c && c <= '9' {
+				continue
+			}
+			if 'A' <= c && c <= 'Z' {
+				continue
+			}
+			if c == '-' && j != 0 {
+				continue
+			}
+			if c == '_' {
+				// _ is not a valid character in hostnames, but it's commonly
+				// found in deployments outside the WebPKI.
+				continue
+			}
+			return false
+		}
+	}
+
+	return true
+}
+
+// commonNameAsHostname reports whether the Common Name field should be
+// considered the hostname that the certificate is valid for. This is a legacy
+// behavior, disabled if the Subject Alt Name extension is present.
+//
+// It applies the strict validHostname check to the Common Name field, so that
+// certificates without SANs can still be validated against CAs with name
+// constraints if there is no risk the CN would be matched as a hostname.
+// See NameConstraintsWithoutSANs and issue 24151.
+func (c *Certificate) commonNameAsHostname() bool {
+	return !c.hasSANExtension() && validHostname(c.Subject.CommonName)
+}
+
 func matchHostnames(pattern, host string) bool {
 	host = strings.TrimSuffix(host, ".")
 	pattern = strings.TrimSuffix(pattern, ".")
@@ -940,15 +990,16 @@ func (c *Certificate) VerifyHostname(h string) error {
 
 	lowered := toLowerCaseASCII(h)
 
-	if c.hasSANExtension() {
+	if c.commonNameAsHostname() {
+		if matchHostnames(toLowerCaseASCII(c.Subject.CommonName), lowered) {
+			return nil
+		}
+	} else {
 		for _, match := range c.DNSNames {
 			if matchHostnames(toLowerCaseASCII(match), lowered) {
 				return nil
 			}
 		}
-		// If Subject Alt Name is given, we ignore the common name.
-	} else if matchHostnames(toLowerCaseASCII(c.Subject.CommonName), lowered) {
-		return nil
 	}
 
 	return HostnameError{c, h}

src/crypto/x509/x509.go

@@ -843,23 +843,16 @@ func (c *Certificate) CheckSignature(algo SignatureAlgorithm, signed, signature
 }
 
 func (c *Certificate) hasNameConstraints() bool {
-	for _, e := range c.Extensions {
-		if len(e.Id) == 4 && e.Id[0] == 2 && e.Id[1] == 5 && e.Id[2] == 29 && e.Id[3] == 30 {
-			return true
-		}
-	}
-
-	return false
+	return oidInExtensions(oidExtensionNameConstraints, c.Extensions)
 }
 
-func (c *Certificate) getSANExtension() ([]byte, bool) {
+func (c *Certificate) getSANExtension() []byte {
 	for _, e := range c.Extensions {
-		if len(e.Id) == 4 && e.Id[0] == 2 && e.Id[1] == 5 && e.Id[2] == 29 && e.Id[3] == 17 {
-			return e.Value, true
+		if e.Id.Equal(oidExtensionSubjectAltName) {
+			return e.Value
 		}
 	}
-
-	return nil, false
+	return nil
 }
 
 func signaturePublicKeyAlgoMismatchError(expectedPubKeyAlgo PublicKeyAlgorithm, pubKey interface{}) error {