Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/sslsecur.c

Index: net/third_party/nss/ssl/sslsecur.c
===================================================================
--- net/third_party/nss/ssl/sslsecur.c	(revision 227672)
+++ net/third_party/nss/ssl/sslsecur.c	(working copy)
@@ -97,23 +97,13 @@
 	    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_FinishHandshake(ss);
+		ssl_ReleaseRecvBufLock(ss);
 	    }
-	    ss->firstHsDone         = PR_TRUE;
-	    ss->gs.writeOffset = 0;
-	    ss->gs.readOffset  = 0;
 	    break;
 	}
 	rv = (*ss->handshake)(ss);
@@ -134,6 +124,24 @@
     return rv;
 }
 
+void
+ssl_FinishHandshake(sslSocket *ss)
+{
+    PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );
+    PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
+
+    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.
@@ -206,6 +214,7 @@
     ssl_Get1stHandshakeLock(ss);
 
     ss->firstHsDone = PR_FALSE;
+    ss->enoughFirstHsDone = PR_FALSE;
     if ( asServer ) {
 	ss->handshake = ssl2_BeginServerHandshake;
 	ss->handshaking = sslHandshakingAsServer;
@@ -221,6 +230,8 @@
     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.
@@ -331,6 +342,71 @@
     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)

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

Nit: client_data => arg

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

Done.

+{
+    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_RecommendedCanFalseStart",
+		 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 
@@ -524,6 +600,7 @@
     int              amount;
     int              available;
 
+    ssl_Get1stHandshakeLock(ss);

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

Please add a comment to explain why we need to get 1stHandshakeLock.
It is only needed for the false start case, when we need to finish the
first handshake inside DoRecv().

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

Done.

     ssl_GetRecvBufLock(ss);
 
     available = ss->gs.writeOffset - ss->gs.readOffset;
@@ -590,6 +667,7 @@
 
 done:
     ssl_ReleaseRecvBufLock(ss);
+    ssl_Release1stHandshakeLock(ss);
     return rv;
 }
 
@@ -1156,7 +1234,8 @@
 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));
@@ -1191,19 +1270,14 @@
     	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 &&
+	    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 &&

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

As I mentioned before, I believe this !falseStart check is not necessary
because the ss->handshake, ss->nextHandshake, ss->securityHandshake pointers
should all be null in the false start case. Otherwise, ssl_SecureRecv would
not finish the handshake inside DoRecv().

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

With the current state of the code, I am almost 100% sure that the canFalseStart
variable is not needed at all because it is redundant with
ss->handshake/nextHandshake/securityHandshake like you mention. I noticed that
last week, and I spent a little time experimenting with removing the
canFalseStart variable.

However, I would very much like to remove the SSL 2.0 code very soon, and when
SSL 2.0 is removed, ss->handshake/nextHandshake/securityHandshake should be
removed. Testing the canFalseStart variable is much clearer than testing
ss->handshake. So, I would like to resolve the temporary redundancy during the
removal of SSL 2.0.

[wtc, 2013/10/30 01:01:20]

Hmm... did you mean ss->ssl3.hs.canFalseStart is not needed because it is
equivalent to the following condition?

  !ss->firstHsDone && !ss->handshake && !ss->nextHandshake &&
!ss->securityHandshake

[briansmith, 2013/11/01 09:55:55]

&& ssl3_WaitingForStartOfServerSecondRound(ss).

Yes, I think that is correct.

 	    (ss->handshake || ss->nextHandshake || ss->securityHandshake)) {
 	    rv = ssl_Do1stHandshake(ss);
 	}
@@ -1228,6 +1302,17 @@
 	goto done;
     }
 
+    if (!ss->firstHsDone) {
+	PORT_Assert(ss->version >= SSL_LIBRARY_VERSION_3_0);
+#ifdef DEBUG
+	ssl_GetSSL3HandshakeLock(ss);
+	PORT_Assert(ss->ssl3.hs.canFalseStart);
+	ssl_ReleaseSSL3HandshakeLock(ss);
+#endif
+	SSL_TRC(3, ("%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