Enforce key usage for RSA keys in TLS 1.2.

For now, this is off by default and controlled by SSL_set_enforce_rsa_key_usage.
This may be set as late as certificate verification so we may start by enforcing
it for known roots.

Generalizes ssl_cert_check_digital_signature_key_usage to check any part of the
key_usage, and adds a new error KEY_USAGE_BIT_INCORRECT for the generalized
method.

Bug: chromium:795089
Change-Id: Ifa504c321bec3263a4e74f2dc48513e3b895d3ee
Reviewed-on: https://boringssl-review.googlesource.com/c/34604
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/err/ssl.errordata

@@ -82,6 +82,7 @@ SSL,269,INVALID_SCT_LIST
 SSL,295,INVALID_SIGNATURE_ALGORITHM
 SSL,160,INVALID_SSL_SESSION
 SSL,161,INVALID_TICKET_KEYS_LENGTH
+SSL,302,KEY_USAGE_BIT_INCORRECT
 SSL,162,LENGTH_MISMATCH
 SSL,164,MISSING_EXTENSION
 SSL,258,MISSING_KEY_SHARE

include/openssl/ssl.h

@@ -3666,6 +3666,12 @@ OPENSSL_EXPORT void SSL_CTX_set_dos_protection_cb(
 // respected on clients.
 OPENSSL_EXPORT void SSL_CTX_set_reverify_on_resume(SSL_CTX *ctx, int enabled);
 
+// SSL_set_enforce_rsa_key_usage configures whether the keyUsage extension of
+// RSA leaf certificates will be checked for consistency with the TLS
+// usage. This parameter may be set late; it will not be read until after the
+// certificate verification callback.
+OPENSSL_EXPORT void SSL_set_enforce_rsa_key_usage(SSL *ssl, int enabled);
+
 // SSL_ST_* are possible values for |SSL_state|, the bitmasks that make them up,
 // and some historical values for compatibility. Only |SSL_ST_INIT| and
 // |SSL_ST_OK| are ever returned.
@@ -4970,6 +4976,7 @@ BSSL_NAMESPACE_END
 #define SSL_R_WRONG_ENCRYPTION_LEVEL_RECEIVED 299
 #define SSL_R_TOO_MUCH_READ_EARLY_DATA 300
 #define SSL_R_INVALID_DELEGATED_CREDENTIAL 301
+#define SSL_R_KEY_USAGE_BIT_INCORRECT 302
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

ssl/handshake_client.cc

@@ -1248,6 +1248,27 @@ static enum ssl_hs_wait_t do_send_client_key_exchange(SSL_HANDSHAKE *hs) {
   Array<uint8_t> pms;
   uint32_t alg_k = hs->new_cipher->algorithm_mkey;
   uint32_t alg_a = hs->new_cipher->algorithm_auth;
+  if (ssl_cipher_uses_certificate_auth(hs->new_cipher)) {
+    CRYPTO_BUFFER *leaf =
+        sk_CRYPTO_BUFFER_value(hs->new_session->certs.get(), 0);
+    CBS leaf_cbs;
+    CBS_init(&leaf_cbs, CRYPTO_BUFFER_data(leaf), CRYPTO_BUFFER_len(leaf));
+
+    // Check the key usage matches the cipher suite. We do this unconditionally
+    // for non-RSA certificates. In particular, it's needed to distinguish ECDH
+    // certificates, which we do not support, from ECDSA certificates.
+    // Historically, we have not checked RSA key usages, so it is controlled by
+    // a flag for now. See https://crbug.com/795089.
+    ssl_key_usage_t intended_use = (alg_k & SSL_kRSA)
+                                       ? key_usage_encipherment
+                                       : key_usage_digital_signature;
+    if (ssl->config->enforce_rsa_key_usage ||
+        EVP_PKEY_id(hs->peer_pubkey.get()) != EVP_PKEY_RSA) {
+      if (!ssl_cert_check_key_usage(&leaf_cbs, intended_use)) {
+        return ssl_hs_error;
+      }
+    }
+  }
 
   // If using a PSK key exchange, prepare the pre-shared key.
   unsigned psk_len = 0;

ssl/internal.h

@@ -1189,11 +1189,15 @@ bool ssl_parse_cert_chain(uint8_t *out_alert,
 // an empty certificate list. It returns true on success and false on error.
 bool ssl_add_cert_chain(SSL_HANDSHAKE *hs, CBB *cbb);
 
-// ssl_cert_check_digital_signature_key_usage parses the DER-encoded, X.509
+enum ssl_key_usage_t {
-// certificate in |in| and returns true if doesn't specify a key usage or, if it
+  key_usage_digital_signature = 0,
-// does, if it includes digitalSignature. Otherwise it pushes to the error queue
+  key_usage_encipherment = 2,
-// and returns false.
+};
-bool ssl_cert_check_digital_signature_key_usage(const CBS *in);
+
+// ssl_cert_check_key_usage parses the DER-encoded, X.509 certificate in |in|
+// and returns true if doesn't specify a key usage or, if it does, if it
+// includes |bit|. Otherwise it pushes to the error queue and returns false.
+bool ssl_cert_check_key_usage(const CBS *in, enum ssl_key_usage_t bit);
 
 // ssl_cert_parse_pubkey extracts the public key from the DER-encoded, X.509
 // certificate in |in|. It returns an allocated |EVP_PKEY| or else returns
@@ -2543,6 +2547,11 @@ struct SSL_CONFIG {
   // advertise support.
   bool channel_id_enabled : 1;
 
+  // If enforce_rsa_key_usage is true, the handshake will fail if the
+  // keyUsage extension is present and incompatible with the TLS usage.
+  // This field is not read until after certificate verification.
+  bool enforce_rsa_key_usage : 1;
+
   // retain_only_sha256_of_client_certs is true if we should compute the SHA256
   // hash of the peer's certificate and then discard it to save memory and
   // session space. Only effective on the server side.

ssl/ssl_cert.cc

@@ -246,7 +246,7 @@ static enum leaf_cert_and_privkey_result_t check_leaf_cert_and_privkey(
   // An ECC certificate may be usable for ECDH or ECDSA. We only support ECDSA
   // certificates, so sanity-check the key usage extension.
   if (pubkey->type == EVP_PKEY_EC &&
-      !ssl_cert_check_digital_signature_key_usage(&cert_cbs)) {
+      !ssl_cert_check_key_usage(&cert_cbs, key_usage_digital_signature)) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
     return leaf_cert_and_privkey_error;
   }
@@ -540,7 +540,7 @@ bool ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey) {
   return ssl_compare_public_and_private_key(pubkey.get(), privkey);
 }
 
-bool ssl_cert_check_digital_signature_key_usage(const CBS *in) {
+bool ssl_cert_check_key_usage(const CBS *in, enum ssl_key_usage_t bit) {
   CBS buf = *in;
 
   CBS tbs_cert, outer_extensions;
@@ -606,8 +606,8 @@ bool ssl_cert_check_digital_signature_key_usage(const CBS *in) {
       return false;
     }
 
-    if (!CBS_asn1_bitstring_has_bit(&bit_string, 0)) {
+    if (!CBS_asn1_bitstring_has_bit(&bit_string, bit)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_ECC_CERT_NOT_FOR_SIGNING);
+      OPENSSL_PUT_ERROR(SSL, SSL_R_KEY_USAGE_BIT_INCORRECT);
       return false;
     }
 
@@ -710,20 +710,6 @@ bool ssl_check_leaf_certificate(SSL_HANDSHAKE *hs, EVP_PKEY *pkey,
     return false;
   }
 
-  // Check key usages for all key types but RSA. This is needed to distinguish
-  // ECDH certificates, which we do not support, from ECDSA certificates. In
-  // principle, we should check RSA key usages based on cipher, but this breaks
-  // buggy antivirus deployments. Other key types are always used for signing.
-  //
-  // TODO(davidben): Get more recent data on RSA key usages.
-  if (EVP_PKEY_id(pkey) != EVP_PKEY_RSA) {
-    CBS leaf_cbs;
-    CBS_init(&leaf_cbs, CRYPTO_BUFFER_data(leaf), CRYPTO_BUFFER_len(leaf));
-    if (!ssl_cert_check_digital_signature_key_usage(&leaf_cbs)) {
-      return false;
-    }
-  }
-
   if (EVP_PKEY_id(pkey) == EVP_PKEY_EC) {
     // Check the key's group and point format are acceptable.
     EC_KEY *ec_key = EVP_PKEY_get0_EC_KEY(pkey);

ssl/ssl_lib.cc

@@ -729,6 +729,7 @@ SSL_CONFIG::SSL_CONFIG(SSL *ssl_arg)
       signed_cert_timestamps_enabled(false),
       ocsp_stapling_enabled(false),
       channel_id_enabled(false),
+      enforce_rsa_key_usage(false),
       retain_only_sha256_of_client_certs(false),
       handoff(false),
       shed_handshake_config(false),
@@ -2697,6 +2698,13 @@ void SSL_CTX_set_reverify_on_resume(SSL_CTX *ctx, int enabled) {
   ctx->reverify_on_resume = !!enabled;
 }
 
+void SSL_set_enforce_rsa_key_usage(SSL *ssl, int enabled) {
+  if (!ssl->config) {
+    return;
+  }
+  ssl->config->enforce_rsa_key_usage = !!enabled;
+}
+
 void SSL_set_renegotiate_mode(SSL *ssl, enum ssl_renegotiate_mode_t mode) {
   ssl->renegotiate_mode = mode;
 

ssl/test/runner/runner.go

@@ -14147,11 +14147,181 @@ func addECDSAKeyUsageTests() {
 				Certificates: []Certificate{cert},
 			},
 			shouldFail:    true,
-			expectedError: ":ECC_CERT_NOT_FOR_SIGNING:",
+			expectedError: ":KEY_USAGE_BIT_INCORRECT:",
 		})
 	}
 }
 
+func addRSAKeyUsageTests() {
+	priv, err := rsa.GenerateKey(rand.Reader, 2048)
+
+	serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128)
+	serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
+	if err != nil {
+		panic(err)
+	}
+
+	dsTemplate := x509.Certificate{
+		SerialNumber: serialNumber,
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotBefore: time.Now(),
+		NotAfter:  time.Now(),
+
+		KeyUsage:              x509.KeyUsageDigitalSignature,
+		BasicConstraintsValid: true,
+	}
+
+	encTemplate := x509.Certificate{
+		SerialNumber: serialNumber,
+		Subject: pkix.Name{
+			Organization: []string{"Acme Co"},
+		},
+		NotBefore: time.Now(),
+		NotAfter:  time.Now(),
+
+		KeyUsage:              x509.KeyUsageKeyEncipherment,
+		BasicConstraintsValid: true,
+	}
+
+	dsDerBytes, err := x509.CreateCertificate(rand.Reader, &dsTemplate, &dsTemplate, &priv.PublicKey, priv)
+	if err != nil {
+		panic(err)
+	}
+
+	encDerBytes, err := x509.CreateCertificate(rand.Reader, &encTemplate, &encTemplate, &priv.PublicKey, priv)
+	if err != nil {
+		panic(err)
+	}
+
+	dsCert := Certificate{
+		Certificate: [][]byte{dsDerBytes},
+		PrivateKey:  priv,
+	}
+
+	encCert := Certificate{
+		Certificate: [][]byte{encDerBytes},
+		PrivateKey:  priv,
+	}
+
+	dsSuites := []uint16{
+		TLS_AES_128_GCM_SHA256,
+		TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+		TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+	}
+	encSuites := []uint16{
+		TLS_RSA_WITH_AES_128_GCM_SHA256,
+		TLS_RSA_WITH_AES_128_CBC_SHA,
+	}
+
+	for _, ver := range tlsVersions {
+		testCases = append(testCases, testCase{
+			testType: clientTest,
+			name:     "RSAKeyUsage-WantSignature-GotEncipherment-" + ver.name,
+			config: Config{
+				MinVersion:   ver.version,
+				MaxVersion:   ver.version,
+				Certificates: []Certificate{encCert},
+				CipherSuites: dsSuites,
+			},
+			shouldFail:    true,
+			expectedError: ":KEY_USAGE_BIT_INCORRECT:",
+			flags: []string{
+				"-enforce-rsa-key-usage",
+			},
+		})
+
+		testCases = append(testCases, testCase{
+			testType: clientTest,
+			name:     "RSAKeyUsage-WantSignature-GotSignature-" + ver.name,
+			config: Config{
+				MinVersion:   ver.version,
+				MaxVersion:   ver.version,
+				Certificates: []Certificate{dsCert},
+				CipherSuites: dsSuites,
+			},
+			flags: []string{
+				"-enforce-rsa-key-usage",
+			},
+		})
+
+		// TLS 1.3 removes the encipherment suites.
+		if ver.version < VersionTLS13 {
+			testCases = append(testCases, testCase{
+				testType: clientTest,
+				name:     "RSAKeyUsage-WantEncipherment-GotEncipherment" + ver.name,
+				config: Config{
+					MinVersion:   ver.version,
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{encCert},
+					CipherSuites: encSuites,
+				},
+				flags: []string{
+					"-enforce-rsa-key-usage",
+				},
+			})
+
+			testCases = append(testCases, testCase{
+				testType: clientTest,
+				name:     "RSAKeyUsage-WantEncipherment-GotSignature-" + ver.name,
+				config: Config{
+					MinVersion:   ver.version,
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{dsCert},
+					CipherSuites: encSuites,
+				},
+				shouldFail:    true,
+				expectedError: ":KEY_USAGE_BIT_INCORRECT:",
+				flags: []string{
+					"-enforce-rsa-key-usage",
+				},
+			})
+
+			// In 1.2 and below, we should not enforce without the enforce-rsa-key-usage flag.
+			testCases = append(testCases, testCase{
+				testType: clientTest,
+				name:     "RSAKeyUsage-WantSignature-GotEncipherment-Unenforced" + ver.name,
+				config: Config{
+					MinVersion:   ver.version,
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{dsCert},
+					CipherSuites: encSuites,
+				},
+			})
+
+			testCases = append(testCases, testCase{
+				testType: clientTest,
+				name:     "RSAKeyUsage-WantEncipherment-GotSignature-Unenforced" + ver.name,
+				config: Config{
+					MinVersion:   ver.version,
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{encCert},
+					CipherSuites: dsSuites,
+				},
+			})
+
+		}
+
+		if ver.version >= VersionTLS13 {
+			// In 1.3 and above, we enforce keyUsage even without the flag.
+			testCases = append(testCases, testCase{
+				testType: clientTest,
+				name:     "RSAKeyUsage-WantSignature-GotEncipherment-Enforced" + ver.name,
+				config: Config{
+					MinVersion:   ver.version,
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{encCert},
+					CipherSuites: dsSuites,
+				},
+				shouldFail:    true,
+				expectedError: ":KEY_USAGE_BIT_INCORRECT:",
+			})
+
+		}
+	}
+}
+
 func addExtraHandshakeTests() {
 	// An extra SSL_do_handshake is normally a no-op. These tests use -async
 	// to ensure there is no transport I/O.
@@ -14995,6 +15165,7 @@ func main() {
 	addCertificateTests()
 	addRetainOnlySHA256ClientCertTests()
 	addECDSAKeyUsageTests()
+	addRSAKeyUsageTests()
 	addExtraHandshakeTests()
 	addOmitExtensionsTests()
 	addCertCompressionTests()

ssl/test/test_config.cc

@@ -145,6 +145,7 @@ const Flag<bool> kBoolFlags[] = {
   { "-is-handshaker-supported", &TestConfig::is_handshaker_supported },
   { "-handshaker-resume", &TestConfig::handshaker_resume },
   { "-reverify-on-resume", &TestConfig::reverify_on_resume },
+  { "-enforce-rsa-key-usage", &TestConfig::enforce_rsa_key_usage },
   { "-jdk11-workaround", &TestConfig::jdk11_workaround },
   { "-server-preference", &TestConfig::server_preference },
   { "-export-traffic-secrets", &TestConfig::export_traffic_secrets },
@@ -1501,6 +1502,9 @@ bssl::UniquePtr<SSL> TestConfig::NewSSL(
   if (reverify_on_resume) {
     SSL_CTX_set_reverify_on_resume(ssl_ctx, 1);
   }
+  if (enforce_rsa_key_usage) {
+    SSL_set_enforce_rsa_key_usage(ssl.get(), 1);
+  }
   if (no_tls13) {
     SSL_set_options(ssl.get(), SSL_OP_NO_TLSv1_3);
   }

ssl/test/test_config.h

@@ -165,6 +165,7 @@ struct TestConfig {
   bool fail_ocsp_callback = false;
   bool install_cert_compression_algs = false;
   bool reverify_on_resume = false;
+  bool enforce_rsa_key_usage = false;
   bool is_handshaker_supported = false;
   bool handshaker_resume = false;
   std::string handshaker_path;

ssl/tls13_both.cc

@@ -212,7 +212,8 @@ bool tls13_process_certificate(SSL_HANDSHAKE *hs, const SSLMessage &msg,
       }
       // TLS 1.3 always uses certificate keys for signing thus the correct
       // keyUsage is enforced.
-      if (!ssl_cert_check_digital_signature_key_usage(&certificate)) {
+      if (!ssl_cert_check_key_usage(&certificate,
+                                    key_usage_digital_signature)) {
         ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
         return false;
       }