[release-branch.go1.24] crypto/x509: rework fix for CVE-2025-58187

In CL 709854 we enabled strict validation for a number of properties of
domain names (and their constraints). This caused significant breakage,
since we didn't previously disallow the creation of certificates which
contained these malformed domains.

Rollback a number of the properties we enforced, making domainNameValid
only enforce the same properties that domainToReverseLabels does. Since
this also undoes some of the DoS protections our initial fix enabled,
this change also adds caching of constraints in isValid (which perhaps
is the fix we should've initially chosen).

Updates #75835
Updates #75828
Fixes #75860

Change-Id: Ie6ca6b4f30e9b8a143692b64757f7bbf4671ed0e
Reviewed-on: https://go-review.googlesource.com/c/go/+/710735
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 1cd71689f2)
Reviewed-on: https://go-review.googlesource.com/c/go/+/710879
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>

src/crypto/x509/name_constraints_test.go

@@ -1456,7 +1456,63 @@ var nameConstraintsTests = []nameConstraintsTest{
 		expectedError: "incompatible key usage",
 	},
 
-	// #77: if several EKUs are requested, satisfying any of them is sufficient.
+	// 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 constraint
+	// checking is not triggered.
+	{
+		roots: make([]constraintsSpec, 1),
+		intermediates: [][]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.
+	{
+		roots: []constraintsSpec{
+			{
+				bad: []string{"uri:"},
+			},
+		},
+		intermediates: [][]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.
+	{
+		roots: []constraintsSpec{
+			{
+				bad: []string{"uri:"},
+			},
+		},
+		intermediates: [][]constraintsSpec{
+			{
+				{},
+			},
+		},
+		leaf: leafSpec{
+			sans: []string{"email:this @ is invalid"},
+		},
+		expectedError: "cannot parse rfc822Name",
+	},
+
+	// #80: if several EKUs are requested, satisfying any of them is sufficient.
 	{
 		roots: make([]constraintsSpec, 1),
 		intermediates: [][]constraintsSpec{
@@ -1471,7 +1527,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		requestedEKUs: []ExtKeyUsage{ExtKeyUsageClientAuth, ExtKeyUsageEmailProtection},
 	},
 
-	// #78: EKUs that are not asserted in VerifyOpts are not required to be
+	// #81: EKUs that are not asserted in VerifyOpts are not required to be
 	// nested.
 	{
 		roots: make([]constraintsSpec, 1),
@@ -1490,7 +1546,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #79: a certificate without SANs and CN is accepted in a constrained chain.
+	// #82: a certificate without SANs and CN is accepted in a constrained chain.
 	{
 		roots: []constraintsSpec{
 			{
@@ -1507,7 +1563,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #80: a certificate without SANs and with a CN that does not parse as a
+	// #83: a certificate without SANs and with a CN that does not parse as a
 	// hostname is accepted in a constrained chain.
 	{
 		roots: []constraintsSpec{
@@ -1526,7 +1582,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #81: a certificate with SANs and CN is accepted in a constrained chain.
+	// #84: a certificate with SANs and CN is accepted in a constrained chain.
 	{
 		roots: []constraintsSpec{
 			{
@@ -1544,7 +1600,14 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #82: URIs with IPv6 addresses with zones and ports are rejected
+	// #85: .example.com is an invalid DNS name, it should not match the
+	// constraint example.com.
+	{
+		roots:         []constraintsSpec{{ok: []string{"dns:example.com"}}},
+		leaf:          leafSpec{sans: []string{"dns:.example.com"}},
+		expectedError: "cannot parse dnsName \".example.com\"",
+	},
+	// #86: URIs with IPv6 addresses with zones and ports are rejected
 	{
 		roots: []constraintsSpec{
 			{

src/crypto/x509/parser.go

@@ -379,14 +379,10 @@ func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string
 			if err := isIA5String(email); err != nil {
 				return errors.New("x509: SAN rfc822Name is malformed")
 			}
-			parsed, ok := parseRFC2821Mailbox(email)
-			if !ok || (ok && !domainNameValid(parsed.domain, false)) {
-				return errors.New("x509: SAN rfc822Name is malformed")
-			}
 			emailAddresses = append(emailAddresses, email)
 		case nameTypeDNS:
 			name := string(data)
-			if err := isIA5String(name); err != nil || (err == nil && !domainNameValid(name, false)) {
+			if err := isIA5String(name); err != nil {
 				return errors.New("x509: SAN dNSName is malformed")
 			}
 			dnsNames = append(dnsNames, string(name))
@@ -396,9 +392,12 @@ func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string
 				return errors.New("x509: SAN uniformResourceIdentifier is malformed")
 			}
 			uri, err := url.Parse(uriStr)
-			if err != nil || (err == nil && uri.Host != "" && !domainNameValid(uri.Host, false)) {
+			if err != nil {
 				return fmt.Errorf("x509: cannot parse URI %q: %s", uriStr, err)
 			}
+			if len(uri.Host) > 0 && !domainNameValid(uri.Host, false) {
+				return fmt.Errorf("x509: cannot parse URI %q: invalid domain", uriStr)
+			}
 			uris = append(uris, uri)
 		case nameTypeIP:
 			switch len(data) {
@@ -1262,36 +1261,58 @@ func ParseRevocationList(der []byte) (*RevocationList, error) {
 	return rl, nil
 }
 
-// domainNameValid does minimal domain name validity checking. In particular it
+// domainNameValid is an alloc-less version of the checks that
-// enforces the following properties:
+// domainToReverseLabels does.
-//   - names cannot have the trailing period
-//   - names can only have a leading period if constraint is true
-//   - names must be <= 253 characters
-//   - names cannot have empty labels
-//   - names cannot labels that are longer than 63 characters
-//
-// Note that this does not enforce the LDH requirements for domain names.
 func domainNameValid(s string, constraint bool) bool {
-	if len(s) == 0 && constraint {
+	// TODO(#75835): This function omits a number of checks which we
+	// really should be doing to enforce that domain names are valid names per
+	// RFC 1034. We previously enabled these checks, but this broke a
+	// significant number of certificates we previously considered valid, and we
+	// happily create via CreateCertificate (et al). We should enable these
+	// checks, but will need to gate them behind a GODEBUG.
+	//
+	// I have left the checks we previously enabled, noted with "TODO(#75835)" so
+	// that we can easily re-enable them once we unbreak everyone.
+
+	// TODO(#75835): this should only be true for constraints.
+	if len(s) == 0 {
 		return true
 	}
-	if len(s) == 0 || (!constraint && s[0] == '.') || s[len(s)-1] == '.' || len(s) > 253 {
+
+	// Do not allow trailing period (FQDN format is not allowed in SANs or
+	// constraints).
+	if s[len(s)-1] == '.' {
 		return false
 	}
+
+	// TODO(#75835): domains must have at least one label, cannot have
+	// a leading empty label, and cannot be longer than 253 characters.
+	// if len(s) == 0 || (!constraint && s[0] == '.') || len(s) > 253 {
+	// 	return false
+	// }
+
 	lastDot := -1
 	if constraint && s[0] == '.' {
 		s = s[1:]
 	}
 
 	for i := 0; i <= len(s); i++ {
+		if i < len(s) && (s[i] < 33 || s[i] > 126) {
+			// Invalid character.
+			return false
+		}
 		if i == len(s) || s[i] == '.' {
 			labelLen := i
 			if lastDot >= 0 {
 				labelLen -= lastDot + 1
 			}
-			if labelLen == 0 || labelLen > 63 {
+			if labelLen == 0 {
 				return false
 			}
+			// TODO(#75835): labels cannot be longer than 63 characters.
+			// if labelLen > 63 {
+			// 	return false
+			// }
 			lastDot = i
 		}
 	}

src/crypto/x509/parser_test.go

@@ -5,6 +5,9 @@
 package x509
 
 import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
 	"encoding/asn1"
 	"encoding/pem"
 	"os"
@@ -193,7 +196,31 @@ func TestDomainNameValid(t *testing.T) {
 		constraint bool
 		valid      bool
 	}{
-		{"empty name, name", "", false, false},
+		// TODO(#75835): these tests are for stricter name validation, which we
+		// had to disable. Once we reenable these strict checks, behind a
+		// GODEBUG, we should add them back in.
+		// {"empty name, name", "", false, false},
+		// {"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
+		// {"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
+		// {"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
+		// {"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
+		// {"64 char single label, name", strings.Repeat("a", 64), false, false},
+		// {"64 char single label, constraint", strings.Repeat("a", 64), true, false},
+		// {"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
+		// {"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
+
+		// TODO(#75835): these are the inverse of the tests above, they should be removed
+		// once the strict checking is enabled.
+		{"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, true},
+		{"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, true},
+		{"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, true},
+		{"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, true},
+		{"64 char single label, name", strings.Repeat("a", 64), false, true},
+		{"64 char single label, constraint", strings.Repeat("a", 64), true, true},
+		{"64 char label, name", "a." + strings.Repeat("a", 64), false, true},
+		{"64 char label, constraint", "a." + strings.Repeat("a", 64), true, true},
+
+		// Check we properly enforce properties of domain names.
 		{"empty name, constraint", "", true, true},
 		{"empty label, name", "a..a", false, false},
 		{"empty label, constraint", "a..a", true, false},
@@ -207,23 +234,60 @@ func TestDomainNameValid(t *testing.T) {
 		{"trailing period, constraint", "a.", true, false},
 		{"bare label, name", "a", false, true},
 		{"bare label, constraint", "a", true, true},
-		{"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
-		{"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
-		{"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
-		{"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
-		{"64 char single label, name", strings.Repeat("a", 64), false, false},
-		{"64 char single label, constraint", strings.Repeat("a", 64), true, false},
 		{"63 char single label, name", strings.Repeat("a", 63), false, true},
 		{"63 char single label, constraint", strings.Repeat("a", 63), true, true},
-		{"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
-		{"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
 		{"63 char label, name", "a." + strings.Repeat("a", 63), false, true},
 		{"63 char label, constraint", "a." + strings.Repeat("a", 63), true, true},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			if tc.valid != domainNameValid(tc.dnsName, tc.constraint) {
+			valid := domainNameValid(tc.dnsName, tc.constraint)
+			if tc.valid != valid {
 				t.Errorf("domainNameValid(%q, %t) = %v; want %v", tc.dnsName, tc.constraint, !tc.valid, tc.valid)
 			}
+			// Also check that we enforce the same properties as domainToReverseLabels
+			trimmedName := tc.dnsName
+			if tc.constraint && len(trimmedName) > 1 && trimmedName[0] == '.' {
+				trimmedName = trimmedName[1:]
+			}
+			_, revValid := domainToReverseLabels(trimmedName)
+			if valid != revValid {
+				t.Errorf("domainNameValid(%q, %t) = %t != domainToReverseLabels(%q) = %t", tc.dnsName, tc.constraint, valid, trimmedName, revValid)
+			}
 		})
 	}
 }
+
+func TestRoundtripWeirdSANs(t *testing.T) {
+	// TODO(#75835): check that certificates we create with CreateCertificate that have malformed SAN values
+	// can be parsed by ParseCertificate. We should eventually restrict this, but for now we have to maintain
+	// this property as people have been relying on it.
+	k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	badNames := []string{
+		"baredomain",
+		"baredomain.",
+		strings.Repeat("a", 255),
+		strings.Repeat("a", 65) + ".com",
+	}
+	tmpl := &Certificate{
+		EmailAddresses: badNames,
+		DNSNames:       badNames,
+	}
+	b, err := CreateCertificate(rand.Reader, tmpl, tmpl, &k.PublicKey, k)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = ParseCertificate(b)
+	if err != nil {
+		t.Fatalf("Couldn't roundtrip certificate: %v", err)
+	}
+}
+
+func FuzzDomainNameValid(f *testing.F) {
+	f.Fuzz(func(t *testing.T, data string) {
+		domainNameValid(data, false)
+		domainNameValid(data, true)
+	})
+}

src/crypto/x509/verify.go

@@ -429,7 +429,7 @@ func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) {
 	return reverseLabels, true
 }
 
-func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) {
+func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
 	// If the constraint contains an @, then it specifies an exact mailbox
 	// name.
 	if strings.Contains(constraint, "@") {
@@ -442,10 +442,10 @@ func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, erro
 
 	// Otherwise the constraint is like a DNS constraint of the domain part
 	// of the mailbox.
-	return matchDomainConstraint(mailbox.domain, constraint)
+	return matchDomainConstraint(mailbox.domain, constraint, reversedDomainsCache, reversedConstraintsCache)
 }
 
-func matchURIConstraint(uri *url.URL, constraint string) (bool, error) {
+func matchURIConstraint(uri *url.URL, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
 	// From RFC 5280, Section 4.2.1.10:
 	// “a uniformResourceIdentifier that does not include an authority
 	// component with a host name specified as a fully qualified domain
@@ -474,7 +474,7 @@ func matchURIConstraint(uri *url.URL, constraint string) (bool, error) {
 		return false, fmt.Errorf("URI with IP (%q) cannot be matched against constraints", uri.String())
 	}
 
-	return matchDomainConstraint(host, constraint)
+	return matchDomainConstraint(host, constraint, reversedDomainsCache, reversedConstraintsCache)
 }
 
 func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) {
@@ -491,16 +491,21 @@ func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) {
 	return true, nil
 }
 
-func matchDomainConstraint(domain, constraint string) (bool, error) {
+func matchDomainConstraint(domain, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
 	// The meaning of zero length constraints is not specified, but this
 	// code follows NSS and accepts them as matching everything.
 	if len(constraint) == 0 {
 		return true, nil
 	}
 
-	domainLabels, ok := domainToReverseLabels(domain)
+	domainLabels, found := reversedDomainsCache[domain]
-	if !ok {
+	if !found {
-		return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain)
+		var ok bool
+		domainLabels, ok = domainToReverseLabels(domain)
+		if !ok {
+			return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain)
+		}
+		reversedDomainsCache[domain] = domainLabels
 	}
 
 	// RFC 5280 says that a leading period in a domain name means that at
@@ -514,9 +519,14 @@ func matchDomainConstraint(domain, constraint string) (bool, error) {
 		constraint = constraint[1:]
 	}
 
-	constraintLabels, ok := domainToReverseLabels(constraint)
+	constraintLabels, found := reversedConstraintsCache[constraint]
-	if !ok {
+	if !found {
-		return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint)
+		var ok bool
+		constraintLabels, ok = domainToReverseLabels(constraint)
+		if !ok {
+			return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint)
+		}
+		reversedConstraintsCache[constraint] = constraintLabels
 	}
 
 	if len(domainLabels) < len(constraintLabels) ||
@@ -637,6 +647,19 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 		}
 	}
 
+	// Each time we do constraint checking, we need to check the constraints in
+	// the current certificate against all of the names that preceded it. We
+	// reverse these names using domainToReverseLabels, which is a relatively
+	// expensive operation. Since we check each name against each constraint,
+	// this requires us to do N*C calls to domainToReverseLabels (where N is the
+	// total number of names that preceed the certificate, and C is the total
+	// number of constraints in the certificate). By caching the results of
+	// calling domainToReverseLabels, we can reduce that to N+C calls at the
+	// cost of keeping all of the parsed names and constraints in memory until
+	// we return from isValid.
+	reversedDomainsCache := map[string][]string{}
+	reversedConstraintsCache := map[string][]string{}
+
 	if (certType == intermediateCertificate || certType == rootCertificate) &&
 		c.hasNameConstraints() {
 		toCheck := []*Certificate{}
@@ -657,20 +680,20 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 
 					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
 						func(parsedName, constraint any) (bool, error) {
-							return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string))
+							return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
 						}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
 						return err
 					}
 
 				case nameTypeDNS:
 					name := string(data)
-					if _, ok := domainToReverseLabels(name); !ok {
+					if !domainNameValid(name, false) {
 						return fmt.Errorf("x509: cannot parse dnsName %q", name)
 					}
 
 					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name,
 						func(parsedName, constraint any) (bool, error) {
-							return matchDomainConstraint(parsedName.(string), constraint.(string))
+							return matchDomainConstraint(parsedName.(string), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
 						}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
 						return err
 					}
@@ -684,7 +707,7 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 
 					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
 						func(parsedName, constraint any) (bool, error) {
-							return matchURIConstraint(parsedName.(*url.URL), constraint.(string))
+							return matchURIConstraint(parsedName.(*url.URL), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
 						}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
 						return err
 					}

src/crypto/x509/verify_test.go

@@ -1352,7 +1352,7 @@ var nameConstraintTests = []struct {
 
 func TestNameConstraints(t *testing.T) {
 	for i, test := range nameConstraintTests {
-		result, err := matchDomainConstraint(test.domain, test.constraint)
+		result, err := matchDomainConstraint(test.domain, test.constraint, map[string][]string{}, map[string][]string{})
 
 		if err != nil && !test.expectError {
 			t.Errorf("unexpected error for test #%d: domain=%s, constraint=%s, err=%s", i, test.domain, test.constraint, err)