diff --git a/smx509/verify.go b/smx509/verify.go index be2c856..29e781a 100644 --- a/smx509/verify.go +++ b/smx509/verify.go @@ -2,6 +2,7 @@ package smx509 import ( "bytes" + "crypto" "crypto/x509" "errors" "fmt" @@ -487,73 +488,101 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V leaf = currentChain[0] } - if (certType == intermediateCertificate || certType == rootCertificate) && - c.hasNameConstraints() && leaf.hasSANExtension() { - err := forEachSAN(leaf.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 (len(c.ExtKeyUsage) > 0 || len(c.UnknownExtKeyUsage) > 0) && len(opts.KeyUsages) > 0 { + acceptableUsage := false + um := make(map[ExtKeyUsage]bool, len(opts.KeyUsages)) + for _, u := range opts.KeyUsages { + um[u] = true + } + if !um[ExtKeyUsageAny] { + for _, u := range c.ExtKeyUsage { + if u == ExtKeyUsageAny || um[u] { + acceptableUsage = true + break } - - if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox, - func(parsedName, constraint interface{}) (bool, error) { - return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) - }, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil { - return err - } - - case nameTypeDNS: - name := string(data) - if _, ok := domainToReverseLabels(name); !ok { - return fmt.Errorf("x509: cannot parse dnsName %q", name) - } - - if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name, - func(parsedName, constraint interface{}) (bool, error) { - return matchDomainConstraint(parsedName.(string), constraint.(string)) - }, 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 interface{}) (bool, error) { - return matchURIConstraint(parsedName.(*url.URL), constraint.(string)) - }, 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 interface{}) (bool, error) { - return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet)) - }, c.PermittedIPRanges, c.ExcludedIPRanges); err != nil { - return err - } - - default: - // Unknown SAN types are ignored. } + if !acceptableUsage { + return CertificateInvalidError{Cert: c.asX509(), Reason: IncompatibleUsage, Detail: ""} + } + } + } - return nil - }) + if (certType == intermediateCertificate || certType == rootCertificate) && + c.hasNameConstraints() { + toCheck := []*Certificate{} + if leaf.hasSANExtension() { + toCheck = append(toCheck, leaf) + } + 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 != nil { - return err + if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox, + func(parsedName, constraint interface{}) (bool, error) { + return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) + }, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil { + return err + } + + case nameTypeDNS: + name := string(data) + if _, ok := domainToReverseLabels(name); !ok { + return fmt.Errorf("x509: cannot parse dnsName %q", name) + } + + if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name, + func(parsedName, constraint interface{}) (bool, error) { + return matchDomainConstraint(parsedName.(string), constraint.(string)) + }, 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 interface{}) (bool, error) { + return matchURIConstraint(parsedName.(*url.URL), constraint.(string)) + }, 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 interface{}) (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 + } } } @@ -654,6 +683,10 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e } } + if len(opts.KeyUsages) == 0 { + opts.KeyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth} + } + err = c.isValid(leafCertificate, nil, &opts) if err != nil { return @@ -666,38 +699,10 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e } } - var candidateChains [][]*Certificate if opts.Roots.contains(c) { - candidateChains = append(candidateChains, []*Certificate{c}) - } else { - if candidateChains, err = c.buildChains(nil, []*Certificate{c}, nil, &opts); err != nil { - return nil, err - } + return [][]*Certificate{{c}}, nil } - - keyUsages := opts.KeyUsages - if len(keyUsages) == 0 { - keyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth} - } - - // If any key usage is acceptable then we're done. - for _, usage := range keyUsages { - if usage == ExtKeyUsageAny { - return candidateChains, nil - } - } - - for _, candidate := range candidateChains { - if checkChainForKeyUsage(candidate, keyUsages) { - chains = append(chains, candidate) - } - } - - if len(chains) == 0 { - return nil, CertificateInvalidError{Cert: c.asX509(), Reason: IncompatibleUsage, Detail: ""} - } - - return chains, nil + return c.buildChains([]*Certificate{c}, nil, &opts) } func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate { @@ -713,15 +718,21 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate // for failed checks due to different intermediates having the same Subject. const maxChainSignatureChecks = 100 -func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, currentChain []*Certificate, sigChecks *int, opts *VerifyOptions) (chains [][]*Certificate, err error) { +func (c *Certificate) buildChains(currentChain []*Certificate, sigChecks *int, opts *VerifyOptions) (chains [][]*Certificate, err error) { var ( hintErr error hintCert *Certificate ) + type pubKeyEqual interface { + Equal(crypto.PublicKey) bool + } considerCandidate := func(certType int, candidate *Certificate) { for _, cert := range currentChain { - if cert.Equal(candidate) { + // If a certificate already appeared in the chain we've built, don't + // reconsider it. This prevents loops, for isntance those created by + // mutual cross-signatures, or other cross-signature bridges oddities. + if bytes.Equal(cert.RawSubject, candidate.RawSubject) && cert.PublicKey.(pubKeyEqual).Equal(candidate.PublicKey) { return } } @@ -752,14 +763,8 @@ func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, curre case rootCertificate: chains = append(chains, appendToFreshChain(currentChain, candidate)) case intermediateCertificate: - if cache == nil { - cache = make(map[*Certificate][][]*Certificate) - } - childChains, ok := cache[candidate] - if !ok { - childChains, err = candidate.buildChains(cache, appendToFreshChain(currentChain, candidate), sigChecks, opts) - cache[candidate] = childChains - } + var childChains [][]*Certificate + childChains, err = candidate.buildChains(appendToFreshChain(currentChain, candidate), sigChecks, opts) chains = append(chains, childChains...) } } diff --git a/smx509/verify_test.go b/smx509/verify_test.go index 047cd27..86ad405 100644 --- a/smx509/verify_test.go +++ b/smx509/verify_test.go @@ -11,7 +11,9 @@ import ( "errors" "fmt" "math/big" + "reflect" "runtime" + "sort" "strings" "testing" "time" @@ -1850,11 +1852,491 @@ func TestSystemRootsError(t *testing.T) { } } -/* func TestSystemRootsErrorUnwrap(t *testing.T) { var err1 = errors.New("err1") err := x509.SystemRootsError{Err: err1} if !errors.Is(err, err1) { t.Error("errors.Is failed, wanted success") } -}*/ +} + +func TestIssue51759(t *testing.T) { + // badCertData contains a cert that we parse as valid + // but that macOS SecCertificateCreateWithData rejects. + const badCertData = "0\x82\x01U0\x82\x01\a\xa0\x03\x02\x01\x02\x02\x01\x020\x05\x06\x03+ep0R1P0N\x06\x03U\x04\x03\x13Gderpkey8dc58100b2493614ee1692831a461f3f4dd3f9b3b088e244f887f81b4906ac260\x1e\x17\r220112235755Z\x17\r220313235755Z0R1P0N\x06\x03U\x04\x03\x13Gderpkey8dc58100b2493614ee1692831a461f3f4dd3f9b3b088e244f887f81b4906ac260*0\x05\x06\x03+ep\x03!\x00bA\xd8e\xadW\xcb\xefZ\x89\xb5\"\x1eR\x9d\xba\x0e:\x1042Q@\u007f\xbd\xfb{ks\x04\xd1£\x020\x000\x05\x06\x03+ep\x03A\x00[\xa7\x06y\x86(\x94\x97\x9eLwA\x00\x01x\xaa\xbc\xbd Ê]\n(΅!ف0\xf5\x9a%I\x19<\xffo\xf1\xeaaf@\xb1\xa7\xaf\xfd\xe9R\xc7\x0f\x8d&\xd5\xfc\x0f;Ϙ\x82\x84a\xbc\r" + badCert, err := ParseCertificate([]byte(badCertData)) + if err != nil { + t.Fatal(err) + } + + t.Run("leaf", func(t *testing.T) { + opts := VerifyOptions{} + _, err = badCert.Verify(opts) + if err == nil { + t.Fatal("expected error") + } + }) + + goodCert, err := certificateFromPEM(googleLeaf) + if err != nil { + t.Fatal(err) + } + + t.Run("intermediate", func(t *testing.T) { + opts := VerifyOptions{ + Intermediates: NewCertPool(), + } + opts.Intermediates.AddCert(badCert) + _, err = goodCert.Verify(opts) + if err == nil { + t.Fatal("expected error") + } + }) +} + +type trustGraphEdge struct { + Issuer string + Subject string + Type int + MutateTemplate func(*Certificate) +} + +type trustGraphDescription struct { + Roots []string + Leaf string + Graph []trustGraphEdge +} + +func genCertEdge(t *testing.T, subject string, key crypto.Signer, mutateTmpl func(*Certificate), certType int, issuer *Certificate, signer crypto.Signer) *Certificate { + t.Helper() + + serial, err := rand.Int(rand.Reader, big.NewInt(100)) + if err != nil { + t.Fatalf("failed to generate test serial: %s", err) + } + tmpl := &Certificate{ + SerialNumber: serial, + Subject: pkix.Name{CommonName: subject}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + } + if certType == rootCertificate || certType == intermediateCertificate { + tmpl.IsCA, tmpl.BasicConstraintsValid = true, true + tmpl.KeyUsage = KeyUsageCertSign + } else if certType == leafCertificate { + tmpl.DNSNames = []string{"localhost"} + } + if mutateTmpl != nil { + mutateTmpl(tmpl) + } + + if certType == rootCertificate { + issuer = tmpl + signer = key + } + + d, err := CreateCertificate(rand.Reader, tmpl.asX509(), issuer.asX509(), key.Public(), signer) + if err != nil { + t.Fatalf("failed to generate test cert: %s", err) + } + c, err := ParseCertificate(d) + if err != nil { + t.Fatalf("failed to parse test cert: %s", err) + } + return c +} + +func buildTrustGraph(t *testing.T, d trustGraphDescription) (*CertPool, *CertPool, *Certificate) { + t.Helper() + + certs := map[string]*Certificate{} + keys := map[string]crypto.Signer{} + roots := []*Certificate{} + for _, r := range d.Roots { + k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("failed to generate test key: %s", err) + } + root := genCertEdge(t, r, k, nil, rootCertificate, nil, nil) + roots = append(roots, root) + certs[r] = root + keys[r] = k + } + + intermediates := []*Certificate{} + var leaf *Certificate + for _, e := range d.Graph { + issuerCert, ok := certs[e.Issuer] + if !ok { + t.Fatalf("unknown issuer %s", e.Issuer) + } + issuerKey, ok := keys[e.Issuer] + if !ok { + t.Fatalf("unknown issuer %s", e.Issuer) + } + + k, ok := keys[e.Subject] + if !ok { + var err error + k, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("failed to generate test key: %s", err) + } + keys[e.Subject] = k + } + cert := genCertEdge(t, e.Subject, k, e.MutateTemplate, e.Type, issuerCert, issuerKey) + certs[e.Subject] = cert + if e.Subject == d.Leaf { + leaf = cert + } else { + intermediates = append(intermediates, cert) + } + } + + rootPool, intermediatePool := NewCertPool(), NewCertPool() + for i := len(roots) - 1; i >= 0; i-- { + rootPool.AddCert(roots[i]) + } + for i := len(intermediates) - 1; i >= 0; i-- { + intermediatePool.AddCert(intermediates[i]) + } + + return rootPool, intermediatePool, leaf +} + +func chainsToStrings(chains [][]*Certificate) []string { + chainStrings := []string{} + for _, chain := range chains { + names := []string{} + for _, c := range chain { + names = append(names, c.Subject.String()) + } + chainStrings = append(chainStrings, strings.Join(names, " -> ")) + } + sort.Strings(chainStrings) + return chainStrings +} + +func TestPathBuilding(t *testing.T) { + tests := []struct { + name string + graph trustGraphDescription + expectedChains []string + expectedErr string + }{ + { + // Build the following graph from RFC 4158, figure 7 (note that in this graph edges represent + // certificates where the parent is the issuer and the child is the subject.) For the certificate + // C->B, use an unsupported ExtKeyUsage (in this case ExtKeyUsageCodeSigning) which invalidates + // the path Trust Anchor -> C -> B -> EE. The remaining valid paths should be: + // * Trust Anchor -> A -> B -> EE + // * Trust Anchor -> C -> A -> B -> EE + // + // +---------+ + // | Trust | + // | Anchor | + // +---------+ + // | | + // v v + // +---+ +---+ + // | A |<-->| C | + // +---+ +---+ + // | | + // | +---+ | + // +->| B |<-+ + // +---+ + // | + // v + // +----+ + // | EE | + // +----+ + name: "bad EKU 1", + graph: trustGraphDescription{ + Roots: []string{"root"}, + Leaf: "leaf", + Graph: []trustGraphEdge{ + { + Issuer: "root", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "root", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter b", + Type: intermediateCertificate, + MutateTemplate: func(t *Certificate) { + t.ExtKeyUsage = []ExtKeyUsage{ExtKeyUsageCodeSigning} + }, + }, + { + Issuer: "inter a", + Subject: "inter b", + Type: intermediateCertificate, + }, + { + Issuer: "inter b", + Subject: "leaf", + Type: leafCertificate, + }, + }, + }, + expectedChains: []string{ + "CN=leaf -> CN=inter b -> CN=inter a -> CN=inter c -> CN=root", + "CN=leaf -> CN=inter b -> CN=inter a -> CN=root", + }, + }, + { + // Build the following graph from RFC 4158, figure 7 (note that in this graph edges represent + // certificates where the parent is the issuer and the child is the subject.) For the certificate + // C->B, use a unconstrained SAN which invalidates the path Trust Anchor -> C -> B -> EE. The + // remaining valid paths should be: + // * Trust Anchor -> A -> B -> EE + // * Trust Anchor -> C -> A -> B -> EE + // + // +---------+ + // | Trust | + // | Anchor | + // +---------+ + // | | + // v v + // +---+ +---+ + // | A |<-->| C | + // +---+ +---+ + // | | + // | +---+ | + // +->| B |<-+ + // +---+ + // | + // v + // +----+ + // | EE | + // +----+ + name: "bad EKU 2", + graph: trustGraphDescription{ + Roots: []string{"root"}, + Leaf: "leaf", + Graph: []trustGraphEdge{ + { + Issuer: "root", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "root", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter b", + Type: intermediateCertificate, + MutateTemplate: func(t *Certificate) { + t.PermittedDNSDomains = []string{"good"} + t.DNSNames = []string{"bad"} + }, + }, + { + Issuer: "inter a", + Subject: "inter b", + Type: intermediateCertificate, + }, + { + Issuer: "inter b", + Subject: "leaf", + Type: leafCertificate, + }, + }, + }, + expectedChains: []string{ + "CN=leaf -> CN=inter b -> CN=inter a -> CN=inter c -> CN=root", + "CN=leaf -> CN=inter b -> CN=inter a -> CN=root", + }, + }, + { + // Build the following graph, we should find both paths: + // * Trust Anchor -> A -> C -> EE + // * Trust Anchor -> A -> B -> C -> EE + // + // +---------+ + // | Trust | + // | Anchor | + // +---------+ + // | + // v + // +---+ + // | A | + // +---+ + // | | + // | +----+ + // | v + // | +---+ + // | | B | + // | +---+ + // | | + // | +---v + // v v + // +---+ + // | C | + // +---+ + // | + // v + // +----+ + // | EE | + // +----+ + name: "all paths", + graph: trustGraphDescription{ + Roots: []string{"root"}, + Leaf: "leaf", + Graph: []trustGraphEdge{ + { + Issuer: "root", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter b", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter b", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "leaf", + Type: leafCertificate, + }, + }, + }, + expectedChains: []string{ + "CN=leaf -> CN=inter c -> CN=inter a -> CN=root", + "CN=leaf -> CN=inter c -> CN=inter b -> CN=inter a -> CN=root", + }, + }, + { + // Build the following graph, which contains a cross-signature loop + // (A and C cross sign each other). Paths that include the A -> C -> A + // (and vice versa) loop should be ignored, resulting in the paths: + // * Trust Anchor -> A -> B -> EE + // * Trust Anchor -> C -> B -> EE + // * Trust Anchor -> A -> C -> B -> EE + // * Trust Anchor -> C -> A -> B -> EE + // + // +---------+ + // | Trust | + // | Anchor | + // +---------+ + // | | + // v v + // +---+ +---+ + // | A |<-->| C | + // +---+ +---+ + // | | + // | +---+ | + // +->| B |<-+ + // +---+ + // | + // v + // +----+ + // | EE | + // +----+ + name: "ignore cross-sig loops", + graph: trustGraphDescription{ + Roots: []string{"root"}, + Leaf: "leaf", + Graph: []trustGraphEdge{ + { + Issuer: "root", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "root", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter a", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter c", + Type: intermediateCertificate, + }, + { + Issuer: "inter c", + Subject: "inter b", + Type: intermediateCertificate, + }, + { + Issuer: "inter a", + Subject: "inter b", + Type: intermediateCertificate, + }, + { + Issuer: "inter b", + Subject: "leaf", + Type: leafCertificate, + }, + }, + }, + expectedChains: []string{ + "CN=leaf -> CN=inter b -> CN=inter a -> CN=inter c -> CN=root", + "CN=leaf -> CN=inter b -> CN=inter a -> CN=root", + "CN=leaf -> CN=inter b -> CN=inter c -> CN=inter a -> CN=root", + "CN=leaf -> CN=inter b -> CN=inter c -> CN=root", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + roots, intermediates, leaf := buildTrustGraph(t, tc.graph) + chains, err := leaf.Verify(VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }) + if err != nil && err.Error() != tc.expectedErr { + t.Fatalf("unexpected error: got %q, want %q", err, tc.expectedErr) + } + gotChains := chainsToStrings(chains) + if !reflect.DeepEqual(gotChains, tc.expectedChains) { + t.Errorf("unexpected chains returned:\ngot:\n\t%s\nwant:\n\t%s", strings.Join(gotChains, "\n\t"), strings.Join(tc.expectedChains, "\n\t")) + } + }) + } +}