Bug 1617956 - Add support for asynchronous client auth hooks. r=mt

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

9 files changed, 309 insertions, 127 deletions

diff --git a/lib/ssl/authcert.c b/lib/ssl/authcert.c
index 54b6f5950..073103d5a 100644
--- a/lib/ssl/authcert.c
+++ b/lib/ssl/authcert.c
@@ -82,7 +82,7 @@ ssl_CertIsUsable(sslSocket *ss, CERTCertificate *cert)
      *   if (!ss->ssl3.hs.hashType == handshake_hash_record  &&
      *           ss->ssl3.hs.hashType == handshake_hash_single) {
      *   return PR_TRUE;
-     * 2) assume if ss->peerSignatureSchemesCount == 0 we are using the
+     * 2) assume if ss->ss->ssl3.hs.clientAuthSignatureSchemesLen == 0 we are using the
      *    old handshake.
      * There is one case where using 2 will be wrong: we somehow call this
      * function outside the case where of out GetClientAuthData context.
@@ -90,15 +90,15 @@ ssl_CertIsUsable(sslSocket *ss, CERTCertificate *cert)
      * best we can do is either always assume good or always assume bad.
      * I think the best results is to always assume good, so we use
      * option 2 here to handle that case as well.*/
-    if (ss->peerSignatureSchemeCount == 0) {
+    if (ss->ssl3.hs.clientAuthSignatureSchemesLen == 0) {
         return PR_TRUE;
     }
-    if (ss->peerSignatureSchemes == NULL) {
+    if (ss->ssl3.hs.clientAuthSignatureSchemes == NULL) {
         return PR_FALSE; /* should this really be an assert? */
     }
     rv = ssl_PickClientSignatureScheme(ss, cert, NULL,
-                                       ss->peerSignatureSchemes,
-                                       ss->peerSignatureSchemeCount,
+                                       ss->ssl3.hs.clientAuthSignatureSchemes,
+                                       ss->ssl3.hs.clientAuthSignatureSchemesLen,
                                        &scheme);
     if (rv != SECSuccess) {
         return PR_FALSE;
diff --git a/lib/ssl/dtlscon.c b/lib/ssl/dtlscon.c
index 10e550e0f..a4a7c998c 100644
--- a/lib/ssl/dtlscon.c
+++ b/lib/ssl/dtlscon.c
@@ -359,7 +359,7 @@ dtls_HandleHandshake(sslSocket *ss, DTLSEpoch epoch, sslSequenceNumber seqNum,
 
             rv = dtls_HandleHandshakeMessage(ss, buf.buf,
                                              buf.len == fragment_length);
-            if (rv == SECFailure) {
+            if (rv != SECSuccess) {
                 goto loser;
             }
         } else {
@@ -468,7 +468,7 @@ dtls_HandleHandshake(sslSocket *ss, DTLSEpoch epoch, sslSequenceNumber seqNum,
                     rv = dtls_HandleHandshakeMessage(ss, ss->ssl3.hs.msg_body.buf,
                                                      buf.len == fragment_length);
 
-                    if (rv == SECFailure) {
+                    if (rv != SECSuccess) {
                         goto loser;
                     }
                 }
diff --git a/lib/ssl/ssl.def b/lib/ssl/ssl.def
index 14fc2b960..75c37f2be 100644
--- a/lib/ssl/ssl.def
+++ b/lib/ssl/ssl.def
@@ -247,3 +247,9 @@ SSL_FilterClientCertListBySocket;
 ;+    local:
 ;+*;
 ;+};
+;+NSS_3.80 {    # NSS 3.80 release
+;+    global:
+SSL_ClientCertCallbackComplete;
+;+    local:
+;+*;
+;+};
diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h
index 90d44f11f..8b6aab20b 100644
--- a/lib/ssl/ssl.h
+++ b/lib/ssl/ssl.h
@@ -856,6 +856,20 @@ SSL_IMPORT SECStatus SSL_AuthCertificate(void *arg, PRFileDesc *fd,
  *  caNames - pointer to distinguished names of CAs that the server likes
  *  pRetCert - pointer to pointer to cert, for return of cert
  *  pRetKey - pointer to key pointer, for return of key
+ *  Return value can be one of {SECSuccess, SECFailure, SECWouldBlock}
+ *
+ *  If SECSuccess, pRetCert and pRetKey should be set to the selected
+ *  client cert and private key respectively. If SECFailure or SECWouldBlock
+ *  they should not be changed.
+ *
+ * Ownership of pRetCert and pRetKey passes to NSS. The application must not
+ * mutate or free the structures after passing them to NSS.
+ *
+ *  Returning SECWouldBlock will block the handshake until SSL_ClientCertCallbackComplete
+ *  is called. Note that references to *caNames should not be kept after SSLGetClientAuthData
+ *  returns. Instead, take a copy of the data.
+ *
+ * See also the comments for SSL_ClientCertCallbackComplete.
  */
 typedef SECStatus(PR_CALLBACK *SSLGetClientAuthData)(void *arg,
                                                      PRFileDesc *fd,
@@ -1505,6 +1519,50 @@ extern const char *NSSSSL_GetVersion(void);
 SSL_IMPORT SECStatus SSL_AuthCertificateComplete(PRFileDesc *fd,
                                                  PRErrorCode error);
 
+/* Restart an SSL connection which was paused to do asynchronous client
+ * certificate selection (when the client certificate hook returned SECWouldBlock).
+ *
+ * This function only works for non-blocking sockets; Do not use it for
+ * blocking sockets. This function works only for the client role of
+ * a connection; it does not work for the server role.
+ *
+ * If a certificate has been sucessfully selected, the application must call
+ * SSL_ClientCertCallbackComplete with:
+ *  - SECSuccess (0) as the value of outcome
+ *  - a valid SECKEYPrivateKey located at *clientPrivateKey
+ *  - a valid CERTCertificate located at *clientCertificate
+ * The ownership of these latter structures will pass to NSS and the application
+ * MUST not retain any references to them or invalidate them. 
+ *
+ * If a certificate has not been selected, the application must call
+ * SSL_ClientCertCallbackComplete with:
+ *  - SECFailure (-1) as the value of outcome
+ *  - *clientPrivateKey set to NULL.
+ *  - *clientCertificate set to NULL
+ *
+ * Once the application has returned SECWouldBlock to getClientAuthData
+ * the handshake will not proceed until this function is called. It is an
+ * error to call this function when the handshake is not waiting on client
+ * certificate selection, or to call this function more than once.
+
+ * This function will not complete the entire handshake. The application must
+ * call SSL_ForceHandshake, PR_Recv, PR_Send, etc. after calling this function
+ * to force the handshake to complete.
+ *
+ * Be careful about converting an application from synchronous cert selection
+ * to asynchronous certificate selection. A naive conversion is likely to
+ * result in deadlocks; e.g. the application will wait in PR_Poll for network
+ * I/O on the connection while all network I/O on the connection is blocked
+ * waiting for this function to be called.
+ *
+ * Note that SSL_ClientCertCallbackComplete will (usually) return
+ * SECSuccess; SECFailure indicates that the function was invoked incorrectly or
+ * an error whilst processing the handshake. The return code does not indicate
+ * whether or not the provided private key and certificate were sucessfully loaded
+ * or accepted by the server.
+ */
+SSL_IMPORT SECStatus SSL_ClientCertCallbackComplete(PRFileDesc *fd, SECStatus outcome, SECKEYPrivateKey *clientPrivateKey, CERTCertificate *clientCertificate);
+
 /*
  * This is used to access experimental APIs.  Don't call this directly.  This is
  * used to enable the experimental APIs that are defined in "sslexp.h".
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index de5eb3e97..aa419bc37 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6954,6 +6954,12 @@ ssl3_HandleServerHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
         SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
         ss->ssl3.clientPrivateKey = NULL;
     }
+    // TODO(djackson) - Bob removed this. Why?
+    if (ss->ssl3.hs.clientAuthSignatureSchemes != NULL) {
+        PR_Free(ss->ssl3.hs.clientAuthSignatureSchemes);
+        ss->ssl3.hs.clientAuthSignatureSchemes = NULL;
+        ss->ssl3.hs.clientAuthSignatureSchemesLen = 0;
+    }
 
     /* Note that if the server selects TLS 1.3, this will set the version to TLS
      * 1.2.  We will amend that once all other fields have been read. */
@@ -7821,9 +7827,9 @@ ssl3_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
 
     ss->ssl3.hs.ws = wait_hello_done;
 
-    rv = ssl3_CompleteHandleCertificateRequest(ss, signatureSchemes,
-                                               signatureSchemeCount, &ca_list);
-    if (rv == SECFailure) {
+    rv = ssl3_BeginHandleCertificateRequest(ss, signatureSchemes,
+                                            signatureSchemeCount, &ca_list);
+    if (rv != SECSuccess) {
         PORT_Assert(0);
         errCode = SEC_ERROR_LIBRARY_FAILURE;
         desc = internal_error;
@@ -7849,51 +7855,11 @@ done:
     return rv;
 }
 
-SECStatus
-ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
-                                      const SSLSignatureScheme *signatureSchemes,
-                                      unsigned int signatureSchemeCount,
-                                      CERTDistNames *ca_list)
+static void
+ssl3_ClientAuthCallbackOutcome(sslSocket *ss, SECStatus outcome)
 {
     SECStatus rv;
-
-    /* Should not send a client cert when (non-GREASE) ECH is rejected. */
-    if (ss->ssl3.hs.echHpkeCtx && !ss->ssl3.hs.echAccepted) {
-        PORT_Assert(ssl3_ExtensionAdvertised(ss, ssl_tls13_encrypted_client_hello_xtn));
-        goto send_no_certificate;
-    }
-
-    if (ss->getClientAuthData != NULL) {
-        PORT_Assert((ss->ssl3.hs.preliminaryInfo & ssl_preinfo_all) ==
-                    ssl_preinfo_all);
-        PORT_Assert(ss->ssl3.clientPrivateKey == NULL);
-        PORT_Assert(ss->ssl3.clientCertificate == NULL);
-        PORT_Assert(ss->ssl3.clientCertChain == NULL);
-        /*
-         * Peer signatures are only available while in the context of
-         * of a getClientAuthData callback. It is required for proper
-         * functioning of SSL_CertIsUsable and SSL_FilterClientCertListBySocket
-         * Calling these functions outside the context of a getClientAuthData
-         * callback will result in no filtering.*/
-        ss->peerSignatureSchemes = signatureSchemes;
-        ss->peerSignatureSchemeCount = signatureSchemeCount;
-        /* XXX Should pass cert_types and algorithms in this call!! */
-        rv = (SECStatus)(*ss->getClientAuthData)(ss->getClientAuthDataArg,
-                                                 ss->fd, ca_list,
-                                                 &ss->ssl3.clientCertificate,
-                                                 &ss->ssl3.clientPrivateKey);
-        /* memory for the signature schemes will go away after the request,
-         * so don't leave dangling pointers around */
-        ss->peerSignatureSchemes = NULL;
-        ss->peerSignatureSchemeCount = 0;
-    } else {
-        rv = SECFailure; /* force it to send a no_certificate alert */
-    }
-    switch (rv) {
-        case SECWouldBlock: /* getClientAuthData has put up a dialog box. */
-            ssl3_SetAlwaysBlock(ss);
-            break; /* not an error */
-
+    switch (outcome) {
         case SECSuccess:
             /* check what the callback function returned */
             if ((!ss->ssl3.clientCertificate) || (!ss->ssl3.clientPrivateKey)) {
@@ -7914,17 +7880,18 @@ ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
                 rv = ssl_PickClientSignatureScheme(ss,
                                                    ss->ssl3.clientCertificate,
                                                    ss->ssl3.clientPrivateKey,
-                                                   signatureSchemes,
-                                                   signatureSchemeCount,
+                                                   ss->ssl3.hs.clientAuthSignatureSchemes,
+                                                   ss->ssl3.hs.clientAuthSignatureSchemesLen,
                                                    &ss->ssl3.hs.signatureScheme);
                 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. */
+                     * does not support PSS schemes.
+                     */
                     goto send_no_certificate;
                 }
             }
-            break; /* not an error */
+            break;
 
         case SECFailure:
         default:
@@ -7943,13 +7910,100 @@ ssl3_CompleteHandleCertificateRequest(sslSocket *ss,
             } else {
                 (void)SSL3_SendAlert(ss, alert_warning, no_certificate);
             }
-            rv = SECSuccess;
             break;
     }
 
+    /* Release the cached parameters */
+    PORT_Free(ss->ssl3.hs.clientAuthSignatureSchemes);
+    ss->ssl3.hs.clientAuthSignatureSchemes = NULL;
+    ss->ssl3.hs.clientAuthSignatureSchemesLen = 0;
+}
+
+SECStatus
+ssl3_BeginHandleCertificateRequest(sslSocket *ss,
+                                   const SSLSignatureScheme *signatureSchemes,
+                                   unsigned int signatureSchemeCount,
+                                   CERTDistNames *ca_list)
+{
+    SECStatus rv;
+
+    PR_ASSERT(!ss->ssl3.hs.clientCertificatePending);
+
+    /* Should not send a client cert when (non-GREASE) ECH is rejected. */
+    if (ss->ssl3.hs.echHpkeCtx && !ss->ssl3.hs.echAccepted) {
+        PORT_Assert(ssl3_ExtensionAdvertised(ss, ssl_tls13_encrypted_client_hello_xtn));
+        rv = SECFailure;
+    } else if (ss->getClientAuthData != NULL) {
+        PORT_Assert((ss->ssl3.hs.preliminaryInfo & ssl_preinfo_all) ==
+                    ssl_preinfo_all);
+        PORT_Assert(ss->ssl3.clientPrivateKey == NULL);
+        PORT_Assert(ss->ssl3.clientCertificate == NULL);
+        PORT_Assert(ss->ssl3.clientCertChain == NULL);
+
+        /* Previously cached parameters should be empty */
+        PORT_Assert(ss->ssl3.hs.clientAuthSignatureSchemes == NULL);
+        PORT_Assert(ss->ssl3.hs.clientAuthSignatureSchemesLen == 0);
+        /*
+         * Peer signatures are only available while in the context of
+         * of a getClientAuthData callback. It is required for proper
+         * functioning of SSL_CertIsUsable and SSL_FilterClientCertListBySocket
+         * Calling these functions outside the context of a getClientAuthData
+         * callback will result in no filtering.*/
+
+        ss->ssl3.hs.clientAuthSignatureSchemes = PORT_ZNewArray(SSLSignatureScheme, signatureSchemeCount);
+        PORT_Memcpy(ss->ssl3.hs.clientAuthSignatureSchemes, signatureSchemes, signatureSchemeCount * sizeof(SSLSignatureScheme));
+        ss->ssl3.hs.clientAuthSignatureSchemesLen = signatureSchemeCount;
+
+        rv = (SECStatus)(*ss->getClientAuthData)(ss->getClientAuthDataArg,
+                                                 ss->fd, ca_list,
+                                                 &ss->ssl3.clientCertificate,
+                                                 &ss->ssl3.clientPrivateKey);
+    } else {
+        rv = SECFailure; /* force it to send a no_certificate alert */
+    }
+
+    if (rv == SECWouldBlock) {
+        /* getClientAuthData needs more time (e.g. for user interaction) */
+
+        /* The out parameters should not have changed. */
+        PORT_Assert(ss->ssl3.clientCertificate == NULL);
+        PORT_Assert(ss->ssl3.clientPrivateKey == NULL);
+
+        /* Mark the handshake as blocked */
+        ss->ssl3.hs.clientCertificatePending = PR_TRUE;
+
+        rv = SECSuccess;
+    } else {
+        /* getClientAuthData returned SECSuccess or SECFailure immediately, handle accordingly */
+        ssl3_ClientAuthCallbackOutcome(ss, rv);
+        rv = SECSuccess;
+    }
     return rv;
 }
 
+/* Invoked by the application when client certificate selection is complete */
+SECStatus
+ssl3_ClientCertCallbackComplete(sslSocket *ss, SECStatus outcome, SECKEYPrivateKey *clientPrivateKey, CERTCertificate *clientCertificate)
+{
+    PORT_Assert(ss->ssl3.hs.clientCertificatePending);
+    ss->ssl3.hs.clientCertificatePending = PR_FALSE;
+
+    ss->ssl3.clientCertificate = clientCertificate;
+    ss->ssl3.clientPrivateKey = clientPrivateKey;
+
+    ssl3_ClientAuthCallbackOutcome(ss, outcome);
+
+    /* Continue the handshake */
+    PORT_Assert(ss->ssl3.hs.restartTarget);
+    if (!ss->ssl3.hs.restartTarget) {
+        FATAL_ERROR(ss, PR_INVALID_STATE_ERROR, internal_error);
+        return SECFailure;
+    }
+    sslRestartTarget target = ss->ssl3.hs.restartTarget;
+    ss->ssl3.hs.restartTarget = NULL;
+    return target(ss);
+}
+
 static SECStatus
 ssl3_CheckFalseStart(sslSocket *ss)
 {
@@ -8105,8 +8159,11 @@ ssl3_SendClientSecondRound(sslSocket *ss)
         PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
         return SECFailure;
     }
-    if (ss->ssl3.hs.authCertificatePending &&
-        (sendClientCert || ss->ssl3.sendEmptyCert || ss->firstHsDone)) {
+    /* Check whether waiting for client certificate selection OR
+       waiting on server certificate verification AND
+       going to send client cert */
+    if ((ss->ssl3.hs.clientCertificatePending) ||
+        (ss->ssl3.hs.authCertificatePending && (sendClientCert || ss->ssl3.sendEmptyCert || ss->firstHsDone))) {
         SSL_TRC(3, ("%d: SSL3[%p]: deferring ssl3_SendClientSecondRound because"
                     " certificate authentication is still pending.",
                     SSL_GETPID(), ss->fd));
@@ -10992,6 +11049,7 @@ ssl3_SendCertificate(sslSocket *ss)
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
+    PR_ASSERT(!ss->ssl3.hs.clientCertificatePending);
 
     if (ss->sec.localCert)
         CERT_DestroyCertificate(ss->sec.localCert);
@@ -11902,6 +11960,7 @@ ssl3_SendFinished(sslSocket *ss, PRInt32 flags)
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
+    PR_ASSERT(!ss->ssl3.hs.clientCertificatePending);
 
     ssl_GetSpecReadLock(ss);
     cwSpec = ss->ssl3.cwSpec;
@@ -12490,8 +12549,11 @@ ssl3_HandleHandshakeMessage(sslSocket *ss, PRUint8 *b, PRUint32 length,
         PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HANDSHAKE);
         return SECFailure;
     }
-
-    if (IS_DTLS(ss) && (rv != SECFailure)) {
+    /* We consider the record to have been handled if SECSuccess or else WOULD_BLOCK is set
+     * Whoever set WOULD_BLOCK must handle any remaining actions required to finsih processing the record.
+     * e.g. by setting restartTarget.
+     */
+    if (IS_DTLS(ss) && (rv == SECSuccess || (rv == SECFailure && PR_GetError() == PR_WOULD_BLOCK_ERROR))) {
         /* Increment the expected sequence number */
         ss->ssl3.hs.recvMessageSeq++;
     }
@@ -13604,6 +13666,9 @@ ssl3_InitState(sslSocket *ss)
     ss->ssl3.hs.echAccepted = PR_FALSE;
     ss->ssl3.hs.echDecided = PR_FALSE;
 
+    ss->ssl3.hs.clientAuthSignatureSchemes = NULL;
+    ss->ssl3.hs.clientAuthSignatureSchemesLen = 0;
+
     PORT_Assert(!ss->ssl3.hs.messages.buf && !ss->ssl3.hs.messages.space);
     ss->ssl3.hs.messages.buf = NULL;
     ss->ssl3.hs.messages.space = 0;
@@ -13917,6 +13982,12 @@ ssl3_DestroySSL3Info(sslSocket *ss)
     if (ss->ssl3.clientPrivateKey != NULL)
         SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
 
+    if (ss->ssl3.hs.clientAuthSignatureSchemes != NULL) {
+        PORT_Free(ss->ssl3.hs.clientAuthSignatureSchemes);
+        ss->ssl3.hs.clientAuthSignatureSchemes = NULL;
+        ss->ssl3.hs.clientAuthSignatureSchemesLen = 0;
+    }
+
     if (ss->ssl3.peerCertArena != NULL)
         ssl3_CleanupPeerCerts(ss);
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 7b4e73c88..5a9da21a0 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -666,6 +666,12 @@ typedef struct SSL3HandshakeStateStr {
         PRUint8 data[72];
     } finishedMsgs;
 
+    /* True when handshake is blocked on client certificate selection */
+    PRBool clientCertificatePending;
+    /* Parameters stored whilst waiting for client certificate */
+    SSLSignatureScheme *clientAuthSignatureSchemes;
+    unsigned int clientAuthSignatureSchemesLen;
+
     PRBool authCertificatePending;
     /* Which function should SSL_RestartHandshake* call if we're blocked?
      * One of NULL, ssl3_SendClientSecondRound, ssl3_FinishHandshake,
@@ -1139,10 +1145,6 @@ struct sslSocketStr {
 
     /* An out-of-band PSK. */
     sslPsk *psk;
-
-    /* peer data passed in during getClientAuthData */
-    const SSLSignatureScheme *peerSignatureSchemes;
-    unsigned int peerSignatureSchemeCount;
 };
 
 struct sslSelfEncryptKeysStr {
@@ -1468,6 +1470,7 @@ extern SECStatus SSL3_SendAlert(sslSocket *ss, SSL3AlertLevel level,
 extern SECStatus ssl3_DecodeError(sslSocket *ss);
 
 extern SECStatus ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode error);
+extern SECStatus ssl3_ClientCertCallbackComplete(sslSocket *ss, SECStatus outcome, SECKEYPrivateKey *clientPrivateKey, CERTCertificate *clientCertificate);
 
 /*
  * for dealing with SSL 3.0 clients sending SSL 2.0 format hellos
@@ -1750,7 +1753,7 @@ SECStatus ssl_GetCertificateRequestCAs(const sslSocket *ss,
                                        unsigned int *nnamesp);
 SECStatus ssl3_ParseCertificateRequestCAs(sslSocket *ss, PRUint8 **b,
                                           PRUint32 *length, CERTDistNames *ca_list);
-SECStatus ssl3_CompleteHandleCertificateRequest(
+SECStatus ssl3_BeginHandleCertificateRequest(
     sslSocket *ss, const SSLSignatureScheme *signatureSchemes,
     unsigned int signatureSchemeCount, CERTDistNames *ca_list);
 SECStatus ssl_ConstructServerHello(sslSocket *ss, PRBool helloRetry,
diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c
index 281ea927a..0422b39c7 100644
--- a/lib/ssl/sslsecur.c
+++ b/lib/ssl/sslsecur.c
@@ -1293,6 +1293,43 @@ SSL_AuthCertificateComplete(PRFileDesc *fd, PRErrorCode error)
     return rv;
 }
 
+SECStatus
+SSL_ClientCertCallbackComplete(PRFileDesc *fd, SECStatus outcome, SECKEYPrivateKey *clientPrivateKey,
+                               CERTCertificate *clientCertificate)
+{
+    SECStatus rv;
+    sslSocket *ss = ssl_FindSocket(fd);
+
+    if (!ss) {
+        SSL_DBG(("%d: SSL[%d]: bad socket in SSL_ClientCertCallbackComplete",
+                 SSL_GETPID(), fd));
+        return SECFailure;
+    }
+
+    /* There exists a codepath which exercises each lock.
+     * Socket is blocked whilst waiting on this callback anyway. */
+    ssl_Get1stHandshakeLock(ss);
+    ssl_GetRecvBufLock(ss);
+    ssl_GetSSL3HandshakeLock(ss);
+
+    if (!ss->ssl3.hs.clientCertificatePending) {
+        /* Application invoked callback at wrong time */
+        SSL_DBG(("%d: SSL[%d]: socket not waiting for SSL_ClientCertCallbackComplete",
+                 SSL_GETPID(), fd));
+        PORT_SetError(PR_INVALID_STATE_ERROR);
+        rv = SECFailure;
+        goto cleanup;
+    }
+
+    rv = ssl3_ClientCertCallbackComplete(ss, outcome, clientPrivateKey, clientCertificate);
+
+cleanup:
+    ssl_ReleaseRecvBufLock(ss);
+    ssl_ReleaseSSL3HandshakeLock(ss);
+    ssl_Release1stHandshakeLock(ss);
+    return rv;
+}
+
 /* For more info see ssl.h */
 SECStatus
 SSL_SNISocketConfigHook(PRFileDesc *fd, SSLSNISocketConfig func,
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index f8afb7627..6b40be759 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -399,10 +399,6 @@ ssl_DupSocket(sslSocket *os)
                 goto loser;
             }
         }
-        /* The original socket 'owns' the copy of these, so
-         * just set the target copies to zero */
-        ss->peerSignatureSchemes = NULL;
-        ss->peerSignatureSchemeCount = 0;
 
         /* Create security data */
         rv = ssl_CopySecurityInfo(ss, os);
@@ -494,10 +490,6 @@ ssl_DestroySocketContents(sslSocket *ss)
     tls13_ReleaseAntiReplayContext(ss->antiReplay);
 
     tls13_DestroyPsk(ss->psk);
-    /* data in peer Signature schemes comes from the buffer system,
-     * so there is nothing to free here. Make sure that's the case */
-    PORT_Assert(ss->peerSignatureSchemes == NULL);
-    PORT_Assert(ss->peerSignatureSchemeCount == 0);
 
     tls13_DestroyEchConfigs(&ss->echConfigs);
     SECKEY_DestroyPrivateKey(ss->echPrivKey);
@@ -2572,8 +2564,7 @@ SSL_ReconfigFD(PRFileDesc *model, PRFileDesc *fd)
         ss->handshakeCallbackData = sm->handshakeCallbackData;
     if (sm->pkcs11PinArg)
         ss->pkcs11PinArg = sm->pkcs11PinArg;
-    ss->peerSignatureSchemes = NULL;
-    ss->peerSignatureSchemeCount = 0;
+
     return fd;
 }
 
@@ -4245,8 +4236,6 @@ ssl_NewSocket(PRBool makeLocks, SSLProtocolVariant protocolVariant)
     ss->echPubKey = NULL;
     ss->antiReplay = NULL;
     ss->psk = NULL;
-    ss->peerSignatureSchemes = NULL;
-    ss->peerSignatureSchemeCount = 0;
 
     if (makeLocks) {
         rv = ssl_MakeLocks(ss);
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 0188ac1d9..51dfebff3 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -97,9 +97,7 @@ static SECStatus tls13_ComputeFinished(
     const SSL3Hashes *hashes, PRBool sending, PRUint8 *output,
     unsigned int *outputLen, unsigned int maxOutputLen);
 static SECStatus tls13_SendClientSecondRound(sslSocket *ss);
-static SECStatus tls13_SendClientSecondFlight(sslSocket *ss,
-                                              PRBool sendClientCert,
-                                              SSL3AlertDescription *sendAlert);
+static SECStatus tls13_SendClientSecondFlight(sslSocket *ss);
 static SECStatus tls13_FinishHandshake(sslSocket *ss);
 
 const char kHkdfLabelClient[] = "c";
@@ -2639,6 +2637,38 @@ loser:
 }
 
 static SECStatus
+tls13_SendPostHandshakeCertificate(sslSocket *ss)
+{
+    SECStatus rv;
+    if (ss->ssl3.hs.restartTarget) {
+        PR_NOT_REACHED("unexpected ss->ssl3.hs.restartTarget");
+        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+        return SECFailure;
+    }
+
+    if (ss->ssl3.hs.clientCertificatePending) {
+        SSL_TRC(3, ("%d: TLS13[%d]: deferring tls13_SendClientSecondFlight because"
+                    " certificate authentication is still pending.",
+                    SSL_GETPID(), ss->fd));
+        ss->ssl3.hs.restartTarget = tls13_SendPostHandshakeCertificate;
+        PORT_SetError(PR_WOULD_BLOCK_ERROR);
+        return SECFailure;
+    }
+
+    ssl_GetXmitBufLock(ss);
+    rv = tls13_SendClientSecondFlight(ss);
+    ssl_ReleaseXmitBufLock(ss);
+    PORT_Assert(ss->ssl3.hs.ws == idle_handshake);
+    PORT_Assert(ss->ssl3.hs.shaPostHandshake != NULL);
+    PK11_DestroyContext(ss->ssl3.hs.shaPostHandshake, PR_TRUE);
+    ss->ssl3.hs.shaPostHandshake = NULL;
+    if (rv != SECSuccess) {
+        return SECFailure;
+    }
+    return rv;
+}
+
+static SECStatus
 tls13_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
 {
     SECStatus rv;
@@ -2695,12 +2725,19 @@ tls13_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
             SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
             ss->ssl3.clientPrivateKey = NULL;
         }
+        if (ss->ssl3.hs.clientAuthSignatureSchemes != NULL) {
+            PORT_Free(ss->ssl3.hs.clientAuthSignatureSchemes);
+            ss->ssl3.hs.clientAuthSignatureSchemes = NULL;
+            ss->ssl3.hs.clientAuthSignatureSchemesLen = 0;
+        }
         SECITEM_FreeItem(&ss->xtnData.certReqContext, PR_FALSE);
         ss->xtnData.certReqContext.data = NULL;
     } else {
         PORT_Assert(ss->ssl3.clientCertChain == NULL);
         PORT_Assert(ss->ssl3.clientCertificate == NULL);
         PORT_Assert(ss->ssl3.clientPrivateKey == NULL);
+        PORT_Assert(ss->ssl3.hs.clientAuthSignatureSchemes == NULL);
+        PORT_Assert(ss->ssl3.hs.clientAuthSignatureSchemesLen == 0);
         PORT_Assert(!ss->ssl3.hs.clientCertRequested);
         PORT_Assert(ss->xtnData.certReqContext.data == NULL);
     }
@@ -2748,33 +2785,16 @@ tls13_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
     ss->ssl3.hs.clientCertRequested = PR_TRUE;
 
     if (ss->firstHsDone) {
-        SSL3AlertDescription sendAlert = no_alert;
 
         /* Request a client certificate. */
-        rv = ssl3_CompleteHandleCertificateRequest(
+        rv = ssl3_BeginHandleCertificateRequest(
             ss, ss->xtnData.sigSchemes, ss->xtnData.numSigSchemes,
             &ss->xtnData.certReqAuthorities);
         if (rv != SECSuccess) {
             FATAL_ERROR(ss, SEC_ERROR_LIBRARY_FAILURE, internal_error);
             return rv;
         }
-
-        ssl_GetXmitBufLock(ss);
-        rv = tls13_SendClientSecondFlight(ss, !ss->ssl3.sendEmptyCert,
-                                          &sendAlert);
-        ssl_ReleaseXmitBufLock(ss);
-        if (rv != SECSuccess) {
-            if (sendAlert != no_alert) {
-                FATAL_ERROR(ss, PORT_GetError(), sendAlert);
-            } else {
-                LOG_ERROR(ss, PORT_GetError());
-            }
-            return SECFailure;
-        }
-        PORT_Assert(ss->ssl3.hs.ws == idle_handshake);
-        PORT_Assert(ss->ssl3.hs.shaPostHandshake != NULL);
-        PK11_DestroyContext(ss->ssl3.hs.shaPostHandshake, PR_TRUE);
-        ss->ssl3.hs.shaPostHandshake = NULL;
+        rv = tls13_SendPostHandshakeCertificate(ss);
     } else {
         TLS13_SET_HS_STATE(ss, wait_server_cert);
     }
@@ -4553,7 +4573,7 @@ tls13_HandleCertificateVerify(sslSocket *ss, PRUint8 *b, PRUint32 length)
     /* Request a client certificate now if one was requested. */
     if (ss->ssl3.hs.clientCertRequested) {
         PORT_Assert(!ss->sec.isServer);
-        rv = ssl3_CompleteHandleCertificateRequest(
+        rv = ssl3_BeginHandleCertificateRequest(
             ss, ss->xtnData.sigSchemes, ss->xtnData.numSigSchemes,
             &ss->xtnData.certReqAuthorities);
         if (rv != SECSuccess) {
@@ -5052,15 +5072,17 @@ tls13_FinishHandshake(sslSocket *ss)
 /* Do the parts of sending the client's second round that require
  * the XmitBuf lock. */
 static SECStatus
-tls13_SendClientSecondFlight(sslSocket *ss, PRBool sendClientCert,
-                             SSL3AlertDescription *sendAlert)
+tls13_SendClientSecondFlight(sslSocket *ss)
 {
     SECStatus rv;
     unsigned int offset = 0;
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
+    PORT_Assert(!ss->ssl3.hs.clientCertificatePending);
 
-    *sendAlert = internal_error;
+    PRBool sendClientCert = !ss->ssl3.sendEmptyCert &&
+                            ss->ssl3.clientCertChain != NULL &&
+                            ss->ssl3.clientPrivateKey != NULL;
 
     if (ss->firstHsDone) {
         offset = SSL_BUFFER_LEN(&ss->sec.ci.sendBuf);
@@ -5071,12 +5093,12 @@ tls13_SendClientSecondFlight(sslSocket *ss, PRBool sendClientCert,
         rv = ssl3_SendEmptyCertificate(ss);
         /* Don't send verify */
         if (rv != SECSuccess) {
-            return SECFailure; /* error code is set. */
+            goto alert_error; /* error code is set. */
         }
     } else if (sendClientCert) {
         rv = tls13_SendCertificate(ss);
         if (rv != SECSuccess) {
-            return SECFailure; /* error code is set. */
+            goto alert_error; /* err code was set. */
         }
     }
 
@@ -5085,7 +5107,7 @@ tls13_SendClientSecondFlight(sslSocket *ss, PRBool sendClientCert,
                                             SSL_BUFFER_BASE(&ss->sec.ci.sendBuf) + offset,
                                             SSL_BUFFER_LEN(&ss->sec.ci.sendBuf) - offset);
         if (rv != SECSuccess) {
-            return SECFailure; /* error code is set. */
+            goto alert_error; /* err code was set. */
         }
     }
 
@@ -5109,7 +5131,7 @@ tls13_SendClientSecondFlight(sslSocket *ss, PRBool sendClientCert,
         SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
         ss->ssl3.clientPrivateKey = NULL;
         if (rv != SECSuccess) {
-            return SECFailure; /* err is set. */
+            goto alert_error; /* err code was set. */
         }
 
         if (ss->firstHsDone) {
@@ -5117,40 +5139,40 @@ tls13_SendClientSecondFlight(sslSocket *ss, PRBool sendClientCert,
                                                 SSL_BUFFER_BASE(&ss->sec.ci.sendBuf) + offset,
                                                 SSL_BUFFER_LEN(&ss->sec.ci.sendBuf) - offset);
             if (rv != SECSuccess) {
-                return SECFailure; /* error is set. */
+                goto alert_error; /* err code was set. */
             }
         }
     }
 
     rv = tls13_SendFinished(ss, ss->firstHsDone ? ss->ssl3.hs.clientTrafficSecret : ss->ssl3.hs.clientHsTrafficSecret);
     if (rv != SECSuccess) {
-        return SECFailure; /* err code was set. */
+        goto alert_error; /* err code was set. */
     }
     rv = ssl3_FlushHandshake(ss, 0);
     if (rv != SECSuccess) {
         /* No point in sending an alert here because we're not going to
          * be able to send it if we couldn't flush the handshake. */
-        *sendAlert = no_alert;
-        return SECFailure;
+        goto error;
     }
 
     return SECSuccess;
+
+alert_error:
+    FATAL_ERROR(ss, PORT_GetError(), internal_error);
+    return SECFailure;
+error:
+    LOG_ERROR(ss, PORT_GetError());
+    return SECFailure;
 }
 
 static SECStatus
 tls13_SendClientSecondRound(sslSocket *ss)
 {
     SECStatus rv;
-    PRBool sendClientCert;
-    SSL3AlertDescription sendAlert = no_alert;
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
-    sendClientCert = !ss->ssl3.sendEmptyCert &&
-                     ss->ssl3.clientCertChain != NULL &&
-                     ss->ssl3.clientPrivateKey != NULL;
-
     /* Defer client authentication sending if we are still waiting for server
      * authentication.  This avoids unnecessary disclosure of client credentials
      * to an unauthenticated server.
@@ -5160,8 +5182,8 @@ tls13_SendClientSecondRound(sslSocket *ss)
         PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
         return SECFailure;
     }
-    if (ss->ssl3.hs.authCertificatePending) {
-        SSL_TRC(3, ("%d: TLS13[%d]: deferring ssl3_SendClientSecondRound because"
+    if (ss->ssl3.hs.authCertificatePending || ss->ssl3.hs.clientCertificatePending) {
+        SSL_TRC(3, ("%d: TLS13[%d]: deferring tls13_SendClientSecondRound because"
                     " certificate authentication is still pending.",
                     SSL_GETPID(), ss->fd));
         ss->ssl3.hs.restartTarget = tls13_SendClientSecondRound;
@@ -5208,14 +5230,10 @@ tls13_SendClientSecondRound(sslSocket *ss)
     }
 
     ssl_GetXmitBufLock(ss); /*******************************/
-    rv = tls13_SendClientSecondFlight(ss, sendClientCert, &sendAlert);
+    /* This call can't block, as clientAuthCertificatePending is checked above */
+    rv = tls13_SendClientSecondFlight(ss);
     ssl_ReleaseXmitBufLock(ss); /*******************************/
     if (rv != SECSuccess) {
-        if (sendAlert != no_alert) {
-            FATAL_ERROR(ss, PORT_GetError(), sendAlert);
-        } else {
-            LOG_ERROR(ss, PORT_GetError());
-        }
         return SECFailure;
     }
     rv = tls13_SetCipherSpec(ss, TrafficKeyApplicationData,