[release-branch.go1.10] crypto/x509: check EKUs like 1.9.

This change brings back the EKU checking from 1.9. In 1.10, we checked
EKU nesting independent of the requested EKUs so that, after verifying a
certifciate, one could inspect the EKUs in the leaf and trust them.

That, however, was too optimistic. I had misunderstood that the PKI was
/currently/ clean enough to require that, rather than it being
desirable. Go generally does not push the envelope on these sorts of
things and lets the browsers clear the path first.

Fixes #25258

Change-Id: I18c070478e3bbb6468800ae461c207af9e954949
Reviewed-on: https://go-review.googlesource.com/113475
Reviewed-by: Filippo Valsorda <filippo@golang.org>
(cherry picked from commit 180e0f8a1b149bd1d15df29b6527748266cacad9)
Reviewed-on: https://go-review.googlesource.com/114035
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>

2 files changed, 121 insertions, 162 deletions

diff --git a/src/crypto/x509/name_constraints_test.go b/src/crypto/x509/name_constraints_test.go
index 0172ccf08c..e89e70d874 100644
--- a/src/crypto/x509/name_constraints_test.go
+++ b/src/crypto/x509/name_constraints_test.go
@@ -1222,8 +1222,9 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #63: A specified key usage in an intermediate forbids other usages
-	// in the leaf.
+	// #63: An intermediate with enumerated EKUs causes a failure if we
+	// test for an EKU not in that set. (ServerAuth is required by
+	// default.)
 	nameConstraintsTest{
 		roots: []constraintsSpec{
 			constraintsSpec{},
@@ -1239,11 +1240,11 @@ var nameConstraintsTests = []nameConstraintsTest{
 			sans: []string{"dns:example.com"},
 			ekus: []string{"serverAuth"},
 		},
-		expectedError: "EKU not permitted",
+		expectedError: "incompatible key usage",
 	},
 
-	// #64: A specified key usage in an intermediate forbids other usages
-	// in the leaf, even if we don't recognise them.
+	// #64: an unknown EKU in the leaf doesn't break anything, even if it's not
+	// correctly nested.
 	nameConstraintsTest{
 		roots: []constraintsSpec{
 			constraintsSpec{},
@@ -1259,7 +1260,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 			sans: []string{"dns:example.com"},
 			ekus: []string{"other"},
 		},
-		expectedError: "EKU not permitted",
+		requestedEKUs: []ExtKeyUsage{ExtKeyUsageAny},
 	},
 
 	// #65: trying to add extra permitted key usages in an intermediate
@@ -1284,24 +1285,25 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 	},
 
-	// #66: EKUs in roots are ignored.
+	// #66: EKUs in roots are not ignored.
 	nameConstraintsTest{
 		roots: []constraintsSpec{
 			constraintsSpec{
-				ekus: []string{"serverAuth"},
+				ekus: []string{"email"},
 			},
 		},
 		intermediates: [][]constraintsSpec{
 			[]constraintsSpec{
 				constraintsSpec{
-					ekus: []string{"serverAuth", "email"},
+					ekus: []string{"serverAuth"},
 				},
 			},
 		},
 		leaf: leafSpec{
 			sans: []string{"dns:example.com"},
-			ekus: []string{"serverAuth", "email"},
+			ekus: []string{"serverAuth"},
 		},
+		expectedError: "incompatible key usage",
 	},
 
 	// #67: in order to support COMODO chains, SGC key usages permit
@@ -1447,8 +1449,7 @@ var nameConstraintsTests = []nameConstraintsTest{
 		expectedError: "\"https://example.com/test\" is excluded",
 	},
 
-	// #75: While serverAuth in a CA certificate permits clientAuth in a leaf,
-	// serverAuth in a leaf shouldn't permit clientAuth when requested in
+	// #75: serverAuth in a leaf shouldn't permit clientAuth when requested in
 	// VerifyOptions.
 	nameConstraintsTest{
 		roots: []constraintsSpec{
@@ -1558,6 +1559,27 @@ var nameConstraintsTests = []nameConstraintsTest{
 		},
 		requestedEKUs: []ExtKeyUsage{ExtKeyUsageClientAuth, ExtKeyUsageEmailProtection},
 	},
+
+	// #81: EKUs that are not asserted in VerifyOpts are not required to be
+	// nested.
+	nameConstraintsTest{
+		roots: []constraintsSpec{
+			constraintsSpec{},
+		},
+		intermediates: [][]constraintsSpec{
+			[]constraintsSpec{
+				constraintsSpec{
+					ekus: []string{"serverAuth"},
+				},
+			},
+		},
+		leaf: leafSpec{
+			sans: []string{"dns:example.com"},
+			// There's no email EKU in the intermediate. This would be rejected if
+			// full nesting was required.
+			ekus: []string{"email", "serverAuth"},
+		},
+	},
 }
 
 func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.PrivateKey, parent *Certificate, parentKey *ecdsa.PrivateKey) (*Certificate, error) {
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 0ea214bd2e..60e415b7ec 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -56,8 +56,7 @@ const (
 	// CPU time to verify.
 	TooManyConstraints
 	// CANotAuthorizedForExtKeyUsage results when an intermediate or root
-	// certificate does not permit an extended key usage that is claimed by
-	// the leaf certificate.
+	// certificate does not permit a requested extended key usage.
 	CANotAuthorizedForExtKeyUsage
 )
 
@@ -82,7 +81,7 @@ func (e CertificateInvalidError) Error() string {
 	case TooManyIntermediates:
 		return "x509: too many intermediates for path length constraint"
 	case IncompatibleUsage:
-		return "x509: certificate specifies an incompatible key usage: " + e.Detail
+		return "x509: certificate specifies an incompatible key usage"
 	case NameMismatch:
 		return "x509: issuer name does not match subject from issuing certificate"
 	case NameConstraintsWithoutSANs:
@@ -185,9 +184,8 @@ type VerifyOptions struct {
 	// list means ExtKeyUsageServerAuth. To accept any key usage, include
 	// ExtKeyUsageAny.
 	//
-	// Certificate chains are required to nest extended key usage values,
-	// irrespective of this value. This matches the Windows CryptoAPI behavior,
-	// but not the spec.
+	// Certificate chains are required to nest these extended key usage values.
+	// (This matches the Windows CryptoAPI behavior, but not the spec.)
 	KeyUsages []ExtKeyUsage
 	// MaxConstraintComparisions is the maximum number of comparisons to
 	// perform when checking a given certificate's name constraints. If
@@ -549,51 +547,6 @@ func (c *Certificate) checkNameConstraints(count *int,
 	return nil
 }
 
-const (
-	checkingAgainstIssuerCert = iota
-	checkingAgainstLeafCert
-)
-
-// ekuPermittedBy returns true iff the given extended key usage is permitted by
-// the given EKU from a certificate. Normally, this would be a simple
-// comparison plus a special case for the “any” EKU. But, in order to support
-// existing certificates, some exceptions are made.
-func ekuPermittedBy(eku, certEKU ExtKeyUsage, context int) bool {
-	if certEKU == ExtKeyUsageAny || eku == certEKU {
-		return true
-	}
-
-	// Some exceptions are made to support existing certificates. Firstly,
-	// the ServerAuth and SGC EKUs are treated as a group.
-	mapServerAuthEKUs := func(eku ExtKeyUsage) ExtKeyUsage {
-		if eku == ExtKeyUsageNetscapeServerGatedCrypto || eku == ExtKeyUsageMicrosoftServerGatedCrypto {
-			return ExtKeyUsageServerAuth
-		}
-		return eku
-	}
-
-	eku = mapServerAuthEKUs(eku)
-	certEKU = mapServerAuthEKUs(certEKU)
-
-	if eku == certEKU {
-		return true
-	}
-
-	// If checking a requested EKU against the list in a leaf certificate there
-	// are fewer exceptions.
-	if context == checkingAgainstLeafCert {
-		return false
-	}
-
-	// ServerAuth in a CA permits ClientAuth in the leaf.
-	return (eku == ExtKeyUsageClientAuth && certEKU == ExtKeyUsageServerAuth) ||
-		// Any CA may issue an OCSP responder certificate.
-		eku == ExtKeyUsageOCSPSigning ||
-		// Code-signing CAs can use Microsoft's commercial and
-		// kernel-mode EKUs.
-		(eku == ExtKeyUsageMicrosoftCommercialCodeSigning || eku == ExtKeyUsageMicrosoftKernelCodeSigning) && certEKU == ExtKeyUsageCodeSigning
-}
-
 // isValid performs validity checks on c given that it is a candidate to append
 // to the chain in currentChain.
 func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *VerifyOptions) error {
@@ -708,59 +661,6 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 		}
 	}
 
-	checkEKUs := certType == intermediateCertificate
-
-	// If no extended key usages are specified, then all are acceptable.
-	if checkEKUs && (len(c.ExtKeyUsage) == 0 && len(c.UnknownExtKeyUsage) == 0) {
-		checkEKUs = false
-	}
-
-	// If the “any” key usage is permitted, then no more checks are needed.
-	if checkEKUs {
-		for _, caEKU := range c.ExtKeyUsage {
-			comparisonCount++
-			if caEKU == ExtKeyUsageAny {
-				checkEKUs = false
-				break
-			}
-		}
-	}
-
-	if checkEKUs {
-	NextEKU:
-		for _, eku := range leaf.ExtKeyUsage {
-			if comparisonCount > maxConstraintComparisons {
-				return CertificateInvalidError{c, TooManyConstraints, ""}
-			}
-
-			for _, caEKU := range c.ExtKeyUsage {
-				comparisonCount++
-				if ekuPermittedBy(eku, caEKU, checkingAgainstIssuerCert) {
-					continue NextEKU
-				}
-			}
-
-			oid, _ := oidFromExtKeyUsage(eku)
-			return CertificateInvalidError{c, CANotAuthorizedForExtKeyUsage, fmt.Sprintf("EKU not permitted: %#v", oid)}
-		}
-
-	NextUnknownEKU:
-		for _, eku := range leaf.UnknownExtKeyUsage {
-			if comparisonCount > maxConstraintComparisons {
-				return CertificateInvalidError{c, TooManyConstraints, ""}
-			}
-
-			for _, caEKU := range c.UnknownExtKeyUsage {
-				comparisonCount++
-				if caEKU.Equal(eku) {
-					continue NextUnknownEKU
-				}
-			}
-
-			return CertificateInvalidError{c, CANotAuthorizedForExtKeyUsage, fmt.Sprintf("EKU not permitted: %#v", eku)}
-		}
-	}
-
 	// KeyUsage status flags are ignored. From Engineering Security, Peter
 	// Gutmann: A European government CA marked its signing certificates as
 	// being valid for encryption only, but no-one noticed. Another
@@ -861,63 +761,38 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e
 		}
 	}
 
-	requestedKeyUsages := make([]ExtKeyUsage, len(opts.KeyUsages))
-	copy(requestedKeyUsages, opts.KeyUsages)
-	if len(requestedKeyUsages) == 0 {
-		requestedKeyUsages = append(requestedKeyUsages, ExtKeyUsageServerAuth)
+	var candidateChains [][]*Certificate
+	if opts.Roots.contains(c) {
+		candidateChains = append(candidateChains, []*Certificate{c})
+	} else {
+		if candidateChains, err = c.buildChains(make(map[int][][]*Certificate), []*Certificate{c}, &opts); err != nil {
+			return nil, err
+		}
 	}
 
-	// If no key usages are specified, then any are acceptable.
-	checkEKU := len(c.ExtKeyUsage) > 0
-
-	for _, eku := range requestedKeyUsages {
-		if eku == ExtKeyUsageAny {
-			checkEKU = false
-			break
-		}
+	keyUsages := opts.KeyUsages
+	if len(keyUsages) == 0 {
+		keyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
 	}
 
-	if checkEKU {
-		foundMatch := false
-	NextUsage:
-		for _, eku := range requestedKeyUsages {
-			for _, leafEKU := range c.ExtKeyUsage {
-				if ekuPermittedBy(eku, leafEKU, checkingAgainstLeafCert) {
-					foundMatch = true
-					break NextUsage
-				}
-			}
+	// If any key usage is acceptable then we're done.
+	for _, usage := range keyUsages {
+		if usage == ExtKeyUsageAny {
+			return candidateChains, nil
 		}
+	}
 
-		if !foundMatch {
-			msg := "leaf contains the following, recognized EKUs: "
-
-			for i, leafEKU := range c.ExtKeyUsage {
-				oid, ok := oidFromExtKeyUsage(leafEKU)
-				if !ok {
-					continue
-				}
-
-				if i > 0 {
-					msg += ", "
-				}
-				msg += formatOID(oid)
-			}
-
-			return nil, CertificateInvalidError{c, IncompatibleUsage, msg}
+	for _, candidate := range candidateChains {
+		if checkChainForKeyUsage(candidate, keyUsages) {
+			chains = append(chains, candidate)
 		}
 	}
 
-	var candidateChains [][]*Certificate
-	if opts.Roots.contains(c) {
-		candidateChains = append(candidateChains, []*Certificate{c})
-	} else {
-		if candidateChains, err = c.buildChains(make(map[int][][]*Certificate), []*Certificate{c}, &opts); err != nil {
-			return nil, err
-		}
+	if len(chains) == 0 {
+		return nil, CertificateInvalidError{c, IncompatibleUsage, ""}
 	}
 
-	return candidateChains, nil
+	return chains, nil
 }
 
 func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate {
@@ -1078,3 +953,65 @@ func (c *Certificate) VerifyHostname(h string) error {
 
 	return HostnameError{c, h}
 }
+
+func checkChainForKeyUsage(chain []*Certificate, keyUsages []ExtKeyUsage) bool {
+	usages := make([]ExtKeyUsage, len(keyUsages))
+	copy(usages, keyUsages)
+
+	if len(chain) == 0 {
+		return false
+	}
+
+	usagesRemaining := len(usages)
+
+	// We walk down the list and cross out any usages that aren't supported
+	// by each certificate. If we cross out all the usages, then the chain
+	// is unacceptable.
+
+NextCert:
+	for i := len(chain) - 1; i >= 0; i-- {
+		cert := chain[i]
+		if len(cert.ExtKeyUsage) == 0 && len(cert.UnknownExtKeyUsage) == 0 {
+			// The certificate doesn't have any extended key usage specified.
+			continue
+		}
+
+		for _, usage := range cert.ExtKeyUsage {
+			if usage == ExtKeyUsageAny {
+				// The certificate is explicitly good for any usage.
+				continue NextCert
+			}
+		}
+
+		const invalidUsage ExtKeyUsage = -1
+
+	NextRequestedUsage:
+		for i, requestedUsage := range usages {
+			if requestedUsage == invalidUsage {
+				continue
+			}
+
+			for _, usage := range cert.ExtKeyUsage {
+				if requestedUsage == usage {
+					continue NextRequestedUsage
+				} else if requestedUsage == ExtKeyUsageServerAuth &&
+					(usage == ExtKeyUsageNetscapeServerGatedCrypto ||
+						usage == ExtKeyUsageMicrosoftServerGatedCrypto) {
+					// In order to support COMODO
+					// certificate chains, we have to
+					// accept Netscape or Microsoft SGC
+					// usages as equal to ServerAuth.
+					continue NextRequestedUsage
+				}
+			}
+
+			usages[i] = invalidUsage
+			usagesRemaining--
+			if usagesRemaining == 0 {
+				return false
+			}
+		}
+	}
+
+	return true
+}