From a614d46d40509ea2f0c10d005972a08909c32b8c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 2 Dec 2022 15:30:15 -0500 Subject: [PATCH] Add SSL_was_key_usage_invalid. This function reports when security-critical checks on the X.509 key usage extension would have failed, but were skipped due to the temporary exception in SSL_set_enforce_rsa_key_usage. This function is meant to aid deployments as they work through enabling this. Change-Id: Ice0359879c0a6cbe55bf0cb81a63685506883123 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55465 Commit-Queue: Adam Langley Reviewed-by: Adam Langley Auto-Submit: David Benjamin --- include/openssl/ssl.h | 18 +++++++++++++++--- ssl/handshake_client.cc | 8 +++++--- ssl/internal.h | 5 +++++ ssl/s3_lib.cc | 3 ++- ssl/ssl_lib.cc | 4 ++++ ssl/test/bssl_shim.cc | 6 ++++++ ssl/test/runner/runner.go | 8 +++++--- ssl/test/test_config.cc | 2 ++ ssl/test/test_config.h | 1 + 9 files changed, 45 insertions(+), 10 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 6c8eba08a..26e8f910e 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4327,12 +4327,24 @@ 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 +// SSL_set_enforce_rsa_key_usage configures whether, when |ssl| is a client +// negotiating TLS 1.2 or below, the keyUsage extension of RSA leaf server +// certificates will be checked for consistency with the TLS usage. In all other +// cases, this check is always enabled. +// +// 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_was_key_usage_invalid returns one if |ssl|'s handshake succeeded despite +// using TLS parameters which were incompatible with the leaf certificate's +// keyUsage extension. Otherwise, it returns zero. +// +// If |SSL_set_enforce_rsa_key_usage| is enabled or not applicable, this +// function will always return zero because key usages will be consistently +// checked. +OPENSSL_EXPORT int SSL_was_key_usage_invalid(const SSL *ssl); + // 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. diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 0a41ffeb6..b9b3f2782 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -1390,11 +1390,13 @@ static enum ssl_hs_wait_t do_send_client_key_exchange(SSL_HANDSHAKE *hs) { ssl_key_usage_t intended_use = (alg_k & SSL_kRSA) ? key_usage_encipherment : key_usage_digital_signature; - if (hs->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)) { + if (!ssl_cert_check_key_usage(&leaf_cbs, intended_use)) { + if (hs->config->enforce_rsa_key_usage || + EVP_PKEY_id(hs->peer_pubkey.get()) != EVP_PKEY_RSA) { return ssl_hs_error; } + ERR_clear_error(); + ssl->s3->was_key_usage_invalid = true; } } diff --git a/ssl/internal.h b/ssl/internal.h index 4d9ab4910..456fa7a48 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -2763,6 +2763,11 @@ struct SSL3_STATE { // HelloRetryRequest message. bool used_hello_retry_request : 1; + // was_key_usage_invalid is whether the handshake succeeded despite using a + // TLS mode which was incompatible with the leaf certificate's keyUsage + // extension. + bool was_key_usage_invalid : 1; + // hs_buf is the buffer of handshake data to process. UniquePtr hs_buf; diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index 2adf38697..8e8a034eb 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc @@ -178,7 +178,8 @@ SSL3_STATE::SSL3_STATE() early_data_accepted(false), alert_dispatch(false), renegotiate_pending(false), - used_hello_retry_request(false) {} + used_hello_retry_request(false), + was_key_usage_invalid(false) {} SSL3_STATE::~SSL3_STATE() {} diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index cfd1862d4..0f0f5b1c3 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -2807,6 +2807,10 @@ void SSL_set_enforce_rsa_key_usage(SSL *ssl, int enabled) { ssl->config->enforce_rsa_key_usage = !!enabled; } +int SSL_was_key_usage_invalid(const SSL *ssl) { + return ssl->s3->was_key_usage_invalid; +} + void SSL_set_renegotiate_mode(SSL *ssl, enum ssl_renegotiate_mode_t mode) { ssl->renegotiate_mode = mode; diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 3e12a0f48..684c254fa 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -666,6 +666,12 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume, return false; } + if (config->expect_key_usage_invalid != !!SSL_was_key_usage_invalid(ssl)) { + fprintf(stderr, "X.509 key usage was %svalid, but wanted opposite.\n", + SSL_was_key_usage_invalid(ssl) ? "in" : ""); + return false; + } + // Test that handshake hints correctly skipped the expected operations. if (config->handshake_hints && !config->allow_hint_mismatch) { const TestState *state = GetTestState(ssl); diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index b66ae525d..a30dba0c0 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -15727,24 +15727,26 @@ func addRSAKeyUsageTests() { // In 1.2 and below, we should not enforce without the enforce-rsa-key-usage flag. testCases = append(testCases, testCase{ testType: clientTest, - name: "RSAKeyUsage-Client-WantSignature-GotEncipherment-Unenforced" + ver.name, + name: "RSAKeyUsage-Client-WantSignature-GotEncipherment-Unenforced-" + ver.name, config: Config{ MinVersion: ver.version, MaxVersion: ver.version, Certificates: []Certificate{dsCert}, CipherSuites: encSuites, }, + flags: []string{"-expect-key-usage-invalid"}, }) testCases = append(testCases, testCase{ testType: clientTest, - name: "RSAKeyUsage-Client-WantEncipherment-GotSignature-Unenforced" + ver.name, + name: "RSAKeyUsage-Client-WantEncipherment-GotSignature-Unenforced-" + ver.name, config: Config{ MinVersion: ver.version, MaxVersion: ver.version, Certificates: []Certificate{encCert}, CipherSuites: dsSuites, }, + flags: []string{"-expect-key-usage-invalid"}, }) } @@ -15752,7 +15754,7 @@ func addRSAKeyUsageTests() { // In 1.3 and above, we enforce keyUsage even without the flag. testCases = append(testCases, testCase{ testType: clientTest, - name: "RSAKeyUsage-Client-WantSignature-GotEncipherment-Enforced" + ver.name, + name: "RSAKeyUsage-Client-WantSignature-GotEncipherment-Enforced-" + ver.name, config: Config{ MinVersion: ver.version, MaxVersion: ver.version, diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 2671370bb..465c616a6 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -365,6 +365,8 @@ std::vector SortedFlags() { &TestConfig::install_one_cert_compression_alg), BoolFlag("-reverify-on-resume", &TestConfig::reverify_on_resume), BoolFlag("-enforce-rsa-key-usage", &TestConfig::enforce_rsa_key_usage), + BoolFlag("-expect-key-usage-invalid", + &TestConfig::expect_key_usage_invalid), BoolFlag("-is-handshaker-supported", &TestConfig::is_handshaker_supported), BoolFlag("-handshaker-resume", &TestConfig::handshaker_resume), diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 1a21ac147..5608dabd0 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -178,6 +178,7 @@ struct TestConfig { int install_one_cert_compression_alg = 0; bool reverify_on_resume = false; bool enforce_rsa_key_usage = false; + bool expect_key_usage_invalid = false; bool is_handshaker_supported = false; bool handshaker_resume = false; std::string handshaker_path;