Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/sslsecur.c

 /*
  * Various SSL functions.
  *
  * 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/. */
 #include "cert.h"
 #include "secitem.h"
 #include "keyhi.h"
 #include "ssl.h"
@@ skipping 79 matching lines @@
             /* Previous handshake finished. Switch to next one */
             ss->handshake = ss->nextHandshake;
             ss->nextHandshake = 0;
         }
         if (ss->handshake == 0) {
             /* Previous handshake finished. Switch to security handshake */
             ss->handshake = ss->securityHandshake;
             ss->securityHandshake = 0;
         }
         if (ss->handshake == 0) {
-            ssl_GetRecvBufLock(ss);
-            ss->gs.recordLen = 0;
-            ssl_ReleaseRecvBufLock(ss);
-
-            SSL_TRC(3, ("%d: SSL[%d]: handshake is completed",
-                        SSL_GETPID(), ss->fd));
-            /* call handshake callback for ssl v2 */
-            /* for v3 this is done in ssl3_HandleFinished() */
-            if ((ss->handshakeCallback != NULL) && /* has callback */
-                (!ss->firstHsDone) &&              /* only first time */
-                (ss->version < SSL_LIBRARY_VERSION_3_0)) {  /* not ssl3 */
-                ss->firstHsDone     = PR_TRUE;
-                (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData);
+            /* for v3 this is done in ssl3_FinishHandshake */
+            if (!ss->firstHsDone && ss->version < SSL_LIBRARY_VERSION_3_0) {
+                ssl_GetRecvBufLock(ss);
+                ss->gs.recordLen = 0;
+                ssl_ReleaseRecvBufLock(ss);

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

Why do we only need to set ss->gs.recordLen to 0 for SSL 2.0?

Can these three lines (lines 102-104) be moved into ssl_FinishHandshake?

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

recordLen is only used for SSL 2.0:

unsigned int  recordLen; /* ssl2 only */

I verified that that is the case.
They could, but I did not do that because we should keep the SSL-2.0-specific
functionality separated so that it is clear what to remove when we remove SSL
2.0 support later.

+                ssl_FinishHandshake(ss);
             }
-            ss->firstHsDone         = PR_TRUE;
-            ss->gs.writeOffset = 0;
-            ss->gs.readOffset  = 0;
             break;
         }
         rv = (*ss->handshake)(ss);
         ++loopCount;
     /* This code must continue to loop on SECWouldBlock, 
      * or any positive value.   See XXX_1 comments.
      */
     } while (rv != SECFailure);         /* was (rv >= 0); XXX_1 */
 
     PORT_Assert(ss->opt.noLocks || !ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || !ssl_HaveXmitBufLock(ss));
     PORT_Assert(ss->opt.noLocks || !ssl_HaveSSL3HandshakeLock(ss));
 
     if (rv == SECWouldBlock) {
         PORT_SetError(PR_WOULD_BLOCK_ERROR);
         rv = SECFailure;
     }
     return rv;
 }
 
+void
+ssl_FinishHandshake(sslSocket *ss)

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

Should we move this function to sslcon.c, since it is SSL 2.0 only?

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

This is not SSL-2.0-only. This is the functionality that is common to both SSL
2.0 and SSL 3.0+. It is called by ssl3_FinishHandshake too.

I think we should leave it in this file because it deals with fields of ss that
the rest of the functions in this file deal with.

+{
+    PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );

[briansmith, 2013/10/17 19:09:01]

Wan-Teh, do you agree that this assertion is correct? That is, do you agree that
we must be holding the 1stHandshakeLock when we update ss->firstHsDone?

If so, then we must grab the 1stHandshakeLock before we call ssl3_HandleRecord.
That in turn means we must be holding the 1stHandshakeLock before we call
ssl3_GatherCompleteHandshake. That in turn means that we must be holding the
lock before we call ssl3_GatherAppDataRecord. That means we must acquire the
1stHandshakeLock in DoRecv. Note that the 1stHandshakeLock must be acquired
before acquiring the RecvBufLock, according to the comment for
ssl_Get1stHandshakeLock in sslimpl.h

This is why I acquire the 1stHanshakeLock in DoRecv.

It seems I should add this here too:
PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );

[wtc, 2013/10/17 23:13:01]

This seems very reasonable, but I didn't find this documented in
sslimpl.h. See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssli...

So I don't really know :-) Also, this code checks ss->firstHsDone without
holding the 1stHandshakeLock:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssls...
Thank you very much for the explanation.
 
I did notice that we should get the RecvBufLock when modifying
ss->gs.writeOffset and ss->gs.readOffset.

+
+    SSL_TRC(3, ("%d: SSL[%d]: handshake is completed", SSL_GETPID(), ss->fd));
+
+    ss->firstHsDone = PR_TRUE;
+    ss->enoughFirstHsDone = PR_TRUE;
+    ss->gs.writeOffset = 0;
+    ss->gs.readOffset  = 0;
+
+    if (ss->handshakeCallback) {
+        (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData);
+    }
+}
+
 /*
  * Handshake function that blocks.  Used to force a
  * retry on a connection on the next read/write.
  */
 static SECStatus
 ssl3_AlwaysBlock(sslSocket *ss)
 {
     PORT_SetError(PR_WOULD_BLOCK_ERROR);        /* perhaps redundant. */
     return SECWouldBlock;
 }
@@ skipping 52 matching lines @@
     if (!ss->opt.useSecurity)
         return SECSuccess;
 
     SSL_LOCK_READER(ss);
     SSL_LOCK_WRITER(ss);
 
     /* Reset handshake state */
     ssl_Get1stHandshakeLock(ss);
 
     ss->firstHsDone = PR_FALSE;
+    ss->enoughFirstHsDone = PR_FALSE;
     if ( asServer ) {
         ss->handshake = ssl2_BeginServerHandshake;
         ss->handshaking = sslHandshakingAsServer;
     } else {
         ss->handshake = ssl2_BeginClientHandshake;
         ss->handshaking = sslHandshakingAsClient;
     }
     ss->nextHandshake       = 0;
     ss->securityHandshake   = 0;
 
     ssl_GetRecvBufLock(ss);
     status = ssl_InitGather(&ss->gs);
     ssl_ReleaseRecvBufLock(ss);
 
     ssl_GetSSL3HandshakeLock(ss);
+    ss->ssl3.hs.canFalseStart = PR_FALSE;
+    ss->ssl3.hs.restartTarget = NULL;
 
     /*
     ** Blow away old security state and get a fresh setup.
     */
     ssl_GetXmitBufLock(ss); 
     ssl_ResetSecurityInfo(&ss->sec, PR_TRUE);
     status = ssl_CreateSecurityInfo(ss);
     ssl_ReleaseXmitBufLock(ss); 
 
     ssl_ReleaseSSL3HandshakeLock(ss);
@@ skipping 90 matching lines @@
 
     ss->handshakeCallback     = cb;
     ss->handshakeCallbackData = client_data;
 
     ssl_ReleaseSSL3HandshakeLock(ss);
     ssl_Release1stHandshakeLock(ss);
 
     return SECSuccess;
 }
 
+/* Register an application callback to be called when false start may happen.
+** Acquires and releases HandshakeLock.
+*/
+SECStatus
+SSL_SetCanFalseStartCallback(PRFileDesc *fd, SSLCanFalseStartCallback cb,
+                             void *client_data)
+{
+    sslSocket *ss;
+
+    ss = ssl_FindSocket(fd);
+    if (!ss) {
+        SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetCanFalseStartCallback",
+                 SSL_GETPID(), fd));
+        return SECFailure;
+    }
+
+    if (!ss->opt.useSecurity) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
+    ssl_Get1stHandshakeLock(ss);
+    ssl_GetSSL3HandshakeLock(ss);
+
+    ss->canFalseStartCallback     = cb;
+    ss->canFalseStartCallbackData = client_data;
+
+    ssl_ReleaseSSL3HandshakeLock(ss);
+    ssl_Release1stHandshakeLock(ss);
+
+    return SECSuccess;
+}
+
+SECStatus
+SSL_RecommendedCanFalseStart(PRFileDesc *fd, PRBool *canFalseStart)
+{
+    sslSocket *ss;
+
+    *canFalseStart = PR_FALSE;
+    ss = ssl_FindSocket(fd);
+    if (!ss) {
+        SSL_DBG(("%d: SSL[%d]: bad socket in SSL_DefaultCanFalseStart",

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

SSL_DefaultCanFalseStart => SSL_RecommendedCanFalseStart

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

Done.

+                 SSL_GETPID(), fd));
+        return SECFailure;
+    }
+
+    if (!ss->ssl3.initialized) {
+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
+        return SECFailure;
+    }
+
+    if (ss->version < SSL_LIBRARY_VERSION_3_0) {
+        PORT_SetError(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2);
+        return SECFailure;
+    }
+
+    /* Require a forward-secret key exchange. */
+    *canFalseStart = 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;
+
+    return SECSuccess;
+}
+
 /* Try to make progress on an SSL handshake by attempting to read the 
 ** next handshake from the peer, and sending any responses.
 ** For non-blocking sockets, returns PR_ERROR_WOULD_BLOCK  if it cannot 
 ** read the next handshake from the underlying socket.
 ** For SSLv2, returns when handshake is complete or fatal error occurs.
 ** For SSLv3, returns when handshake is complete, or application data has
 ** arrived that must be taken by application before handshake can continue, 
 ** or a fatal error occurs.
 ** Application should use handshake completion callback to tell which. 
 */
@@ skipping 173 matching lines @@
 ** This code is similar to, and easily confused with, 
 **   ssl_GatherRecord1stHandshake() in sslcon.c
 */
 static int 
 DoRecv(sslSocket *ss, unsigned char *out, int len, int flags)
 {
     int              rv;
     int              amount;
     int              available;
 
+    ssl_Get1stHandshakeLock(ss);

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

Why does DoRecv() need to get the 1stHandshakeLock?

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

It calls ssl3_GatherAppDataRecord and ssl3_GatherAppDataRecord requires the
1stHandshakeLock to be held. Otherwise selfserv fails in the false start case in
the sslauth.txt tests.

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

I'm sorry that I still don't understand this. Could you provide more detail,
such as the call stack of the selfserv fails (I assume it's a lock assertion
failure)?

I traced ssl3_GatherAppDataRecord -> ssl3_GatherCompleteHandshake but couldn't
find where they require the 1stHandshakeLock to be held. Perhaps you were
referring to some SSL_XXX function called from the handshakeCallback?

In any case, we should at least add a comment to explain why we need to get the
1stHandshakeLock and outline a possible solution.
(Perhaps only call ssl_Get1stHandshakeLock if ss->firstHsDone is false?)

     ssl_GetRecvBufLock(ss);
 
     available = ss->gs.writeOffset - ss->gs.readOffset;
     if (available == 0) {
         /* Get some more data */
         if (ss->version >= SSL_LIBRARY_VERSION_3_0) {
             /* Wait for application data to arrive.  */
             rv = ssl3_GatherAppDataRecord(ss, 0);
         } else {
             /* See if we have a complete record */
@@ skipping 46 matching lines @@
     }
     PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset);
     rv = amount;
 
     SSL_TRC(30, ("%d: SSL[%d]: amount=%d available=%d",
                  SSL_GETPID(), ss->fd, amount, available));
     PRINT_BUF(4, (ss, "DoRecv receiving plaintext:", out, amount));
 
 done:
     ssl_ReleaseRecvBufLock(ss);
+    ssl_Release1stHandshakeLock(ss);
     return rv;
 }
 
 /************************************************************************/
 
 /*
 ** Return SSLKEAType derived from cert's Public Key algorithm info.
 */
 SSLKEAType
 NSS_FindCertKEAType(CERTCertificate * cert)
@@ skipping 546 matching lines @@
 int
 ssl_SecureRead(sslSocket *ss, unsigned char *buf, int len)
 {
     return ssl_SecureRecv(ss, buf, len, 0);
 }
 
 /* Caller holds the SSL Socket's write lock. SSL_LOCK_WRITER(ss) */
 int
 ssl_SecureSend(sslSocket *ss, const unsigned char *buf, int len, int flags)
 {
-    int              rv         = 0;
+    int rv = 0;
+    PRBool falseStart = PR_FALSE;
 
     SSL_TRC(2, ("%d: SSL[%d]: SecureSend: sending %d bytes",
                 SSL_GETPID(), ss->fd, len));
 
     if (ss->shutdownHow & ssl_SHUTDOWN_SEND) {
         PORT_SetError(PR_SOCKET_SHUTDOWN_ERROR);
         rv = PR_FAILURE;
         goto done;
     }
     if (flags) {
@@ skipping 14 matching lines @@
     }
     ssl_ReleaseXmitBufLock(ss);
     if (rv < 0) {
         goto done;
     }
 
     if (len > 0) 
         ss->writerThread = PR_GetCurrentThread();
     /* If any of these is non-zero, the initial handshake is not done. */
     if (!ss->firstHsDone) {
-        PRBool canFalseStart = PR_FALSE;
         ssl_Get1stHandshakeLock(ss);
-        if (ss->version >= SSL_LIBRARY_VERSION_3_0) {
+        if (ss->opt.enableFalseStart &&

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

Why do you check ss->opt.enableFalseStart?

I guess ss->opt.enableFalseStart can be checked without getting the
SSL3HandshakeLock, so you can check it first as an optimization?

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

Yes

+            ss->version >= SSL_LIBRARY_VERSION_3_0) {
             ssl_GetSSL3HandshakeLock(ss);
-            if ((ss->ssl3.hs.ws == wait_change_cipher ||
-                ss->ssl3.hs.ws == wait_finished ||
-                ss->ssl3.hs.ws == wait_new_session_ticket) &&
-                ssl3_CanFalseStart(ss)) {
-                canFalseStart = PR_TRUE;
-            }
+            falseStart = ss->ssl3.hs.canFalseStart;
             ssl_ReleaseSSL3HandshakeLock(ss);
         }
-        if (!canFalseStart &&
+        if (!falseStart &&
             (ss->handshake || ss->nextHandshake || ss->securityHandshake)) {

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

Hmm... If we false started, ss->handshake etc. should all be NULL, so we don't
need to check ss->ssl3.hs.canFalseStart, right?

             rv = ssl_Do1stHandshake(ss);
         }
         ssl_Release1stHandshakeLock(ss);
     }
     if (rv < 0) {
         ss->writerThread = NULL;
         goto done;
     }
 
     /* Check for zero length writes after we do housekeeping so we make forward
      * progress.
      */
     if (len == 0) {
         rv = 0;
         goto done;
     }
     PORT_Assert(buf != NULL);
     if (!buf) {
         PORT_SetError(PR_INVALID_ARGUMENT_ERROR);
         rv = PR_FAILURE;
         goto done;
     }
 
+    if (ssl_trace >= 2 &&
+        !ss->firstHsDone && !falseStart &&
+        ss->opt.enableFalseStart && ss->version >= SSL_LIBRARY_VERSION_3_0) {
+        /* We need to check ss->ssl3.hs.canFalseStart again in case we called
+         * ssl_Do1stHandshake, because ssl_Do1stHandshake might have changed
+         * it.
+         */
+        ssl_GetSSL3HandshakeLock(ss);
+        falseStart = ss->ssl3.hs.canFalseStart;
+        ssl_ReleaseSSL3HandshakeLock(ss);

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

falseStart is set here but never used again. So lines 1307-1313 can all be
deleted. Perhaps this code is incomplete?

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

You are right that this complicated code can be deleted. I had originally not
realized that this code can only be reached in the case that !ss->firstHsDone if
we are false starting, so I had made things more complicated than necessary. I
didn't simplify the code enough when I cleaned it up.

+
+        SSL_TRC(2, ("%d: SSL[%d]: SecureSend: sending data due to false start",
+                    SSL_GETPID(), ss->fd));
+    }
+
     /* Send out the data using one of these functions:
      *  ssl2_SendClear, ssl2_SendStream, ssl2_SendBlock, 
      *  ssl3_SendApplicationData
      */
     ssl_GetXmitBufLock(ss);
     rv = (*ss->sec.send)(ss, buf, len, flags);
     ssl_ReleaseXmitBufLock(ss);
     ss->writerThread = NULL;
 done:
     if (rv < 0) {
@@ skipping 348 matching lines @@
     if (!ss) {
         SSL_DBG(("%d: SSL[%d]: bad socket in SNISocketConfigHook",
                  SSL_GETPID(), fd));
         return SECFailure;
     }
 
     ss->sniSocketConfig = func;
     ss->sniSocketConfigArg = arg;
     return SECSuccess;
 }