[release-branch.go1.13-security] crypto/x509: respect VerifyOptions.K…

…eyUsages on Windows

When using the platform verifier on Windows (because Roots is nil) we
were always enforcing server auth EKUs if DNSName was set, and none
otherwise. If an application was setting KeyUsages, they were not being
respected.

Started correctly surfacing IncompatibleUsage errors from the system
verifier, as those are the ones applications will see if they are
affected by this change.

Also refactored verify_test.go to make it easier to add tests for this,
and replaced the EKULeaf chain with a new one that doesn't have a SHA-1
signature.

Thanks to Niall Newman for reporting this.

Fixes #39360
Fixes CVE-2020-14039

Change-Id: If5c00d615f2944f7d57007891aae1307f9571c32
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/774414
Reviewed-by: Katie Hockman <[email protected]>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/793509
Reviewed-by: Filippo Valsorda <[email protected]>

src/crypto/x509/root_windows.go

@@ -88,6 +88,9 @@ func checkChainTrustStatus(c *Certificate, chainCtx *syscall.CertChainContext) e
 		switch status {
 		case syscall.CERT_TRUST_IS_NOT_TIME_VALID:
 			return CertificateInvalidError{c, Expired, ""}
+		case syscall.CERT_TRUST_IS_NOT_VALID_FOR_USAGE:
+			return CertificateInvalidError{c, IncompatibleUsage, ""}
+		// TODO(filippo): surface more error statuses.
 		default:
 			return UnknownAuthorityError{c, nil, nil}
 		}
@@ -138,11 +141,19 @@ func checkChainSSLServerPolicy(c *Certificate, chainCtx *syscall.CertChainContex
 	return nil
 }
 
+// windowsExtKeyUsageOIDs are the C NUL-terminated string representations of the
+// OIDs for use with the Windows API.
+var windowsExtKeyUsageOIDs = make(map[ExtKeyUsage][]byte, len(extKeyUsageOIDs))
+
+func init() {
+	for _, eku := range extKeyUsageOIDs {
+		windowsExtKeyUsageOIDs[eku.extKeyUsage] = []byte(eku.oid.String() + "\x00")
+	}
+}
+
 // systemVerify is like Verify, except that it uses CryptoAPI calls
 // to build certificate chains and verify them.
 func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) {
-	hasDNSName := opts != nil && len(opts.DNSName) > 0
-
 	storeCtx, err := createStoreContext(c, opts)
 	if err != nil {
 		return nil, err
@@ -152,17 +163,26 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate
 	para := new(syscall.CertChainPara)
 	para.Size = uint32(unsafe.Sizeof(*para))
 
-	// If there's a DNSName set in opts, assume we're verifying
-	// a certificate from a TLS server.
-	if hasDNSName {
-		oids := []*byte{
-			&syscall.OID_PKIX_KP_SERVER_AUTH[0],
-			// Both IE and Chrome allow certificates with
-			// Server Gated Crypto as well. Some certificates
-			// in the wild require them.
-			&syscall.OID_SERVER_GATED_CRYPTO[0],
-			&syscall.OID_SGC_NETSCAPE[0],
+	keyUsages := opts.KeyUsages
+	if len(keyUsages) == 0 {
+		keyUsages = []ExtKeyUsage{ExtKeyUsageServerAuth}
+	}
+	oids := make([]*byte, 0, len(keyUsages))
+	for _, eku := range keyUsages {
+		if eku == ExtKeyUsageAny {
+			oids = nil
+			break
+		}
+		if oid, ok := windowsExtKeyUsageOIDs[eku]; ok {
+			oids = append(oids, &oid[0])
 		}
+		// Like the standard verifier, accept SGC EKUs as equivalent to ServerAuth.
+		if eku == ExtKeyUsageServerAuth {
+			oids = append(oids, &syscall.OID_SERVER_GATED_CRYPTO[0])
+			oids = append(oids, &syscall.OID_SGC_NETSCAPE[0])
+		}
+	}
+	if oids != nil {
 		para.RequestedUsage.Type = syscall.USAGE_MATCH_TYPE_OR
 		para.RequestedUsage.Usage.Length = uint32(len(oids))
 		para.RequestedUsage.Usage.UsageIdentifiers = &oids[0]
@@ -208,7 +228,7 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate
 		return nil, err
 	}
 
-	if hasDNSName {
+	if opts != nil && len(opts.DNSName) > 0 {
 		err = checkChainSSLServerPolicy(c, chainCtx, opts)
 		if err != nil {
 			return nil, err

src/crypto/x509/verify.go

@@ -188,23 +188,32 @@ var errNotParsed = errors.New("x509: missing ASN.1 contents; use ParseCertificat
 // VerifyOptions contains parameters for Certificate.Verify. It's a structure
 // because other PKIX verification APIs have ended up needing many options.
 type VerifyOptions struct {
-	DNSName       string
+	// DNSName, if set, is checked against the leaf certificate with
+	// Certificate.VerifyHostname or the platform verifier.
+	DNSName string
+
+	// Intermediates is an optional pool of certificates that are not trust
+	// anchors, but can be used to form a chain from the leaf certificate to a
+	// root certificate.
 	Intermediates *CertPool
-	Roots         *CertPool // if nil, the system roots are used
-	CurrentTime   time.Time // if zero, the current time is used
-	// KeyUsage specifies which Extended Key Usage values are acceptable. A leaf
-	// certificate is accepted if it contains any of the listed values. An empty
-	// list means ExtKeyUsageServerAuth. To accept any key usage, include
-	// ExtKeyUsageAny.
-	//
-	// Certificate chains are required to nest these extended key usage values.
-	// (This matches the Windows CryptoAPI behavior, but not the spec.)
+	// Roots is the set of trusted root certificates the leaf certificate needs
+	// to chain up to. If nil, the system roots or the platform verifier are used.
+	Roots *CertPool
+
+	// CurrentTime is used to check the validity of all certificates in the
+	// chain. If zero, the current time is used.
+	CurrentTime time.Time
+
+	// KeyUsages specifies which Extended Key Usage values are acceptable. A
+	// chain is accepted if it allows any of the listed values. An empty list
+	// means ExtKeyUsageServerAuth. To accept any key usage, include ExtKeyUsageAny.
 	KeyUsages []ExtKeyUsage
+
 	// MaxConstraintComparisions is the maximum number of comparisons to
 	// perform when checking a given certificate's name constraints. If
 	// zero, a sensible default is used. This limit prevents pathological
 	// certificates from consuming excessive amounts of CPU time when
-	// validating.
+	// validating. It does not apply to the platform verifier.
 	MaxConstraintComparisions int
 }
 
@@ -707,18 +716,21 @@ func (c *Certificate) isValid(certType int, currentChain []*Certificate, opts *V
 // needed. If successful, it returns one or more chains where the first
 // element of the chain is c and the last element is from opts.Roots.
 //
-// If opts.Roots is nil and system roots are unavailable the returned error
-// will be of type SystemRootsError.
+// If opts.Roots is nil, the platform verifier might be used, and
+// verification details might differ from what is described below. If system
+// roots are unavailable the returned error will be of type SystemRootsError.
 //
 // Name constraints in the intermediates will be applied to all names claimed
 // in the chain, not just opts.DNSName. Thus it is invalid for a leaf to claim
 // example.com if an intermediate doesn't permit it, even if example.com is not
 // the name being validated. Note that DirectoryName constraints are not
 // supported.
 //
-// Extended Key Usage values are enforced down a chain, so an intermediate or
-// root that enumerates EKUs prevents a leaf from asserting an EKU not in that
-// list.
+//
+// Extended Key Usage values are enforced nested down a chain, so an intermediate
+// or root that enumerates EKUs prevents a leaf from asserting an EKU not in that
+// list. (While this is not specified, it is common practice in order to limit
+// the types of certificates a CA can issue.)
 //
 // WARNING: this function doesn't do any revocation checking.
 func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err error) {