crypto/x509: rework path building

smx509/verify.go

@@ -2,6 +2,7 @@ package smx509
 
 import (
 	"bytes"
+	"crypto"
 	"crypto/x509"
 	"errors"
 	"fmt"
@@ -487,9 +488,36 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 		leaf = currentChain[0]
 	}
 
+	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 !acceptableUsage {
+				return CertificateInvalidError{Cert: c.asX509(), Reason: IncompatibleUsage, Detail: ""}
+			}
+		}
+	}
+
 	if (certType == intermediateCertificate || certType == rootCertificate) &&
-		c.hasNameConstraints() && leaf.hasSANExtension() {
+		c.hasNameConstraints() {
-		err := forEachSAN(leaf.getSANExtension(), func(tag int, data []byte) error {
+		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)
@@ -556,6 +584,7 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 				return err
 			}
 		}
+	}
 
 	// KeyUsage status flags are ignored. From Engineering Security, Peter
 	// Gutmann: A European government CA marked its signing certificates as
@@ -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})
+		return [][]*Certificate{{c}}, nil
-	} else {
-		if candidateChains, err = c.buildChains(nil, []*Certificate{c}, nil, &opts); err != nil {
-			return nil, err
 	}
-	}
+	return c.buildChains([]*Certificate{c}, nil, &opts)
-
-	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
 }
 
 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 {
+			var childChains [][]*Certificate
-				cache = make(map[*Certificate][][]*Certificate)
+			childChains, err = candidate.buildChains(appendToFreshChain(currentChain, candidate), sigChecks, opts)
-			}
-			childChains, ok := cache[candidate]
-			if !ok {
-				childChains, err = candidate.buildChains(cache, appendToFreshChain(currentChain, candidate), sigChecks, opts)
-				cache[candidate] = childChains
-			}
 			chains = append(chains, childChains...)
 		}
 	}

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"))
+			}
+		})
+	}
+}