crypto/x509: fix EKU nesting enforcement

The path building rework broke the enforcement of EKU nesting, this
change goes back to using the old method of enforcement, since it ends
up being more efficient to check the chains after building, rather than
at each step during path building.

Fixes #52659

Change-Id: Ic7c3717a10c33905677cf7bc4bc0a20f5f15f259
Reviewed-on: https://go-review.googlesource.com/c/go/+/403554
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <valsorda@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>

src/crypto/x509/name_constraints_test.go

@@ -1860,124 +1860,131 @@ func TestConstraintCases(t *testing.T) {
 	}
 
 	for i, test := range nameConstraintsTests {
-		rootPool := NewCertPool()
+		t.Run(fmt.Sprintf("#%d", i), func(t *testing.T) {
-		rootKey := privateKeys.Get().(*ecdsa.PrivateKey)
+			rootPool := NewCertPool()
-		rootName := "Root " + strconv.Itoa(i)
+			rootKey := privateKeys.Get().(*ecdsa.PrivateKey)
+			rootName := "Root " + strconv.Itoa(i)
 
-		// keys keeps track of all the private keys used in a given
+			// keys keeps track of all the private keys used in a given
-		// test and puts them back in the privateKeys pool at the end.
+			// test and puts them back in the privateKeys pool at the end.
-		keys := []*ecdsa.PrivateKey{rootKey}
+			keys := []*ecdsa.PrivateKey{rootKey}
 
-		// At each level (root, intermediate(s), leaf), parent points to
+			// At each level (root, intermediate(s), leaf), parent points to
-		// an example parent certificate and parentKey the key for the
+			// an example parent certificate and parentKey the key for the
-		// parent level. Since all certificates at a given level have
+			// parent level. Since all certificates at a given level have
-		// the same name and public key, any parent certificate is
+			// the same name and public key, any parent certificate is
-		// sufficient to get the correct issuer name and authority
+			// sufficient to get the correct issuer name and authority
-		// key ID.
+			// key ID.
-		var parent *Certificate
+			var parent *Certificate
-		parentKey := rootKey
+			parentKey := rootKey
 
-		for _, root := range test.roots {
+			for _, root := range test.roots {
-			rootCert, err := makeConstraintsCACert(root, rootName, rootKey, nil, rootKey)
+				rootCert, err := makeConstraintsCACert(root, rootName, rootKey, nil, rootKey)
-			if err != nil {
-				t.Fatalf("#%d: failed to create root: %s", i, err)
-			}
-
-			parent = rootCert
-			rootPool.AddCert(rootCert)
-		}
-
-		intermediatePool := NewCertPool()
-
-		for level, intermediates := range test.intermediates {
-			levelKey := privateKeys.Get().(*ecdsa.PrivateKey)
-			keys = append(keys, levelKey)
-			levelName := "Intermediate level " + strconv.Itoa(level)
-			var last *Certificate
-
-			for _, intermediate := range intermediates {
-				caCert, err := makeConstraintsCACert(intermediate, levelName, levelKey, parent, parentKey)
 				if err != nil {
-					t.Fatalf("#%d: failed to create %q: %s", i, levelName, err)
+					t.Fatalf("failed to create root: %s", err)
 				}
 
-				last = caCert
+				parent = rootCert
-				intermediatePool.AddCert(caCert)
+				rootPool.AddCert(rootCert)
 			}
 
-			parent = last
+			intermediatePool := NewCertPool()
-			parentKey = levelKey
-		}
 
-		leafKey := privateKeys.Get().(*ecdsa.PrivateKey)
+			for level, intermediates := range test.intermediates {
-		keys = append(keys, leafKey)
+				levelKey := privateKeys.Get().(*ecdsa.PrivateKey)
+				keys = append(keys, levelKey)
+				levelName := "Intermediate level " + strconv.Itoa(level)
+				var last *Certificate
 
-		leafCert, err := makeConstraintsLeafCert(test.leaf, leafKey, parent, parentKey)
+				for _, intermediate := range intermediates {
-		if err != nil {
+					caCert, err := makeConstraintsCACert(intermediate, levelName, levelKey, parent, parentKey)
-			t.Fatalf("#%d: cannot create leaf: %s", i, err)
+					if err != nil {
-		}
+						t.Fatalf("failed to create %q: %s", levelName, err)
+					}
 
-		// Skip tests with CommonName set because OpenSSL will try to match it
+					last = caCert
-		// against name constraints, while we ignore it when it's not hostname-looking.
+					intermediatePool.AddCert(caCert)
-		if !test.noOpenSSL && testNameConstraintsAgainstOpenSSL && test.leaf.cn == "" {
-			output, err := testChainAgainstOpenSSL(t, leafCert, intermediatePool, rootPool)
-			if err == nil && len(test.expectedError) > 0 {
-				t.Errorf("#%d: unexpectedly succeeded against OpenSSL", i)
-				if debugOpenSSLFailure {
-					return
 				}
+
+				parent = last
+				parentKey = levelKey
 			}
 
+			leafKey := privateKeys.Get().(*ecdsa.PrivateKey)
+			keys = append(keys, leafKey)
+
+			leafCert, err := makeConstraintsLeafCert(test.leaf, leafKey, parent, parentKey)
 			if err != nil {
-				if _, ok := err.(*exec.ExitError); !ok {
+				t.Fatalf("cannot create leaf: %s", err)
-					t.Errorf("#%d: OpenSSL failed to run: %s", i, err)
+			}
-				} else if len(test.expectedError) == 0 {
+
-					t.Errorf("#%d: OpenSSL unexpectedly failed: %v", i, output)
+			// Skip tests with CommonName set because OpenSSL will try to match it
+			// against name constraints, while we ignore it when it's not hostname-looking.
+			if !test.noOpenSSL && testNameConstraintsAgainstOpenSSL && test.leaf.cn == "" {
+				output, err := testChainAgainstOpenSSL(t, leafCert, intermediatePool, rootPool)
+				if err == nil && len(test.expectedError) > 0 {
+					t.Error("unexpectedly succeeded against OpenSSL")
 					if debugOpenSSLFailure {
 						return
 					}
 				}
+
+				if err != nil {
+					if _, ok := err.(*exec.ExitError); !ok {
+						t.Errorf("OpenSSL failed to run: %s", err)
+					} else if len(test.expectedError) == 0 {
+						t.Errorf("OpenSSL unexpectedly failed: %v", output)
+						if debugOpenSSLFailure {
+							return
+						}
+					}
+				}
 			}
-		}
 
-		verifyOpts := VerifyOptions{
+			verifyOpts := VerifyOptions{
-			Roots:         rootPool,
+				Roots:         rootPool,
-			Intermediates: intermediatePool,
+				Intermediates: intermediatePool,
-			CurrentTime:   time.Unix(1500, 0),
+				CurrentTime:   time.Unix(1500, 0),
-			KeyUsages:     test.requestedEKUs,
+				KeyUsages:     test.requestedEKUs,
-		}
+			}
-		_, err = leafCert.Verify(verifyOpts)
+			_, err = leafCert.Verify(verifyOpts)
 
-		logInfo := true
+			logInfo := true
-		if len(test.expectedError) == 0 {
+			if len(test.expectedError) == 0 {
-			if err != nil {
+				if err != nil {
-				t.Errorf("#%d: unexpected failure: %s", i, err)
+					t.Errorf("unexpected failure: %s", err)
+				} else {
+					logInfo = false
+				}
 			} else {
-				logInfo = false
+				if err == nil {
+					t.Error("unexpected success")
+				} else if !strings.Contains(err.Error(), test.expectedError) {
+					t.Errorf("expected error containing %q, but got: %s", test.expectedError, err)
+				} else {
+					logInfo = false
+				}
 			}
-		} else {
-			if err == nil {
-				t.Errorf("#%d: unexpected success", i)
-			} else if !strings.Contains(err.Error(), test.expectedError) {
-				t.Errorf("#%d: expected error containing %q, but got: %s", i, test.expectedError, err)
-			} else {
-				logInfo = false
-			}
-		}
 
-		if logInfo {
+			if logInfo {
-			certAsPEM := func(cert *Certificate) string {
+				certAsPEM := func(cert *Certificate) string {
-				var buf bytes.Buffer
+					var buf bytes.Buffer
-				pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
+					pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
-				return buf.String()
+					return buf.String()
+				}
+				t.Errorf("root:\n%s", certAsPEM(rootPool.mustCert(t, 0)))
+				if intermediates := allCerts(t, intermediatePool); len(intermediates) > 0 {
+					for ii, intermediate := range intermediates {
+						t.Errorf("intermediate %d:\n%s", ii, certAsPEM(intermediate))
+					}
+				}
+				t.Errorf("leaf:\n%s", certAsPEM(leafCert))
 			}
-			t.Errorf("#%d: root:\n%s", i, certAsPEM(rootPool.mustCert(t, 0)))
-			t.Errorf("#%d: leaf:\n%s", i, certAsPEM(leafCert))
-		}
 
-		for _, key := range keys {
+			for _, key := range keys {
-			privateKeys.Put(key)
+				privateKeys.Put(key)
-		}
+			}
+		})
 	}
 }
 

src/crypto/x509/verify.go

@@ -599,25 +599,6 @@ 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{c, IncompatibleUsage, ""}
-			}
-		}
-	}
-
 	if (certType == intermediateCertificate || certType == rootCertificate) &&
 		c.hasNameConstraints() {
 		toCheck := []*Certificate{}
@@ -804,10 +785,6 @@ 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
@@ -820,10 +797,40 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e
 		}
 	}
 
+	var candidateChains [][]*Certificate
 	if opts.Roots.contains(c) {
-		return [][]*Certificate{{c}}, nil
+		candidateChains = [][]*Certificate{{c}}
+	} else {
+		candidateChains, err = c.buildChains([]*Certificate{c}, nil, &opts)
+		if err != nil {
+			return nil, err
+		}
 	}
-	return c.buildChains([]*Certificate{c}, nil, &opts)
+
+	if len(opts.KeyUsages) == 0 {
+		opts.KeyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
+	}
+
+	for _, eku := range opts.KeyUsages {
+		if eku == ExtKeyUsageAny {
+			// If any key usage is acceptable, no need to check the chain for
+			// key usages.
+			return candidateChains, nil
+		}
+	}
+
+	chains = make([][]*Certificate, 0, len(candidateChains))
+	for _, candidate := range candidateChains {
+		if checkChainForKeyUsage(candidate, opts.KeyUsages) {
+			chains = append(chains, candidate)
+		}
+	}
+
+	if len(chains) == 0 {
+		return nil, CertificateInvalidError{c, IncompatibleUsage, ""}
+	}
+
+	return chains, nil
 }
 
 func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate {

src/crypto/x509/verify_test.go

@@ -10,6 +10,7 @@ import (
 	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/x509/pkix"
+	"encoding/asn1"
 	"encoding/pem"
 	"errors"
 	"fmt"
@@ -2363,6 +2364,54 @@ func TestPathBuilding(t *testing.T) {
 				"CN=root -> CN=root",
 			},
 		},
+		{
+			// Build a basic graph with two paths from leaf to root, but the path passing
+			// through C should be ignored, because it has invalid EKU nesting.
+			name: "ignore invalid EKU path",
+			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 b",
+						Type:    intermediateCertificate,
+						MutateTemplate: func(t *Certificate) {
+							t.ExtKeyUsage = []ExtKeyUsage{ExtKeyUsageCodeSigning}
+						},
+					},
+					{
+						Issuer:  "inter a",
+						Subject: "inter b",
+						Type:    intermediateCertificate,
+						MutateTemplate: func(t *Certificate) {
+							t.ExtKeyUsage = []ExtKeyUsage{ExtKeyUsageServerAuth}
+						},
+					},
+					{
+						Issuer:  "inter b",
+						Subject: "leaf",
+						Type:    leafCertificate,
+						MutateTemplate: func(t *Certificate) {
+							t.ExtKeyUsage = []ExtKeyUsage{ExtKeyUsageServerAuth}
+						},
+					},
+				},
+			},
+			expectedChains: []string{
+				"CN=leaf -> CN=inter b -> CN=inter a -> CN=root",
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -2382,3 +2431,194 @@ func TestPathBuilding(t *testing.T) {
 		})
 	}
 }
+
+func TestEKUEnforcement(t *testing.T) {
+	type ekuDescs struct {
+		EKUs    []ExtKeyUsage
+		Unknown []asn1.ObjectIdentifier
+	}
+	tests := []struct {
+		name       string
+		root       ekuDescs
+		inters     []ekuDescs
+		leaf       ekuDescs
+		verifyEKUs []ExtKeyUsage
+		err        string
+	}{
+		{
+			name:       "valid, full chain",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			inters:     []ekuDescs{ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+		},
+		{
+			name:       "valid, only leaf has EKU",
+			root:       ekuDescs{},
+			inters:     []ekuDescs{ekuDescs{}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+		},
+		{
+			name:       "invalid, serverAuth not nested",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageClientAuth}},
+			inters:     []ekuDescs{ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			err:        "x509: certificate specifies an incompatible key usage",
+		},
+		{
+			name:       "valid, two EKUs, one path",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			inters:     []ekuDescs{ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth},
+		},
+		{
+			name: "invalid, ladder",
+			root: ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			inters: []ekuDescs{
+				ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}},
+				ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageClientAuth}},
+				ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth}},
+				ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth},
+			err:        "x509: certificate specifies an incompatible key usage",
+		},
+		{
+			name:       "valid, intermediate has no EKU",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			inters:     []ekuDescs{ekuDescs{}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+		},
+		{
+			name:       "invalid, intermediate has no EKU and no nested path",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageClientAuth}},
+			inters:     []ekuDescs{ekuDescs{}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth, ExtKeyUsageClientAuth},
+			err:        "x509: certificate specifies an incompatible key usage",
+		},
+		{
+			name:       "invalid, intermediate has unknown EKU",
+			root:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			inters:     []ekuDescs{ekuDescs{Unknown: []asn1.ObjectIdentifier{{1, 2, 3}}}},
+			leaf:       ekuDescs{EKUs: []ExtKeyUsage{ExtKeyUsageServerAuth}},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			err:        "x509: certificate specifies an incompatible key usage",
+		},
+	}
+
+	k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("failed to generate test key: %s", err)
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			rootPool := NewCertPool()
+			root := genCertEdge(t, "root", k, func(c *Certificate) {
+				c.ExtKeyUsage = tc.root.EKUs
+				c.UnknownExtKeyUsage = tc.root.Unknown
+			}, rootCertificate, nil, k)
+			rootPool.AddCert(root)
+
+			parent := root
+			interPool := NewCertPool()
+			for i, interEKUs := range tc.inters {
+				inter := genCertEdge(t, fmt.Sprintf("inter %d", i), k, func(c *Certificate) {
+					c.ExtKeyUsage = interEKUs.EKUs
+					c.UnknownExtKeyUsage = interEKUs.Unknown
+				}, intermediateCertificate, parent, k)
+				interPool.AddCert(inter)
+				parent = inter
+			}
+
+			leaf := genCertEdge(t, "leaf", k, func(c *Certificate) {
+				c.ExtKeyUsage = tc.leaf.EKUs
+				c.UnknownExtKeyUsage = tc.leaf.Unknown
+			}, intermediateCertificate, parent, k)
+
+			_, err := leaf.Verify(VerifyOptions{Roots: rootPool, Intermediates: interPool, KeyUsages: tc.verifyEKUs})
+			if err == nil && tc.err != "" {
+				t.Errorf("expected error")
+			} else if err != nil && err.Error() != tc.err {
+				t.Errorf("unexpected error: want %q, got %q", err.Error(), tc.err)
+			}
+		})
+	}
+}
+
+func TestVerifyEKURootAsLeaf(t *testing.T) {
+	k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("failed to generate key: %s", err)
+	}
+
+	for _, tc := range []struct {
+		rootEKUs   []ExtKeyUsage
+		verifyEKUs []ExtKeyUsage
+		succeed    bool
+	}{
+		{
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			succeed:    true,
+		},
+		{
+			rootEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			succeed:  true,
+		},
+		{
+			rootEKUs:   []ExtKeyUsage{ExtKeyUsageServerAuth},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			succeed:    true,
+		},
+		{
+			rootEKUs:   []ExtKeyUsage{ExtKeyUsageServerAuth},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageAny},
+			succeed:    true,
+		},
+		{
+			rootEKUs:   []ExtKeyUsage{ExtKeyUsageAny},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			succeed:    true,
+		},
+		{
+			rootEKUs:   []ExtKeyUsage{ExtKeyUsageClientAuth},
+			verifyEKUs: []ExtKeyUsage{ExtKeyUsageServerAuth},
+			succeed:    false,
+		},
+	} {
+		t.Run(fmt.Sprintf("root EKUs %#v, verify EKUs %#v", tc.rootEKUs, tc.verifyEKUs), func(t *testing.T) {
+			tmpl := &Certificate{
+				SerialNumber: big.NewInt(1),
+				Subject:      pkix.Name{CommonName: "root"},
+				NotBefore:    time.Now().Add(-time.Hour),
+				NotAfter:     time.Now().Add(time.Hour),
+				DNSNames:     []string{"localhost"},
+				ExtKeyUsage:  tc.rootEKUs,
+			}
+			rootDER, err := CreateCertificate(rand.Reader, tmpl, tmpl, k.Public(), k)
+			if err != nil {
+				t.Fatalf("failed to create certificate: %s", err)
+			}
+			root, err := ParseCertificate(rootDER)
+			if err != nil {
+				t.Fatalf("failed to parse certificate: %s", err)
+			}
+			roots := NewCertPool()
+			roots.AddCert(root)
+
+			_, err = root.Verify(VerifyOptions{Roots: roots, KeyUsages: tc.verifyEKUs})
+			if err == nil && !tc.succeed {
+				t.Error("verification succeed")
+			} else if err != nil && tc.succeed {
+				t.Errorf("verification failed: %q", err)
+			}
+		})
+	}
+
+}