crypto/x509: move constraint checking after chain building

The standard approach to constraint checking involves checking the
constraints during chain building. This is typically done as most chain
building algorithms want to find a single chain. We don't do this, and
instead build every valid chain we can find. Because of this, we don't
_need_ to do constraint checking during the chain building stage, and
instead can defer it until we have built all of the potentially valid
chains (we already do this for EKU nesting and policy checking).

This allows us to limit the constraints we check to only chains issued
by trusted roots, which reduces the attack surface for constraint
checking, which is an annoyingly algorithmically complex process (for
now).

To maintain previous behavior, if we see an error during constraint
checking, and we end up with no valid chains, we return the first
constraint checking error, instead of a more verbose error indicating
if there were different problems during filtering. At some point we
probably should come up with a more unified error type for chain
building that can contain information about multiple failure modes.

Change-Id: I5780b3adce8538eb4c3b56ddec52f0723d39009e
Reviewed-on: https://go-review.googlesource.com/c/go/+/713240
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Roland Shoemaker <[email protected]>
Reviewed-by: Daniel McCarney <[email protected]>

Author: rolandshoemaker

src/crypto/x509/verify.go

@@ -636,107 +636,8 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 		}
 	}
 
-	maxConstraintComparisons := opts.MaxConstraintComparisions
-	if maxConstraintComparisons == 0 {
-		maxConstraintComparisons = 250000
-	}
-	comparisonCount := 0
-
-	if certType == intermediateCertificate || certType == rootCertificate {
-		if len(currentChain) == 0 {
-			return errors.New("x509: internal error: empty chain when appending CA cert")
-		}
-	}
-
-	// 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{}
-		for _, c := range currentChain {
-			if c.hasSANExtension() {
-				toCheck = append(toCheck, c)
-			}
-		}
-		for _, sanCert := range toCheck {
-			err := forEachSAN(sanCert.getSANExtension(), func(tag int, data []byte) error {
-				switch tag {
-				case nameTypeEmail:
-					name := string(data)
-					mailbox, ok := parseRFC2821Mailbox(name)
-					if !ok {
-						return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
-					}
-
-					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
-						func(parsedName, constraint any) (bool, error) {
-							return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
-						}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
-						return err
-					}
-
-				case nameTypeDNS:
-					name := string(data)
-					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), reversedDomainsCache, reversedConstraintsCache)
-						}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
-						return err
-					}
-
-				case nameTypeURI:
-					name := string(data)
-					uri, err := url.Parse(name)
-					if err != nil {
-						return fmt.Errorf("x509: internal error: URI SAN %q failed to parse", name)
-					}
-
-					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
-						func(parsedName, constraint any) (bool, error) {
-							return matchURIConstraint(parsedName.(*url.URL), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
-						}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
-						return err
-					}
-
-				case nameTypeIP:
-					ip := net.IP(data)
-					if l := len(ip); l != net.IPv4len && l != net.IPv6len {
-						return fmt.Errorf("x509: internal error: IP SAN %x failed to parse", data)
-					}
-
-					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "IP address", ip.String(), ip,
-						func(parsedName, constraint any) (bool, error) {
-							return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet))
-						}, c.PermittedIPRanges, c.ExcludedIPRanges); err != nil {
-						return err
-					}
-
-				default:
-					// Unknown SAN types are ignored.
-				}
-
-				return nil
-			})
-
-			if err != nil {
-				return err
-			}
-		}
+	if (certType == intermediateCertificate || certType == rootCertificate) && len(currentChain) == 0 {
+		return errors.New("x509: internal error: empty chain when appending CA cert")
 	}
 
 	// KeyUsage status flags are ignored. From Engineering Security, Peter
@@ -881,6 +782,7 @@ func (c *Certificate) Verify(opts VerifyOptions) ([][]*Certificate, error) {
 
 	var invalidPoliciesChains int
 	var incompatibleKeyUsageChains int
+	var constraintsHintErr error
 	candidateChains = slices.DeleteFunc(candidateChains, func(chain []*Certificate) bool {
 		if !policiesValid(chain, opts) {
 			invalidPoliciesChains++
@@ -892,10 +794,19 @@ func (c *Certificate) Verify(opts VerifyOptions) ([][]*Certificate, error) {
 			incompatibleKeyUsageChains++
 			return true
 		}
+		if err := checkChainConstraints(chain, opts); err != nil {
+			if constraintsHintErr == nil {
+				constraintsHintErr = err
+			}
+			return true
+		}
 		return false
 	})
 
 	if len(candidateChains) == 0 {
+		if constraintsHintErr != nil {
+			return nil, constraintsHintErr // Preserve previous constraint behavior
+		}
 		var details []string
 		if incompatibleKeyUsageChains > 0 {
 			if invalidPoliciesChains == 0 {
@@ -1271,6 +1182,106 @@ NextCert:
 	return true
 }
 
+func checkChainConstraints(chain []*Certificate, opts VerifyOptions) error {
+	maxConstraintComparisons := opts.MaxConstraintComparisions
+	if maxConstraintComparisons == 0 {
+		maxConstraintComparisons = 250000
+	}
+	comparisonCount := 0
+
+	// 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{}
+
+	for i, c := range chain {
+		if !c.hasNameConstraints() {
+			continue
+		}
+		for _, sanCert := range chain[:i] {
+			if !sanCert.hasSANExtension() {
+				continue
+			}
+			err := forEachSAN(sanCert.getSANExtension(), func(tag int, data []byte) error {
+				switch tag {
+				case nameTypeEmail:
+					name := string(data)
+					mailbox, ok := parseRFC2821Mailbox(name)
+					if !ok {
+						return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
+					}
+
+					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
+						func(parsedName, constraint any) (bool, error) {
+							return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
+						}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
+						return err
+					}
+
+				case nameTypeDNS:
+					name := string(data)
+					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), reversedDomainsCache, reversedConstraintsCache)
+						}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
+						return err
+					}
+
+				case nameTypeURI:
+					name := string(data)
+					uri, err := url.Parse(name)
+					if err != nil {
+						return fmt.Errorf("x509: internal error: URI SAN %q failed to parse", name)
+					}
+
+					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
+						func(parsedName, constraint any) (bool, error) {
+							return matchURIConstraint(parsedName.(*url.URL), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
+						}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
+						return err
+					}
+
+				case nameTypeIP:
+					ip := net.IP(data)
+					if l := len(ip); l != net.IPv4len && l != net.IPv6len {
+						return fmt.Errorf("x509: internal error: IP SAN %x failed to parse", data)
+					}
+
+					if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "IP address", ip.String(), ip,
+						func(parsedName, constraint any) (bool, error) {
+							return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet))
+						}, c.PermittedIPRanges, c.ExcludedIPRanges); err != nil {
+						return err
+					}
+
+				default:
+					// Unknown SAN types are ignored.
+				}
+
+				return nil
+			})
+
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 func mustNewOIDFromInts(ints []uint64) OID {
 	oid, err := OIDFromInts(ints)
 	if err != nil {