Check client certificate types in TLS <= 1.2

TLS <= 1.2 servers indicate supported client certificate key types with
a certificate_types field in the CertificateRequest. Historically, we've
just ignored this field, because we've always outsourced certificate
selection to the caller anyway. This meant that, if you configured an
RSA client certificate in response to a server that requested only ECDSA
certificates, we would happily send the certificate and leave it to the
server to decide if it was happy.

Strictly speaking, this was in violation of RFC 5246:

   -  The end-entity certificate provided by the client MUST contain a
      key that is compatible with certificate_types. [...]

Although prior TLS versions didn't say anything useful about this either
way.

Once we move certificate selection into the library, we'll want to start
evaluating supported algorithms ourselves. A natural implementation of
it will, as a side effect, cause us to enforce this match, even when
only a single certificate is configured. Since this is unlikely to have
any real compatibility impact (every TLS server I've seen just hardcodes
this list), let's just try turning it on. On the off chance it does
break someone, I've left a flag, SSL_set_check_client_certificate_type,
for folks to turn this check off. The flag will most likely be
unnecessary, in which case we can retire it after a few months.

If this does cause a problem, we can opt to turn it off for the default
certificate, or only enable it when multiple certificates are
configured, or lean on the sigalgs list (doesn't work for 1.0/1.1), but
these all result in some slightly suboptimal behavior, so I think we
should treat them as contingency plans.

Update-Note: A TLS 1.2 (or below) client, using client certificates,
connecting to a TLS server which doesn't support its certificate type
will now fail the connection slightly earlier, rather than sending the
certificate and waiting for the server to reject it. The connection
should fail either way, but now it will fail earlier with
SSL_R_UNKNOWN_CERTIFICATE_TYPE. If the server was buggy and did not
correctly advertise its own capabilities (very very unlikely), this may
cause a connection to fail despite previously succeeding. We have
included a temporary API, SSL_set_check_client_certificate_type, to
disable this behavior in the unlikely event this has any impact, but
please contact the BoringSSL team if you need it, as it will interfere
with improvements down the line.

This change does not affect servers requesting client certificates, only
clients sending them.

Bug: 249
Change-Id: I159bc444c4ee79fbe5c476d4253b48d58d2538be
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66687
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
(cherry picked from commit 60c2867092af66bbe369f00d8214b6d06fcb376a)

include/openssl/ssl.h

@@ -5034,6 +5034,15 @@ OPENSSL_EXPORT int SSL_used_hello_retry_request(const SSL *ssl);
 // https://bugs.openjdk.java.net/browse/JDK-8213202
 OPENSSL_EXPORT void SSL_set_jdk11_workaround(SSL *ssl, int enable);
 
+// SSL_set_check_client_certificate_type configures whether the client, in
+// TLS 1.2 and below, will check its certificate against the server's requested
+// certificate types.
+//
+// By default, this option is enabled. If disabled, certificate selection within
+// the library may not function correctly. This flag is provided temporarily in
+// case of compatibility issues. It will be removed sometime after June 2024.
+OPENSSL_EXPORT void SSL_set_check_client_certificate_type(SSL *ssl, int enable);
+
 
 // SSL Stat Counters.
 

ssl/handshake_client.cc

@@ -1379,17 +1379,41 @@ static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) {
     }
   }
 
-  if (!ssl_has_certificate(hs)) {
+  if (!ssl_on_certificate_selected(hs)) {
+    return ssl_hs_error;
+  }
+
+  if (ssl_has_certificate(hs)) {
+    if (hs->config->check_client_certificate_type) {
+      // Check the certificate types advertised by the peer.
+      uint8_t cert_type;
+      switch (EVP_PKEY_id(hs->local_pubkey.get())) {
+        case EVP_PKEY_RSA:
+          cert_type = SSL3_CT_RSA_SIGN;
+          break;
+        case EVP_PKEY_EC:
+        case EVP_PKEY_ED25519:
+          cert_type = TLS_CT_ECDSA_SIGN;
+          break;
+        default:
+          OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
+          return ssl_hs_error;
+      }
+      if (std::find(hs->certificate_types.begin(), hs->certificate_types.end(),
+                    cert_type) == hs->certificate_types.end()) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_CERTIFICATE_TYPE);
+        return ssl_hs_error;
+      }
+    }
+  } else {
     // Without a client certificate, the handshake buffer may be released.
     hs->transcript.FreeBuffer();
   }
 
-  if (!ssl_on_certificate_selected(hs) ||
-      !ssl_output_cert_chain(hs)) {
+  if (!ssl_output_cert_chain(hs)) {
     return ssl_hs_error;
   }
 
-
   hs->state = state_send_client_key_exchange;
   return ssl_hs_ok;
 }

ssl/internal.h

@@ -3380,6 +3380,11 @@ struct SSL_CONFIG {
   // alps_use_new_codepoint if set indicates we use new ALPS extension codepoint
   // to negotiate and convey application settings.
   bool alps_use_new_codepoint : 1;
+
+  // check_client_certificate_type indicates whether the client, in TLS 1.2 and
+  // below, will check its certificate against the server's requested
+  // certificate types.
+  bool check_client_certificate_type : 1;
 };
 
 // From RFC 8446, used in determining PSK modes.

ssl/ssl_lib.cc

@@ -727,7 +727,8 @@ SSL_CONFIG::SSL_CONFIG(SSL *ssl_arg)
       permute_extensions(false),
       conf_max_version_use_default(true),
       conf_min_version_use_default(true),
-      alps_use_new_codepoint(false) {
+      alps_use_new_codepoint(false),
+      check_client_certificate_type(true) {
   assert(ssl);
 }
 
@@ -3224,6 +3225,13 @@ void SSL_set_jdk11_workaround(SSL *ssl, int enable) {
   ssl->config->jdk11_workaround = !!enable;
 }
 
+void SSL_set_check_client_certificate_type(SSL *ssl, int enable) {
+  if (!ssl->config) {
+    return;
+  }
+  ssl->config->check_client_certificate_type = !!enable;
+}
+
 void SSL_set_quic_use_legacy_codepoint(SSL *ssl, int use_legacy) {
   if (!ssl->config) {
     return;

ssl/test/runner/runner.go

@@ -2096,6 +2096,27 @@ func addBasicTests() {
 				}),
 			},
 		},
+		{
+			name: "CheckClientCertificateTypes",
+			config: Config{
+				MaxVersion:             VersionTLS12,
+				ClientAuth:             RequestClientCert,
+				ClientCertificateTypes: []byte{CertTypeECDSASign},
+			},
+			shimCertificate: &rsaCertificate,
+			shouldFail:      true,
+			expectedError:   ":UNKNOWN_CERTIFICATE_TYPE:",
+		},
+		{
+			name: "NoCheckClientCertificateTypes",
+			config: Config{
+				MaxVersion:             VersionTLS12,
+				ClientAuth:             RequestClientCert,
+				ClientCertificateTypes: []byte{CertTypeECDSASign},
+			},
+			shimCertificate: &rsaCertificate,
+			flags:           []string{"-no-check-client-certificate-type"},
+		},
 		{
 			name: "UnauthenticatedECDH",
 			config: Config{

ssl/test/test_config.cc

@@ -271,8 +271,7 @@ std::vector<Flag> SortedFlags() {
                            &TestConfig::application_settings),
       OptionalStringFlag("-expect-peer-application-settings",
                          &TestConfig::expect_peer_application_settings),
-      BoolFlag("-alps-use-new-codepoint",
-               &TestConfig::alps_use_new_codepoint),
+      BoolFlag("-alps-use-new-codepoint", &TestConfig::alps_use_new_codepoint),
       Base64Flag("-quic-transport-params", &TestConfig::quic_transport_params),
       Base64Flag("-expect-quic-transport-params",
                  &TestConfig::expect_quic_transport_params),
@@ -429,6 +428,8 @@ std::vector<Flag> SortedFlags() {
       StringFlag("-ssl-fuzz-seed-path-prefix", &TestConfig::ssl_fuzz_seed_path_prefix),
       StringFlag("-tls13-ciphersuites", &TestConfig::tls13_ciphersuites),
       StringPairVectorFlag("-multiple-certs-slot", &TestConfig::multiple_certs_slot),
+      BoolFlag("-no-check-client-certificate-type",
+               &TestConfig::no_check_client_certificate_type),
   };
   std::sort(flags.begin(), flags.end(), [](const Flag &a, const Flag &b) {
     return strcmp(a.name, b.name) < 0;
@@ -1907,6 +1908,9 @@ bssl::UniquePtr<SSL> TestConfig::NewSSL(
   if (enforce_rsa_key_usage) {
     SSL_set_enforce_rsa_key_usage(ssl.get(), 1);
   }
+  if (no_check_client_certificate_type) {
+    SSL_set_check_client_certificate_type(ssl.get(), 0);
+  }
   if (no_tls13) {
     SSL_set_options(ssl.get(), SSL_OP_NO_TLSv1_3);
   }

ssl/test/test_config.h

@@ -220,6 +220,7 @@ struct TestConfig {
   // When |multiple_certs_slot| is defined, the certificates defined are
   // prioritized over certs defined with |cert_file| and |key_file|.
   std::vector<std::pair<std::string, std::string>> multiple_certs_slot;
+  bool no_check_client_certificate_type = false;
 
   std::vector<const char*> handshaker_args;