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 <[email protected]> Reviewed-by: Adam Langley <[email protected]> Auto-Submit: David Benjamin <[email protected]>
briansmith · Dec 8, 2022 · a614d46 · a614d46
1 parent 4acc7dd
commit a614d46
Show file tree

Hide file tree

Showing 9 changed files with 45 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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<BUF_MEM> hs_buf;
 

diff --git 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
@@ -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
@@ -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
@@ -15727,32 +15727,34 @@ 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"},
 			})
 		}
 
 		if ver.version >= VersionTLS13 {
 			// 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
@@ -365,6 +365,8 @@ std::vector<Flag> 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
@@ -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;