Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c	(revision 227672)
+++ net/third_party/nss/ssl/ssl3con.c	(working copy)
@@ -2890,7 +2890,7 @@
     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) );
 
@@ -7344,36 +7344,47 @@
     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);
@@ -7463,6 +7474,9 @@
     }
     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;
     }
@@ -7500,20 +7514,59 @@
 	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. */
@@ -7526,11 +7579,6 @@
     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:
@@ -10147,13 +10195,6 @@
 
 	    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) {
@@ -10278,6 +10319,12 @@
     } 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
@@ -10287,7 +10334,30 @@
 	    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:
@@ -10913,9 +10983,6 @@
         return rv;
     }
 
-    ss->gs.writeOffset = 0;
-    ss->gs.readOffset  = 0;
-
     if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa) {
 	effectiveExchKeyType = kt_rsa;
     } else {
@@ -10980,6 +11047,9 @@
     return rv;
 }
 
+/* The return type is SECStatus instead of void because this function needs
+ * to have type sslRestartTarget.
+ */
 SECStatus
 ssl3_FinishHandshake(sslSocket * ss)
 {
@@ -10989,19 +11059,16 @@
 
     /* 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;
 }
@@ -11569,11 +11636,10 @@
     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. */
     	}
@@ -11933,7 +11999,6 @@
     /* 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.
@@ -11964,9 +12029,7 @@
 	break;
     }
 
-    ssl_ReleaseSSL3HandshakeLock(ss);
     return rv;
-
 }
 
 /*