Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/ssl3con.c

 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */
 
@@ skipping 2872 matching lines @@
                    PRInt32            flags)
 {
     sslBuffer      *          wrBuf       = &ss->sec.writeBuf;
     SECStatus                 rv;
     PRInt32                   totalSent   = 0;
     PRBool                    capRecordVersion;
 
     SSL_TRC(3, ("%d: SSL3[%d] SendRecord type: %s nIn=%d",
                 SSL_GETPID(), ss->fd, ssl3_DecodeContentType(type),
                 nIn));
-    PRINT_BUF(3, (ss, "Send record (plain text)", pIn, nIn));
+    PRINT_BUF(50, (ss, "Send record (plain text)", pIn, nIn));
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss) );
 
     capRecordVersion = ((flags & ssl_SEND_FLAG_CAP_RECORD_VERSION) != 0);
 
     if (capRecordVersion) {
         /* ssl_SEND_FLAG_CAP_RECORD_VERSION can only be used with the
          * TLS initial ClientHello. */
         PORT_Assert(!IS_DTLS(ss));
         PORT_Assert(!ss->firstHsDone);
@@ skipping 4433 matching lines @@
         }
         if (certChain) {
             CERT_DestroyCertificateList(certChain);
         }
         PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
         rv = SECFailure;
     }
     return rv;
 }
 
-PRBool
-ssl3_CanFalseStart(sslSocket *ss) {
-    PRBool rv;
+static SECStatus
+ssl3_CheckFalseStart(sslSocket *ss)
+{
+    SECStatus rv;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
+    PORT_Assert( !ss->ssl3.hs.authCertificatePending );
 
-    /* XXX: does not take into account whether we are waiting for
-     * SSL_AuthCertificateComplete or SSL_RestartHandshakeAfterCertReq. If/when
-     * that is done, this function could return different results each time it
-     * would be called.
-     */
+    if (ss->canFalseStartCallback) {
+        PRBool maybeFalseStart;
 
-    ssl_GetSpecReadLock(ss);
-    rv = ss->opt.enableFalseStart &&
-         !ss->sec.isServer &&
-         !ss->ssl3.hs.isResuming &&
-         ss->ssl3.cwSpec &&
+        /* An attacker can control the selected ciphersuite so we only wish to
+         * do False Start in the case that the selected ciphersuite is
+         * sufficiently strong that the attack can gain no advantage.
+         * Therefore we always require an 80-bit cipher. */
+        ssl_GetSpecReadLock(ss);
+        maybeFalseStart = ss->ssl3.cwSpec->cipher_def->secret_key_size >= 10;
+        ssl_ReleaseSpecReadLock(ss);
 
-         /* An attacker can control the selected ciphersuite so we only wish to
-          * do False Start in the case that the selected ciphersuite is
-          * sufficiently strong that the attack can gain no advantage.
-          * Therefore we require an 80-bit cipher and a forward-secret key
-          * exchange. */
-         ss->ssl3.cwSpec->cipher_def->secret_key_size >= 10 &&
-        (ss->ssl3.hs.kea_def->kea == kea_dhe_dss ||
-         ss->ssl3.hs.kea_def->kea == kea_dhe_rsa ||
-         ss->ssl3.hs.kea_def->kea == kea_ecdhe_ecdsa ||
-         ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa);
-    ssl_ReleaseSpecReadLock(ss);
-    return rv;
+        if (maybeFalseStart) {
+            rv = (ss->canFalseStartCallback)(ss->fd,
+                                             ss->canFalseStartCallbackData,
+                                             &ss->ssl3.hs.canFalseStart);
+            if (rv == SECSuccess) {
+                SSL_TRC(3, ("%d: SSL[%d]: false start callback returned %s",
+                            SSL_GETPID(), ss->fd,
+                            ss->ssl3.hs.canFalseStart ? "TRUE" : "FALSE"));
+            } else {
+                SSL_TRC(3, ("%d: SSL[%d]: false start callback failed (%s)",
+                            SSL_GETPID(), ss->fd,
+                            PR_ErrorToName(PR_GetError())));
+            }
+            return rv;
+        }
+    }
+
+    SSL_TRC(3, ("%d: SSL[%d]: no false start callback so no false start",
+                SSL_GETPID(), ss->fd));
+
+    ss->ssl3.hs.canFalseStart = PR_FALSE;
+    return SECSuccess;

[wtc, 2013/10/17 00:10:53]

Nit: it seems better to handle the "no false start callback" case first because
it has much fewer lines of code.

But I see that the "have false start callback" case may also fall through here.
So the SSL_TRC message on line 7383 needs to be updated to say "weak cipher
suite or no false start callback ..."

[briansmith, 2013/10/17 01:42:40]

Done.

 }
 
 static SECStatus ssl3_SendClientSecondRound(sslSocket *ss);
 
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
  * ssl3 Server Hello Done message.
  * Caller must hold Handshake and RecvBuf locks.
  */
 static SECStatus
 ssl3_HandleServerHelloDone(sslSocket *ss)
@@ skipping 69 matching lines @@
      * before using the cipher specs agreed upon for that handshake for
      * application data.
      */
     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.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));
         ss->ssl3.hs.restartTarget = ssl3_SendClientSecondRound;
         return SECWouldBlock;
     }
 
     ssl_GetXmitBufLock(ss);             /*******************************/
 
     if (ss->ssl3.sendEmptyCert) {
         ss->ssl3.sendEmptyCert = PR_FALSE;
         rv = ssl3_SendEmptyCertificate(ss);
         /* Don't send verify */
@@ skipping 17 matching lines @@
         if (rv != SECSuccess) {
             goto loser; /* err is set. */
         }
     }
 
     rv = ssl3_SendChangeCipherSpecs(ss);
     if (rv != SECSuccess) {
         goto loser;     /* err code was set. */
     }
 
-    /* XXX: If the server's certificate hasn't been authenticated by this
-     * point, then we may be leaking this NPN message to an attacker.
+    /* This must be done after we've set ss->ssl3.cwSpec in
+     * ssl3_SendChangeCipherSpecs because SSL_GetChannelInfo uses information
+     * from cwSpec. This must be done before we call ssl3_CheckFalseStart
+     * because the false start callback (if any) may need the information from
+     * the functions that depend on this being set.
      */
+    ss->enoughFirstHsDone = PR_TRUE;
+
     if (!ss->firstHsDone) {
+        /* XXX: If the server's certificate hasn't been authenticated by this
+         * point, then we may be leaking this NPN message to an attacker.
+         */
         rv = ssl3_SendNextProto(ss);
         if (rv != SECSuccess) {
             goto loser; /* err code was set. */
         }
     }
+
     rv = ssl3_SendEncryptedExtensions(ss);
     if (rv != SECSuccess) {
         goto loser; /* err code was set. */
     }
 
+    if (!ss->firstHsDone) {
+        if (ss->opt.enableFalseStart) {
+            if (!ss->ssl3.hs.authCertificatePending) {
+                /* When we fix bug 589047, we will need to know whether we are
+                 * false starting before we try to flush the client second
+                 * round to the network. With that in mind, we purposefully
+                 * call ssl3_CheckFalseStart before calling ssl3_SendFinished,
+                 * which includes a call to ssl3_FlushHandshake, so that
+                 * no application develops a reliance on such flushing being
+                 * done before its false start callback is called. 
+                 */
+                ssl_ReleaseXmitBufLock(ss);
+                rv = ssl3_CheckFalseStart(ss);
+                ssl_GetXmitBufLock(ss);
+                if (rv != SECSuccess) {
+                    goto loser;
+                }
+            } else {
+                /* The certificate authentication and the server's Finished
+                 * message are racing each other. If the certificate
+                 * authentication wins, then we will try to false start in
+                 * ssl3_AuthCertificateComplete.
+                 */
+                SSL_TRC(3, ("%d: SSL3[%p]: deferring false start check because"
+                            " certificate authentication is still pending.",
+                            SSL_GETPID(), ss->fd));
+            }
+        }
+    }
+
     rv = ssl3_SendFinished(ss, 0);
     if (rv != SECSuccess) {
         goto loser;     /* err code was set. */
     }
 
     ssl_ReleaseXmitBufLock(ss);         /*******************************/
 
     if (ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn))
         ss->ssl3.hs.ws = wait_new_session_ticket;
     else
         ss->ssl3.hs.ws = wait_change_cipher;
 
-    /* Do the handshake callback for sslv3 here, if we can false start. */
-    if (ss->handshakeCallback != NULL && ssl3_CanFalseStart(ss)) {
-        (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData);
-    }
-
     return SECSuccess;
 
 loser:
     ssl_ReleaseXmitBufLock(ss);
     return rv;
 }
 
 /*
  * Routines used by servers
  */
@@ skipping 2596 matching lines @@
 
         if (rv == SECWouldBlock) {
             if (ss->sec.isServer) {
                 errCode = SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SERVERS;
                 rv = SECFailure;
                 goto loser;
             }
 
             ss->ssl3.hs.authCertificatePending = PR_TRUE;
             rv = SECSuccess;
-
-            /* XXX: Async cert validation and False Start don't work together
-             * safely yet; if we leave False Start enabled, we may end up false
-             * starting (sending application data) before we
-             * SSL_AuthCertificateComplete has been called.
-             */
-            ss->opt.enableFalseStart = PR_FALSE;
         }
 
         if (rv != SECSuccess) {
             ssl3_SendAlertForCertError(ss, errCode);
             goto loser;
         }
     }
 
     ss->sec.ci.sid->peerCert = CERT_DupCertificate(ss->sec.peerCert);
     ssl3_CopyPeerCertsToSID(ss->ssl3.peerCertChain, ss->sec.ci.sid);
@@ skipping 104 matching lines @@
 
     ss->ssl3.hs.authCertificatePending = PR_FALSE;
 
     if (error != 0) {
         ss->ssl3.hs.restartTarget = ssl3_AlwaysFail;
         ssl3_SendAlertForCertError(ss, error);
         rv = SECSuccess;
     } else if (ss->ssl3.hs.restartTarget != NULL) {
         sslRestartTarget target = ss->ssl3.hs.restartTarget;
         ss->ssl3.hs.restartTarget = NULL;
+
+        if (target == ssl3_FinishHandshake) {
+            SSL_TRC(3,("%d: SSL3[%p]: certificate authentication lost the race"
+                       " with peer's finished message", SSL_GETPID(), ss->fd));
+        }

[wtc, 2013/10/17 00:10:53]

If |target| is ssl3_SendClientSecondRound here, does that also mean certificate
authentication won the race with peer's finished message?

Or does that mean we canceled the race (when we set ss->ssl3.hs.restartTarget to
ssl3_SendClientSecondRound)?

[briansmith, 2013/10/17 01:42:40]

if target == ssl3_SendClientSecondRound then that means we haven't sent our
client key exchange message yet so there is no race since the server is waiting
for us to send our client key exchange and other "client second round" stuff.

+
         rv = target(ss);
         /* Even if we blocked here, we have accomplished enough to claim
          * success. Any remaining work will be taken care of by subsequent
          * calls to SSL_ForceHandshake/PR_Send/PR_Read/etc. 
          */
         if (rv == SECWouldBlock) {
             rv = SECSuccess;
         }
     } else {
-        rv = SECSuccess;
+        SSL_TRC(3, ("%d: SSL3[%p]: certificate authentication won the race with"
+                    " peer's finished message", SSL_GETPID(), ss->fd));
+
+        PORT_Assert(!ss->firstHsDone);
+        PORT_Assert(!ss->sec.isServer);
+        PORT_Assert(!ss->ssl3.hs.isResuming);
+        PORT_Assert(ss->ssl3.hs.ws == wait_change_cipher ||
+                    ss->ssl3.hs.ws == wait_finished ||
+                    ss->ssl3.hs.ws == wait_new_session_ticket);
+
+        /* ssl3_SendClientSecondRound deferred the false start check because
+         * certificate authentication was pending, so we have to do it now.
+         */
+        if (ss->opt.enableFalseStart &&
+            !ss->firstHsDone &&
+            !ss->sec.isServer &&
+            !ss->ssl3.hs.isResuming &&
+            (ss->ssl3.hs.ws == wait_change_cipher ||
+             ss->ssl3.hs.ws == wait_finished ||
+             ss->ssl3.hs.ws == wait_new_session_ticket)) {
+            rv = ssl3_CheckFalseStart(ss);
+        } else {
+            rv = SECSuccess;
+        }
     }
 
 done:
     ssl_ReleaseSSL3HandshakeLock(ss);
     ssl_ReleaseRecvBufLock(ss);
 
     return rv;
 }
 
 static SECStatus
@@ skipping 605 matching lines @@
             goto xmit_loser;    /* err is set. */
         }
     }
 
 xmit_loser:
     ssl_ReleaseXmitBufLock(ss); /*************************************/
     if (rv != SECSuccess) {
         return rv;
     }
 
-    ss->gs.writeOffset = 0;
-    ss->gs.readOffset  = 0;
-
     if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) {
         effectiveExchKeyType = kt_rsa;
     } else {
         effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType;
     }
 
     if (sid->cached == never_cached && !ss->opt.noCache && ss->sec.cache) {
         /* fill in the sid */
         sid->u.ssl3.cipherSuite = ss->ssl3.hs.cipher_suite;
         sid->u.ssl3.compression = ss->ssl3.hs.compression;
@@ skipping 44 matching lines @@
         }
 
         ss->ssl3.hs.restartTarget = ssl3_FinishHandshake;
         return SECWouldBlock;
     }
 
     rv = ssl3_FinishHandshake(ss);
     return rv;
 }
 
+/* The return type is SECStatus instead of void because this function needs
+ * to have type sslRestartTarget.
+ */
 SECStatus
 ssl3_FinishHandshake(sslSocket * ss)
 {
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
     PORT_Assert( ss->ssl3.hs.restartTarget == NULL );
 
     /* The first handshake is now completed. */
     ss->handshake           = NULL;
-    ss->firstHsDone         = PR_TRUE;
 
     if (ss->ssl3.hs.cacheSID) {
         (*ss->sec.cache)(ss->sec.ci.sid);
         ss->ssl3.hs.cacheSID = PR_FALSE;
     }
 
+    ss->ssl3.hs.canFalseStart = PR_FALSE; /* False Start phase is complete */
     ss->ssl3.hs.ws = idle_handshake;
 
-    /* Do the handshake callback for sslv3 here, if we cannot false start. */
-    if (ss->handshakeCallback != NULL && !ssl3_CanFalseStart(ss)) {
-        (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData);
-    }
+    ssl_FinishHandshake(ss);
 
     return SECSuccess;
 }
 
 /* Called from ssl3_HandleHandshake() when it has gathered a complete ssl3
  * hanshake message.
  * Caller must hold Handshake and RecvBuf locks.
  */
 SECStatus
 ssl3_HandleHandshakeMessage(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
@@ skipping 547 matching lines @@
     sslBuffer            temp_buf;
     PRUint64             dtls_seq_num;
     unsigned int         ivLen = 0;
     unsigned int         originalLen = 0;
     unsigned int         good;
     unsigned int         minLength;
     unsigned char        header[13];
     unsigned int         headerLen;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
+    PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );

[wtc, 2013/10/17 00:10:53]

Why do we need to call ssl3_HandleRecord while holding SSL3HandshakeLock?
Did you discover that we are already calling ssl3_HandleRecord with
SSL3HandshakeLock held?

[briansmith, 2013/10/17 01:42:40]

ssl3_GatherCompleteHandshake is the function that calls ssl3_HandleRecord. In
ssl3_GatherCompleteHandshake, we were acquiring and releasing the
ssl_HaveSSL3HandshakeLock over and over again. Also, we were checking
ss->ssl3.hs.ws != idle_handshake at the end of the do { } while loop without
holding the SSL3HandshakeLock, which seemed wrong. So, I fixed that function by
holding the SSL3HandshakeLock for the duration of that loop. The updates to this
function are a consequence of the updates to ssl3_GatherCompleteHandshake.

[wtc, 2013/10/17 15:28:14]

I see. We should reconsider this change. This function makes no apparent use of
ss->ssl3.hs (which I think is protected by the SSL3HandshakeLock) for about 350
lines of code until the very end. I will also discuss this issue in
ssl3_GatherCompleteHandshake.

 
     if (!ss->ssl3.initialized) {
-        ssl_GetSSL3HandshakeLock(ss);
         rv = ssl3_InitState(ss);
-        ssl_ReleaseSSL3HandshakeLock(ss);
         if (rv != SECSuccess) {
             return rv;          /* ssl3_InitState has set the error code. */
         }
     }
 
     /* check for Token Presence */
     if (!ssl3_ClientAuthTokenPresent(ss->sec.ci.sid)) {
         PORT_SetError(SSL_ERROR_TOKEN_INSERTION_REMOVAL);
         return SECFailure;
     }
@@ skipping 339 matching lines @@
         PORT_SetError(SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA);
         return SECFailure;
     }
 
     /* It's a record that must be handled by ssl itself, not the application.
     */
 process_it:
     /* XXX  Get the xmit lock here.  Odds are very high that we'll be xmiting
      * data ang getting the xmit lock here prevents deadlocks.
      */
-    ssl_GetSSL3HandshakeLock(ss);
 
     /* All the functions called in this switch MUST set error code if
     ** they return SECFailure or SECWouldBlock.
     */
     switch (rType) {
     case content_change_cipher_spec:
         rv = ssl3_HandleChangeCipherSpecs(ss, databuf);
         break;
     case content_alert:
         rv = ssl3_HandleAlert(ss, databuf);
@@ skipping 10 matching lines @@
     */
     default:
         SSL_DBG(("%d: SSL3[%d]: bogus content type=%d",
                  SSL_GETPID(), ss->fd, cText->type));
         /* XXX Send an alert ???  */
         PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE);
         rv = SECFailure;
         break;
     }
 
-    ssl_ReleaseSSL3HandshakeLock(ss);
     return rv;
-
 }
 
 /*
  * Initialization functions
  */
 
 /* Called from ssl3_InitState, immediately below. */
 /* Caller must hold the SpecWriteLock. */
 static void
 ssl3_InitCipherSpec(sslSocket *ss, ssl3CipherSpec *spec)
@@ skipping 490 matching lines @@
             PORT_Free(ss->ssl3.hs.recvdFragments.buf);
         }
     }
 
     ss->ssl3.initialized = PR_FALSE;
 
     SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
 }
 
 /* End of ssl3con.c */