Skip to content

Commit 65153e4

Browse files
rolandshoemakergopherbot
rolandshoemaker
authored and
gopherbot
committed
crypto/x509: rework path building
This change does four things: * removes the chain cache * during path building, equality is determined by checking if the subjects and public keys match, rather than checking if the entire certificates are equal * enforces EKU suitability during path building * enforces name constraints on intermediates and roots which have SANs during path building The chain cache is removed as it was causing duplicate chains to be returned, in some cases shadowing better, shorter chains if a longer chain was found first. Checking equality using the subjects and public keys, rather than the entire certificates, allows the path builder to ignore chains which contain cross-signature loops. EKU checking is done during path building, as the previous behavior of only checking EKUs once the path had been built caused the path builder to incorrectly ignore valid paths when it encountered a path which would later be ruled invalid because of unacceptable EKU usage. Name constraints are applied uniformly across all certificates, not just leaves, in order to be more consistent. Fixes #48869 Fixes #45856 Change-Id: I4ca1cd43510d061e148f953d6c1ed935100fdb10 Reviewed-on: https://go-review.googlesource.com/c/go/+/389555 Reviewed-by: Damien Neil <dneil@google.com> Trust: Cherry Mui <cherryyz@google.com> Trust: Roland Shoemaker <roland@golang.org> Run-TryBot: Roland Shoemaker <roland@golang.org> Auto-Submit: Roland Shoemaker <roland@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
1 parent 1a955bc commit 65153e4

File tree

2 files changed

+556
-101
lines changed

2 files changed

+556
-101
lines changed

‎src/crypto/x509/verify.go

+107-101
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package x509
66

77
import (
88
"bytes"
9+
"crypto"
910
"errors"
1011
"fmt"
1112
"net"
@@ -597,73 +598,101 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
597598
leaf = currentChain[0]
598599
}
599600

600-
if (certType == intermediateCertificate || certType == rootCertificate) &&
601-
c.hasNameConstraints() && leaf.hasSANExtension() {
602-
err := forEachSAN(leaf.getSANExtension(), func(tag int, data []byte) error {
603-
switch tag {
604-
case nameTypeEmail:
605-
name := string(data)
606-
mailbox, ok := parseRFC2821Mailbox(name)
607-
if !ok {
608-
return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
609-
}
610-
611-
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
612-
func(parsedName, constraint any) (bool, error) {
613-
return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string))
614-
}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
615-
return err
616-
}
617-
618-
case nameTypeDNS:
619-
name := string(data)
620-
if _, ok := domainToReverseLabels(name); !ok {
621-
return fmt.Errorf("x509: cannot parse dnsName %q", name)
622-
}
623-
624-
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name,
625-
func(parsedName, constraint any) (bool, error) {
626-
return matchDomainConstraint(parsedName.(string), constraint.(string))
627-
}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
628-
return err
629-
}
630-
631-
case nameTypeURI:
632-
name := string(data)
633-
uri, err := url.Parse(name)
634-
if err != nil {
635-
return fmt.Errorf("x509: internal error: URI SAN %q failed to parse", name)
636-
}
637-
638-
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
639-
func(parsedName, constraint any) (bool, error) {
640-
return matchURIConstraint(parsedName.(*url.URL), constraint.(string))
641-
}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
642-
return err
601+
if (len(c.ExtKeyUsage) > 0 || len(c.UnknownExtKeyUsage) > 0) && len(opts.KeyUsages) > 0 {
602+
acceptableUsage := false
603+
um := make(map[ExtKeyUsage]bool, len(opts.KeyUsages))
604+
for _, u := range opts.KeyUsages {
605+
um[u] = true
606+
}
607+
if !um[ExtKeyUsageAny] {
608+
for _, u := range c.ExtKeyUsage {
609+
if u == ExtKeyUsageAny || um[u] {
610+
acceptableUsage = true
611+
break
643612
}
613+
}
614+
if !acceptableUsage {
615+
return CertificateInvalidError{c, IncompatibleUsage, ""}
616+
}
617+
}
618+
}
644619

645-
case nameTypeIP:
646-
ip := net.IP(data)
647-
if l := len(ip); l != net.IPv4len && l != net.IPv6len {
648-
return fmt.Errorf("x509: internal error: IP SAN %x failed to parse", data)
620+
if (certType == intermediateCertificate || certType == rootCertificate) &&
621+
c.hasNameConstraints() {
622+
toCheck := []*Certificate{}
623+
if leaf.hasSANExtension() {
624+
toCheck = append(toCheck, leaf)
625+
}
626+
if c.hasSANExtension() {
627+
toCheck = append(toCheck, c)
628+
}
629+
for _, sanCert := range toCheck {
630+
err := forEachSAN(sanCert.getSANExtension(), func(tag int, data []byte) error {
631+
switch tag {
632+
case nameTypeEmail:
633+
name := string(data)
634+
mailbox, ok := parseRFC2821Mailbox(name)
635+
if !ok {
636+
return fmt.Errorf("x509: cannot parse rfc822Name %q", mailbox)
637+
}
638+
639+
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
640+
func(parsedName, constraint any) (bool, error) {
641+
return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string))
642+
}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
643+
return err
644+
}
645+
646+
case nameTypeDNS:
647+
name := string(data)
648+
if _, ok := domainToReverseLabels(name); !ok {
649+
return fmt.Errorf("x509: cannot parse dnsName %q", name)
650+
}
651+
652+
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name,
653+
func(parsedName, constraint any) (bool, error) {
654+
return matchDomainConstraint(parsedName.(string), constraint.(string))
655+
}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
656+
return err
657+
}
658+
659+
case nameTypeURI:
660+
name := string(data)
661+
uri, err := url.Parse(name)
662+
if err != nil {
663+
return fmt.Errorf("x509: internal error: URI SAN %q failed to parse", name)
664+
}
665+
666+
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
667+
func(parsedName, constraint any) (bool, error) {
668+
return matchURIConstraint(parsedName.(*url.URL), constraint.(string))
669+
}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
670+
return err
671+
}
672+
673+
case nameTypeIP:
674+
ip := net.IP(data)
675+
if l := len(ip); l != net.IPv4len && l != net.IPv6len {
676+
return fmt.Errorf("x509: internal error: IP SAN %x failed to parse", data)
677+
}
678+
679+
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "IP address", ip.String(), ip,
680+
func(parsedName, constraint any) (bool, error) {
681+
return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet))
682+
}, c.PermittedIPRanges, c.ExcludedIPRanges); err != nil {
683+
return err
684+
}
685+
686+
default:
687+
// Unknown SAN types are ignored.
649688
}
650689

651-
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "IP address", ip.String(), ip,
652-
func(parsedName, constraint any) (bool, error) {
653-
return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet))
654-
}, c.PermittedIPRanges, c.ExcludedIPRanges); err != nil {
655-
return err
656-
}
690+
return nil
691+
})
657692

658-
default:
659-
// Unknown SAN types are ignored.
693+
if err != nil {
694+
return err
660695
}
661-
662-
return nil
663-
})
664-
665-
if err != nil {
666-
return err
667696
}
668697
}
669698

@@ -767,6 +796,10 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e
767796
}
768797
}
769798

799+
if len(opts.KeyUsages) == 0 {
800+
opts.KeyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
801+
}
802+
770803
err = c.isValid(leafCertificate, nil, &opts)
771804
if err != nil {
772805
return
@@ -779,38 +812,10 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e
779812
}
780813
}
781814

782-
var candidateChains [][]*Certificate
783815
if opts.Roots.contains(c) {
784-
candidateChains = append(candidateChains, []*Certificate{c})
785-
} else {
786-
if candidateChains, err = c.buildChains(nil, []*Certificate{c}, nil, &opts); err != nil {
787-
return nil, err
788-
}
789-
}
790-
791-
keyUsages := opts.KeyUsages
792-
if len(keyUsages) == 0 {
793-
keyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
794-
}
795-
796-
// If any key usage is acceptable then we're done.
797-
for _, usage := range keyUsages {
798-
if usage == ExtKeyUsageAny {
799-
return candidateChains, nil
800-
}
801-
}
802-
803-
for _, candidate := range candidateChains {
804-
if checkChainForKeyUsage(candidate, keyUsages) {
805-
chains = append(chains, candidate)
806-
}
807-
}
808-
809-
if len(chains) == 0 {
810-
return nil, CertificateInvalidError{c, IncompatibleUsage, ""}
816+
return [][]*Certificate{{c}}, nil
811817
}
812-
813-
return chains, nil
818+
return c.buildChains([]*Certificate{c}, nil, &opts)
814819
}
815820

816821
func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate {
@@ -826,15 +831,22 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate
826831
// for failed checks due to different intermediates having the same Subject.
827832
const maxChainSignatureChecks = 100
828833

829-
func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, currentChain []*Certificate, sigChecks *int, opts *VerifyOptions) (chains [][]*Certificate, err error) {
834+
func (c *Certificate) buildChains(currentChain []*Certificate, sigChecks *int, opts *VerifyOptions) (chains [][]*Certificate, err error) {
830835
var (
831836
hintErr error
832837
hintCert *Certificate
833838
)
834839

840+
type pubKeyEqual interface {
841+
Equal(crypto.PublicKey) bool
842+
}
843+
835844
considerCandidate := func(certType int, candidate *Certificate) {
836845
for _, cert := range currentChain {
837-
if cert.Equal(candidate) {
846+
// If a certificate already appeared in the chain we've built, don't
847+
// reconsider it. This prevents loops, for isntance those created by
848+
// mutual cross-signatures, or other cross-signature bridges oddities.
849+
if bytes.Equal(cert.RawSubject, candidate.RawSubject) && cert.PublicKey.(pubKeyEqual).Equal(candidate.PublicKey) {
838850
return
839851
}
840852
}
@@ -865,14 +877,8 @@ func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, curre
865877
case rootCertificate:
866878
chains = append(chains, appendToFreshChain(currentChain, candidate))
867879
case intermediateCertificate:
868-
if cache == nil {
869-
cache = make(map[*Certificate][][]*Certificate)
870-
}
871-
childChains, ok := cache[candidate]
872-
if !ok {
873-
childChains, err = candidate.buildChains(cache, appendToFreshChain(currentChain, candidate), sigChecks, opts)
874-
cache[candidate] = childChains
875-
}
880+
var childChains [][]*Certificate
881+
childChains, err = candidate.buildChains(appendToFreshChain(currentChain, candidate), sigChecks, opts)
876882
chains = append(chains, childChains...)
877883
}
878884
}

0 commit comments

Comments
 (0)