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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index f62416c..ddb383c 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -82,6 +82,7 @@
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
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 52d713a..fa0f6b2 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3666,6 +3666,12 @@
// 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 @@
#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
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index e2b1ffe..b0de670 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1248,6 +1248,27 @@
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;
diff --git a/ssl/internal.h b/ssl/internal.h
index 158a233..2c7f606 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1189,11 +1189,15 @@
// 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
-// certificate in |in| and returns true if doesn't specify a key usage or, if it
-// does, if it includes digitalSignature. Otherwise it pushes to the error queue
-// and returns false.
-bool ssl_cert_check_digital_signature_key_usage(const CBS *in);
+enum ssl_key_usage_t {
+ key_usage_digital_signature = 0,
+ key_usage_encipherment = 2,
+};
+
+// 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 @@
// 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.
diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc
index d23e1e6..52a1ddf 100644
--- a/ssl/ssl_cert.cc
+++ b/ssl/ssl_cert.cc
@@ -246,7 +246,7 @@
// 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 @@
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 @@
return false;
}
- if (!CBS_asn1_bitstring_has_bit(&bit_string, 0)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_ECC_CERT_NOT_FOR_SIGNING);
+ if (!CBS_asn1_bitstring_has_bit(&bit_string, bit)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_KEY_USAGE_BIT_INCORRECT);
return false;
}
@@ -710,20 +710,6 @@
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);
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index bcf4bd2..a4f2044 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -729,6 +729,7 @@
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 @@
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;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 34cb109..8430ae4 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -14147,11 +14147,181 @@
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 @@
addCertificateTests()
addRetainOnlySHA256ClientCertTests()
addECDSAKeyUsageTests()
+ addRSAKeyUsageTests()
addExtraHandshakeTests()
addOmitExtensionsTests()
addCertCompressionTests()
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 2f53156..70e061b 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -145,6 +145,7 @@
{ "-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 @@
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);
}
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 8b63bc8..9221d6f 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -165,6 +165,7 @@
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;
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc
index eb1c15e..ba5719f 100644
--- a/ssl/tls13_both.cc
+++ b/ssl/tls13_both.cc
@@ -212,7 +212,8 @@
}
// 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;
}
