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_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);
@@ -134,6 +124,23 @@
     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.
@@ -206,6 +213,7 @@
     ssl_Get1stHandshakeLock(ss);
 
     ss->firstHsDone = PR_FALSE;
+    ss->enoughFirstHsDone = PR_FALSE;
     if ( asServer ) {
 	ss->handshake = ssl2_BeginServerHandshake;
 	ss->handshaking = sslHandshakingAsServer;
@@ -221,6 +229,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 +341,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)
+{
+    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 
@@ -524,6 +599,7 @@
     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;
@@ -590,6 +666,7 @@
 
 done:
     ssl_ReleaseRecvBufLock(ss);
+    ssl_Release1stHandshakeLock(ss);
     return rv;
 }
 
@@ -1156,7 +1233,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 +1269,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 &&

[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);
 	}
@@ -1228,6 +1301,21 @@
 	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