]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/x509: move constraint checking after chain building
authorRoland Shoemaker <roland@golang.org>
Mon, 20 Oct 2025 18:04:38 +0000 (11:04 -0700)
committerGopher Robot <gobot@golang.org>
Fri, 24 Oct 2025 16:13:02 +0000 (09:13 -0700)
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
src/crypto/x509/verify.go

index 7c9aa8268ea8d34f0365ef025b5c225c1d1021a2..12e59335b2d88f8a9c036b284c3766eeef8dbdc2 100644 (file)
@@ -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 {