Bug 1588941 - Send empty client cert msg when signature scheme selection fails. r=mt

`ssl3_CompleteHandleCertificateRequest` does essentially two things: 1) Calls the `getClientAuthData` hook for certificate selection, and 2) calls `ssl_PickClientSignatureScheme` to select an appropriate signature scheme when a cert is selected.

If the first function returns SECFailure, we default to sending an empty certificate message. If the latter fails, however, this bubbles up as a [[ https://searchfox.org/mozilla-central/rev/56bb74ea8e04bdac57c33cbe9b54d889b9262ade/security/nss/lib/ssl/tls13con.c#2670 | fatal error ]] (and an assertion failure) on the connection. Importantly, the signature scheme selection can fail for reasons that should not be considered fatal - notably when an RSA-PSS cert is selected, but the token on which the key resides does not actually support PSS.

This patch treats the failure to find a usable signature scheme as a "no certificate" response, rather than killing the connection entirely.

Differential Revision: https://phabricator.services.mozilla.com/D85451

2 files changed, 113 insertions, 14 deletions

diff --git a/gtests/ssl_gtest/ssl_auth_unittest.cc b/gtests/ssl_gtest/ssl_auth_unittest.cc
index 6982a6c9b..d8a8221b4 100644
--- a/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -915,6 +915,104 @@ TEST_P(TlsConnectTls12, ClientAuthNoSigAlgs) {
   client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM);
 }
 
+static SECStatus GetEcClientAuthDataHook(void* self, PRFileDesc* fd,
+                                         CERTDistNames* caNames,
+                                         CERTCertificate** clientCert,
+                                         SECKEYPrivateKey** clientKey) {
+  ScopedCERTCertificate cert;
+  ScopedSECKEYPrivateKey priv;
+  // use a different certificate than TlsAgent::kClient
+  if (!TlsAgent::LoadCertificate(TlsAgent::kServerEcdsa256, &cert, &priv)) {
+    return SECFailure;
+  }
+
+  *clientCert = cert.release();
+  *clientKey = priv.release();
+  return SECSuccess;
+}
+
+TEST_P(TlsConnectTls12Plus, ClientAuthDisjointSchemes) {
+  EnsureTlsSetup();
+  client_->SetupClientAuth();
+  server_->RequestClientAuth(true);
+
+  SSLSignatureScheme server_scheme = ssl_sig_rsa_pss_rsae_sha256;
+  std::vector<SSLSignatureScheme> client_schemes{
+      ssl_sig_rsa_pss_rsae_sha256, ssl_sig_ecdsa_secp256r1_sha256};
+  SECStatus rv =
+      SSL_SignatureSchemePrefSet(server_->ssl_fd(), &server_scheme, 1);
+  EXPECT_EQ(SECSuccess, rv);
+  rv = SSL_SignatureSchemePrefSet(
+      client_->ssl_fd(), client_schemes.data(),
+      static_cast<unsigned int>(client_schemes.size()));
+  EXPECT_EQ(SECSuccess, rv);
+
+  // Select an EC cert that's incompatible with server schemes.
+  EXPECT_EQ(SECSuccess,
+            SSL_GetClientAuthDataHook(client_->ssl_fd(),
+                                      GetEcClientAuthDataHook, nullptr));
+
+  StartConnect();
+  client_->Handshake();  // CH
+  server_->Handshake();  // SH
+  client_->Handshake();
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    ASSERT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+    ExpectAlert(server_, kTlsAlertCertificateRequired);
+    server_->Handshake();  // Alert
+    server_->CheckErrorCode(SSL_ERROR_NO_CERTIFICATE);
+    client_->Handshake();  // Receive Alert
+    client_->CheckErrorCode(SSL_ERROR_RX_CERTIFICATE_REQUIRED_ALERT);
+  } else {
+    ASSERT_EQ(TlsAgent::STATE_CONNECTING, client_->state());
+    ExpectAlert(server_, kTlsAlertBadCertificate);
+    server_->Handshake();  // Alert
+    server_->CheckErrorCode(SSL_ERROR_NO_CERTIFICATE);
+    client_->Handshake();  // Receive Alert
+    client_->CheckErrorCode(SSL_ERROR_BAD_CERT_ALERT);
+  }
+}
+
+TEST_F(TlsConnectStreamTls13, PostHandshakeAuthDisjointSchemes) {
+  EnsureTlsSetup();
+  SSLSignatureScheme server_scheme = ssl_sig_rsa_pss_rsae_sha256;
+  std::vector<SSLSignatureScheme> client_schemes{
+      ssl_sig_rsa_pss_rsae_sha256, ssl_sig_ecdsa_secp256r1_sha256};
+  SECStatus rv =
+      SSL_SignatureSchemePrefSet(server_->ssl_fd(), &server_scheme, 1);
+  EXPECT_EQ(SECSuccess, rv);
+  rv = SSL_SignatureSchemePrefSet(
+      client_->ssl_fd(), client_schemes.data(),
+      static_cast<unsigned int>(client_schemes.size()));
+  EXPECT_EQ(SECSuccess, rv);
+
+  client_->SetupClientAuth();
+  client_->SetOption(SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE);
+
+  // Select an EC cert that's incompatible with server schemes.
+  EXPECT_EQ(SECSuccess,
+            SSL_GetClientAuthDataHook(client_->ssl_fd(),
+                                      GetEcClientAuthDataHook, nullptr));
+
+  Connect();
+
+  // Send CertificateRequest.
+  EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()))
+      << "Unexpected error: " << PORT_ErrorToName(PORT_GetError());
+
+  // Need to do a round-trip so that the post-handshake message is
+  // handled on both client and server.
+  server_->SendData(50);
+  client_->ReadBytes(50);
+  client_->SendData(50);
+  server_->ReadBytes(50);
+
+  ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd()));
+  ASSERT_EQ(nullptr, cert1.get());
+  ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd()));
+  ASSERT_EQ(nullptr, cert2.get());
+}
+
 static const SSLSignatureScheme kSignatureSchemeEcdsaSha384[] = {
     ssl_sig_ecdsa_secp384r1_sha384};
 static const SSLSignatureScheme kSignatureSchemeEcdsaSha256[] = {
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index d0cb3f67b..503d14b3b 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -7653,16 +7653,6 @@ ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
             /* check what the callback function returned */
             if ((!ss->ssl3.clientCertificate) || (!ss->ssl3.clientPrivateKey)) {
                 /* we are missing either the key or cert */
-                if (ss->ssl3.clientCertificate) {
-                    /* got a cert, but no key - free it */
-                    CERT_DestroyCertificate(ss->ssl3.clientCertificate);
-                    ss->ssl3.clientCertificate = NULL;
-                }
-                if (ss->ssl3.clientPrivateKey) {
-                    /* got a key, but no cert - free it */
-                    SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
-                    ss->ssl3.clientPrivateKey = NULL;
-                }
                 goto send_no_certificate;
             }
             /* Setting ssl3.clientCertChain non-NULL will cause
@@ -7672,22 +7662,33 @@ ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
                 ss->ssl3.clientCertificate,
                 certUsageSSLClient, PR_FALSE);
             if (ss->ssl3.clientCertChain == NULL) {
-                CERT_DestroyCertificate(ss->ssl3.clientCertificate);
-                ss->ssl3.clientCertificate = NULL;
-                SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
-                ss->ssl3.clientPrivateKey = NULL;
                 goto send_no_certificate;
             }
             if (ss->ssl3.hs.hashType == handshake_hash_record ||
                 ss->ssl3.hs.hashType == handshake_hash_single) {
                 rv = ssl_PickClientSignatureScheme(ss, signatureSchemes,
                                                    signatureSchemeCount);
+                if (rv != SECSuccess) {
+                    /* This should only happen if our schemes changed or
+                     * if an RSA-PSS cert was selected, but the token
+                     * does not support PSS schemes. */
+                    goto send_no_certificate;
+                }
             }
             break; /* not an error */
 
         case SECFailure:
         default:
         send_no_certificate:
+            CERT_DestroyCertificate(ss->ssl3.clientCertificate);
+            SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
+            ss->ssl3.clientCertificate = NULL;
+            ss->ssl3.clientPrivateKey = NULL;
+            if (ss->ssl3.clientCertChain) {
+                CERT_DestroyCertificateList(ss->ssl3.clientCertChain);
+                ss->ssl3.clientCertChain = NULL;
+            }
+
             if (ss->version > SSL_LIBRARY_VERSION_3_0) {
                 ss->ssl3.sendEmptyCert = PR_TRUE;
             } else {