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;
 }
 
+static SECStatus
+ssl3_CheckFalseStart(sslSocket *ss)
+{
+    SECStatus rv;
+    PRBool maybeFalseStart = PR_FALSE;
+
+    PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
+    PORT_Assert( !ss->ssl3.hs.authCertificatePending );
+    PORT_Assert( !ss->ssl3.hs.canFalseStart );
+
+    if (!ss->canFalseStartCallback) {
+        SSL_TRC(3, ("%d: SSL[%d]: no false start callback so no false start",
+                    SSL_GETPID(), ss->fd));
+    } else {
+        /* An attacker can control the selected ciphersuite so we only wish to

[wtc, 2013/10/29 01:21:51]

Nit: you can still declare maybeFalseStart in this block because it is only used
in this block.

[briansmith, 2013/10/29 21:00:13]

Done. Also did the same for rv.

+         * 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);
+
+        if (!maybeFalseStart) {
+            SSL_TRC(3, ("%d: SSL[%d]: no false start due to weak cipher",
+                        SSL_GETPID(), ss->fd));
+        } else {
+            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;
+        }
+    }
+
+    ss->ssl3.hs.canFalseStart = PR_FALSE;
+    return SECSuccess;
+}
+
 PRBool
-ssl3_CanFalseStart(sslSocket *ss) {
-    PRBool rv;
+ssl3_WaitingForStartOfServerSecondRound(sslSocket *ss)
+{
+    PRBool result = PR_FALSE;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
 
-    /* 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.
-     */
+    switch (ss->ssl3.hs.ws) {
+    case wait_new_session_ticket:
+        result = PR_TRUE;
+        break;
+    case wait_change_cipher:
+        result = !ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn);
+        break;
+    case wait_finished:
+        break;
+    default:
+        PR_NOT_REACHED("ssl3_WaitingForStartOfServerSecondRound");
+    }
 
-    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 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;
+    return result;
 }
 
 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);
-    }
+    PORT_Assert(ssl3_WaitingForStartOfServerSecondRound(ss));

[wtc, 2013/10/29 01:21:51]

Nit: this assertion will never fail.

[briansmith, 2013/10/29 21:00:13]

Ideally no assertions will fail. This is mostly for documentation's sake and to
sanity-check the implementation of the ssl3_WaitingForStartOfServerSecondRound
function itself.

 
     return SECSuccess;
 
 loser:
     ssl_ReleaseXmitBufLock(ss);
     return rv;
 }
 
 /*
  * Routines used by servers
@@ skipping 2597 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));
+        }
+
         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_new_session_ticket ||
+                    ss->ssl3.hs.ws == wait_change_cipher ||
+                    ss->ssl3.hs.ws == wait_finished);

[briansmith, 2013/10/25 19:17:14]

This change is just putting the states in chronological order instead of
alphabetical order, to make things a little clearer.

+
+        /* ssl3_SendClientSecondRound deferred the false start check because
+         * certificate authentication was pending, so we have to do it now.
+         *
+         * If we've received any part of the server's second round then don't
+         * bother false starting. See the false-start-ws comment in

[wtc, 2013/10/29 19:54:22]

Nit: "false-start-ws" should be clarified.

+         * ssl3_GatherCompleteHandshake , since it is almost always the case that the
+         * NewSessionTicket,
+         * ChangeCipherSoec, and Finished mesage were sent in the same packet

[wtc, 2013/10/29 01:21:51]

Nit: these three lines of comments should be reformatted.

Line 10380 seems too long.

Line 10381 is too short.

[briansmith, 2013/10/29 21:00:13]

I removed this part of the comment since it is redundant with the documentation
for ssl3_WaitingForStartOfServerSecondRound anyway.

+         * and we want to process them all at the same time.

[briansmith, 2013/10/25 19:17:14]

This comment addition is redundant with the documentation comment for
ssl3_WaitingForStartOfServerSecondRound and I forgot to remove it when I added
the comment for ssl3_WaitingForStartOfServerSecondRound.

+         */
+        if (ss->opt.enableFalseStart &&
+            !ss->firstHsDone &&
+            !ss->sec.isServer &&
+            !ss->ssl3.hs.isResuming &&
+            ssl3_WaitingForStartOfServerSecondRound(ss)) {
+            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 551 matching lines @@
     unsigned int         good;
     unsigned int         minLength;
     unsigned char        header[13];
     unsigned int         headerLen;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
 
     if (!ss->ssl3.initialized) {
         ssl_GetSSL3HandshakeLock(ss);
         rv = ssl3_InitState(ss);
         ssl_ReleaseSSL3HandshakeLock(ss);

[briansmith, 2013/10/25 19:17:14]

This function was reverted to the way it was before on your request.

         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 372 matching lines @@
         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;
 

[wtc, 2013/10/29 01:21:51]

Delete this blank line.

[briansmith, 2013/10/29 21:00:13]

Done.

 }
 
 /*
  * 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 */