From e6cff690517e66f610bf8787a442a47d92fc14c7 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 20 Oct 2025 11:04:38 -0700 Subject: [PATCH] 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 Reviewed-by: Dmitri Shuralyov Auto-Submit: Roland Shoemaker Reviewed-by: Daniel McCarney --- src/crypto/x509/verify.go | 213 ++++++++++++++++++++------------------ 1 file changed, 112 insertions(+), 101 deletions(-) diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index 7c9aa8268e..12e59335b2 100644 --- a/src/crypto/x509/verify.go +++ b/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 { -- 2.52.0