From d7266ecc9bf92ffad277bc39653919da79c8f42b Mon Sep 17 00:00:00 2001 From: Jesse Selover Date: Wed, 30 Jan 2019 16:06:10 -0500 Subject: [PATCH] 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 Commit-Queue: David Benjamin --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 7 ++ ssl/handshake_client.cc | 21 +++++ ssl/internal.h | 19 +++-- ssl/ssl_cert.cc | 22 +---- ssl/ssl_lib.cc | 8 ++ ssl/test/runner/runner.go | 173 +++++++++++++++++++++++++++++++++++++- ssl/test/test_config.cc | 4 + ssl/test/test_config.h | 1 + ssl/tls13_both.cc | 3 +- 10 files changed, 234 insertions(+), 25 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index f62416cd..ddb383c3 100644 --- a/crypto/err/ssl.errordata +++ b/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 diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 52d713aa..fa0f6b2b 100644 --- a/include/openssl/ssl.h +++ b/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 diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index e2b1ffe9..b0de6708 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -1248,6 +1248,27 @@ static enum ssl_hs_wait_t do_send_client_key_exchange(SSL_HANDSHAKE *hs) { Array 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 158a233c..2c7f606e 100644 --- a/ssl/internal.h +++ b/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 -// 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 @@ 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. diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc index d23e1e63..52a1ddf3 100644 --- a/ssl/ssl_cert.cc +++ b/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)) { - 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 @@ 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); diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index bcf4bd22..a4f20444 100644 --- a/ssl/ssl_lib.cc +++ b/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; diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 34cb1096..8430ae48 100644 --- a/ssl/test/runner/runner.go +++ b/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() diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 2f53156a..70e061b0 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -145,6 +145,7 @@ const Flag 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 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); } diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 8b63bc8e..9221d6f1 100644 --- a/ssl/test/test_config.h +++ b/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; diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index eb1c15ed..ba5719fd 100644 --- a/ssl/tls13_both.cc +++ b/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; }