Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/ssl3gthr.c

Index: net/third_party/nss/ssl/ssl3gthr.c
===================================================================
--- net/third_party/nss/ssl/ssl3gthr.c	(revision 227672)
+++ net/third_party/nss/ssl/ssl3gthr.c	(working copy)
@@ -275,24 +275,24 @@
 {
     SSL3Ciphertext cText;
     int            rv;
-    PRBool         canFalseStart = PR_FALSE;
 
     SSL_TRC(30, ("ssl3_GatherCompleteHandshake"));
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
+
+    ssl_GetSSL3HandshakeLock(ss);
+
     do {
 	PRBool handleRecordNow = PR_FALSE;
 
-	ssl_GetSSL3HandshakeLock(ss);
-
 	/* Without this, we may end up wrongly reporting
 	 * SSL_ERROR_RX_UNEXPECTED_* errors if we receive any records from the
 	 * peer while we are waiting to be restarted.
 	 */
 	if (ss->ssl3.hs.restartTarget) {
-	    ssl_ReleaseSSL3HandshakeLock(ss);
 	    PORT_SetError(PR_WOULD_BLOCK_ERROR);
-	    return (int) SECFailure;
+	    rv = (int) SECFailure;
+	    goto done;
 	}
 
 	/* Treat an empty msgState like a NULL msgState. (Most of the time
@@ -308,8 +308,6 @@
 	    }
 	}
 
-	ssl_ReleaseSSL3HandshakeLock(ss);
-
 	if (handleRecordNow) {
 	    /* ssl3_HandleHandshake previously returned SECWouldBlock and the
 	     * as-yet-unprocessed plaintext of that previous handshake record.
@@ -329,16 +327,16 @@
 		 * retransmit */
 		if (rv == SECFailure &&
 		    (PORT_GetError() == PR_WOULD_BLOCK_ERROR)) {
-		    ssl_GetSSL3HandshakeLock(ss);
 		    dtls_CheckTimer(ss);
-		    ssl_ReleaseSSL3HandshakeLock(ss);
 		    /* Restore the error in case something succeeded */
 		    PORT_SetError(PR_WOULD_BLOCK_ERROR);
 		}
 	    }
 
+            PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );

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

Is this assertion necessary? We already checked this on line 281.

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

Done.

+
 	    if (rv <= 0) {
-		return rv;
+		goto done;
 	    }
 
 	    /* decipher it, and handle it if it's a handshake.
@@ -364,28 +362,25 @@
 
 	    cText.buf     = &ss->gs.inbuf;
 	    rv = ssl3_HandleRecord(ss, &cText, &ss->gs.buf);
+
+	    PORT_Assert(rv < 0 || ss->gs.buf.len == 0 ||
+			cText.type == content_application_data);

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

Can you explain this assertion with a comment?

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

Done.

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

Making this clearer, and fixing the locking according to your feedback,
motivated me to rearrange this code this way, to make it clearer.

The conditions for ending the loop are the same as before; they are just
organized in a clearer way.

 	}
 	if (rv < 0) {
-	    return ss->recvdCloseNotify ? 0 : rv;
+	    rv = ss->recvdCloseNotify ? 0 : rv;
+	    goto done;
 	}
-
-	/* If we kicked off a false start in ssl3_HandleServerHelloDone, break
-	 * out of this loop early without finishing the handshake.
-	 */
-	if (ss->opt.enableFalseStart) {
-	    ssl_GetSSL3HandshakeLock(ss);
-	    canFalseStart = (ss->ssl3.hs.ws == wait_change_cipher ||
-			     ss->ssl3.hs.ws == wait_new_session_ticket) &&

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

Note that ss->ssl3.hs.ws == wait_finished is not part of the check. This means
the original code will not set canFalseStart to true if we have already received
the server's ChangeCipherSpec but not Finished.

I am not sure whether this is intentional or an oversight. I suspect it doesn't
matter.

[briansmith, 2013/10/17 01:44:46]

The false start code, before this patch, was inconsistent in this respect. I
agree that it doesn't matter.

[briansmith, 2013/10/24 20:57:04]

I found a reason why we should probably care about this. See the comment in
sslimpl.h for ssl3_WaitingForStartOfServerSecondRound.

-		            ssl3_CanFalseStart(ss);
-	    ssl_ReleaseSSL3HandshakeLock(ss);
-	}
     } while (ss->ssl3.hs.ws != idle_handshake &&
-             !canFalseStart &&
+             !ss->ssl3.hs.canFalseStart &&
              ss->gs.buf.len == 0);
 
     ss->gs.readOffset = 0;
     ss->gs.writeOffset = ss->gs.buf.len;
-    return 1;
+    rv = 1;
+
+done:
+    ssl_ReleaseSSL3HandshakeLock(ss);
+    return rv;
 }
 
 /* Repeatedly gather in a record and when complete, Handle that record.
@@ -405,6 +400,8 @@
     int            rv;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
+    PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );
+
     do {
 	rv = ssl3_GatherCompleteHandshake(ss, flags);
     } while (rv > 0 && ss->gs.buf.len == 0);