Make SSL False Start work with asynchronous certificate validation

net/third_party/nss/ssl/ssl3gthr.c

 /*
  * Gather (Read) entire SSL3 records from socket into buffer.  
  *
  * 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 "ssl.h"
 #include "sslimpl.h"
@@ skipping 257 matching lines @@
  *    and from SSL_ForceHandshake in sslsecur.c
  *    and from ssl3_GatherAppDataRecord below (<- DoRecv in sslsecur.c).
  *
  * Caller must hold the recv buf lock.
  */
 int
 ssl3_GatherCompleteHandshake(sslSocket *ss, int flags)
 {
     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
          * when ssl3_HandleHandshake returns SECWouldBlock, it leaves
          * behind a non-NULL but zero-length msgState).
          * Test: async_cert_restart_server_sends_hello_request_first_in_separate_record
          */
         if (ss->ssl3.hs.msgState.buf) {
             if (ss->ssl3.hs.msgState.len == 0) {
                 ss->ssl3.hs.msgState.buf = NULL;
             } else {
                 handleRecordNow = PR_TRUE;
             }
         }
 
-        ssl_ReleaseSSL3HandshakeLock(ss);
-
         if (handleRecordNow) {
             /* ssl3_HandleHandshake previously returned SECWouldBlock and the
              * as-yet-unprocessed plaintext of that previous handshake record.
              * We need to process it now before we overwrite it with the next
              * handshake record.
              */
             rv = ssl3_HandleRecord(ss, NULL, &ss->gs.buf);
         } else {
             /* bring in the next sslv3 record. */
             if (!IS_DTLS(ss)) {
                 rv = ssl3_GatherData(ss, &ss->gs, flags);
             } else {
                 rv = dtls_GatherData(ss, &ss->gs, flags);

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

I agree we should be holding the SSL3HandshakeLock most of the time in this
function, at least whenever it accesses ss->ssl3.hs. But when this function
calls out to other functions, we may need to release the SSL3HandshakeLock.

A good example is here. We need to read more data from the underlying TCP or UDP
socket. If the socket is in blocking mode, ssl3_GatherData and dtls_GatherData
may block indefinitely. It is bad to hold a lock for a long time.

So, please try to release the lock before line 311 and re-acquire the lock
before line 373. You may need to change to goto statements back to return
statements in this block of code.

                 
                 /* If we got a would block error, that means that no data was
                  * available, so we check the timer to see if it's time to
                  * 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.
              * If it's application data, ss->gs.buf will not be empty upon return.
              * If it's a change cipher spec, alert, or handshake message,
              * ss->gs.buf.len will be 0 when ssl3_HandleRecord returns SECSuccess.
              */
             cText.type    = (SSL3ContentType)ss->gs.hdr[0];
             cText.version = (ss->gs.hdr[1] << 8) | ss->gs.hdr[2];
 
             if (IS_DTLS(ss)) {
                 int i;
 
                 cText.version = dtls_DTLSVersionToTLSVersion(cText.version);
                 /* DTLS sequence number */
                 cText.seq_num.high = 0; cText.seq_num.low = 0;
                 for (i = 0; i < 4; i++) {
                     cText.seq_num.high <<= 8; cText.seq_num.low <<= 8;
                     cText.seq_num.high |= ss->gs.hdr[3 + i];
                     cText.seq_num.low |= ss->gs.hdr[7 + i];
                 }
             }
 
             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;
-        }
-
-        /* 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);
+            rv = ss->recvdCloseNotify ? 0 : rv;
+            goto done;
         }
     } 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.
  * Repeat this until some application data is received.
  *
  * Returns  1 when application data is available.
  * Returns  0 if ssl3_GatherData hits EOF.
  * Returns -1 on read error, or PR_WOULD_BLOCK_ERROR, or handleRecord error.
  * Returns -2 on SECWouldBlock return from ssl3_HandleRecord.
  *
  * Called from DoRecv in sslsecur.c
  * Caller must hold the recv buf lock.
  */
 int
 ssl3_GatherAppDataRecord(sslSocket *ss, int flags)
 {
     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);
 
     return rv;
 }