Bug 1483416 - Disable false start if there might be a downgrade, r=ekr
authorMartin Thomson <martin.thomson@gmail.com>
Wed, 15 Aug 2018 10:24:18 +1000
changeset 14456 91b6fb509cb01bbf4466f647cccfa2a2e060c504
parent 14455 ee357b00f2e6c44589dcd406097357888d59ef06
child 14457 2ed9f6afd84eb7fd1033ffaf448655e1d5a79bb8
push id3169
push usermartin.thomson@gmail.com
push dateSun, 26 Aug 2018 23:51:29 +0000
reviewersekr
bugs1483416
Bug 1483416 - Disable false start if there might be a downgrade, r=ekr
gtests/ssl_gtest/ssl_version_unittest.cc
lib/ssl/ssl3con.c
--- a/gtests/ssl_gtest/ssl_version_unittest.cc
+++ b/gtests/ssl_gtest/ssl_version_unittest.cc
@@ -121,16 +121,44 @@ TEST_F(TlsConnectTest, TestFallbackFromT
                            SSL_LIBRARY_VERSION_TLS_1_1);
   server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
                            SSL_LIBRARY_VERSION_TLS_1_2);
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
   client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
   server_->CheckErrorCode(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
 }
 
+static SECStatus AllowFalseStart(PRFileDesc* fd, void* arg,
+                                 PRBool* can_false_start) {
+  bool* false_start_attempted = reinterpret_cast<bool*>(arg);
+  *false_start_attempted = true;
+  *can_false_start = PR_TRUE;
+  return SECSuccess;
+}
+
+// If we disable the downgrade check, the sentinel is still generated, and we
+// disable false start instead.
+TEST_F(TlsConnectTest, DisableFalseStartOnFallback) {
+  // Don't call client_->EnableFalseStart(), because that sets the client up for
+  // success, and we want false start to fail.
+  client_->SetOption(SSL_ENABLE_FALSE_START, PR_TRUE);
+  bool false_start_attempted = false;
+  EXPECT_EQ(SECSuccess,
+            SSL_SetCanFalseStartCallback(client_->ssl_fd(), AllowFalseStart,
+                                         &false_start_attempted));
+
+  client_->SetDowngradeCheckVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_2);
+  server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+  EXPECT_FALSE(false_start_attempted);
+}
+
 TEST_F(TlsConnectTest, TestFallbackFromTls13) {
   client_->SetOption(SSL_ENABLE_HELLO_DOWNGRADE_CHECK, PR_TRUE);
   client_->SetDowngradeCheckVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
                            SSL_LIBRARY_VERSION_TLS_1_2);
   server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
                            SSL_LIBRARY_VERSION_TLS_1_3);
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6393,16 +6393,51 @@ ssl_CheckServerSessionIdCorrectness(sslS
     if (sentRealSid || sentFakeSid) {
         return sidMatch;
     }
 
     /* TLS 1.3: The server shouldn't send a session ID. */
     return sidBytes->len == 0;
 }
 
+static SECStatus
+ssl_CheckServerRandom(sslSocket *ss)
+{
+    /* Check the ServerHello.random per [RFC 8446 Section 4.1.3].
+     *
+     * TLS 1.3 clients receiving a ServerHello indicating TLS 1.2 or below
+     * MUST check that the last 8 bytes are not equal to either of these
+     * values.  TLS 1.2 clients SHOULD also check that the last 8 bytes are
+     * not equal to the second value if the ServerHello indicates TLS 1.1 or
+     * below.  If a match is found, the client MUST abort the handshake with
+     * an "illegal_parameter" alert.
+     */
+    SSL3ProtocolVersion checkVersion =
+        ss->ssl3.downgradeCheckVersion ? ss->ssl3.downgradeCheckVersion
+                                       : ss->vrange.max;
+
+    if (checkVersion >= SSL_LIBRARY_VERSION_TLS_1_2 &&
+        checkVersion > ss->version) {
+        /* Both sections use the same sentinel region. */
+        PRUint8 *downgrade_sentinel =
+            ss->ssl3.hs.server_random +
+            SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
+        if (!PORT_Memcmp(downgrade_sentinel,
+                         tls13_downgrade_random,
+                         sizeof(tls13_downgrade_random)) ||
+            !PORT_Memcmp(downgrade_sentinel,
+                         tls12_downgrade_random,
+                         sizeof(tls12_downgrade_random))) {
+            return SECFailure;
+        }
+    }
+
+    return SECSuccess;
+}
+
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
  * ssl3 ServerHello message.
  * Caller must hold Handshake and RecvBuf locks.
  */
 static SECStatus
 ssl3_HandleServerHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
 {
     PRUint32 cipher;
@@ -6545,51 +6580,27 @@ ssl3_HandleServerHello(sslSocket *ss, PR
      * us to be getting this version number, but it's what we have.
      * (1294697). */
     if (ss->firstHsDone && (ss->version != ss->ssl3.crSpec->version)) {
         desc = protocol_version;
         errCode = SSL_ERROR_UNSUPPORTED_VERSION;
         goto alert_loser;
     }
 
-    /* Disable this check for DTLS while we're on draft versions. */
     if (ss->opt.enableHelloDowngradeCheck
 #ifdef DTLS_1_3_DRAFT_VERSION
+        /* Disable this check while we are on draft DTLS 1.3 versions. */
         && !IS_DTLS(ss)
 #endif
             ) {
-        /* Check the ServerHello.random per [RFC 8446 Section 4.1.3].
-         *
-         * TLS 1.3 clients receiving a ServerHello indicating TLS 1.2 or below
-         * MUST check that the last 8 bytes are not equal to either of these
-         * values.  TLS 1.2 clients SHOULD also check that the last 8 bytes are
-         * not equal to the second value if the ServerHello indicates TLS 1.1 or
-         * below.  If a match is found, the client MUST abort the handshake with
-         * an "illegal_parameter" alert.
-         */
-        SSL3ProtocolVersion checkVersion =
-            ss->ssl3.downgradeCheckVersion ? ss->ssl3.downgradeCheckVersion
-                                           : ss->vrange.max;
-
-        if (checkVersion >= SSL_LIBRARY_VERSION_TLS_1_2 &&
-            checkVersion > ss->version) {
-            /* Both sections use the same sentinel region. */
-            PRUint8 *downgrade_sentinel =
-                ss->ssl3.hs.server_random +
-                SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
-            if (!PORT_Memcmp(downgrade_sentinel,
-                             tls13_downgrade_random,
-                             sizeof(tls13_downgrade_random)) ||
-                !PORT_Memcmp(downgrade_sentinel,
-                             tls12_downgrade_random,
-                             sizeof(tls12_downgrade_random))) {
-                desc = illegal_parameter;
-                errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
-                goto alert_loser;
-            }
+        rv = ssl_CheckServerRandom(ss);
+        if (rv != SECSuccess) {
+            desc = illegal_parameter;
+            errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
+            goto alert_loser;
         }
     }
 
     /* Finally, now all the version-related checks have passed. */
     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
     /* Update the write cipher spec to match the version. But not after
      * HelloRetryRequest, because cwSpec might be a 0-RTT cipher spec,
      * in which case this is a no-op. */
@@ -7422,26 +7433,35 @@ ssl3_CheckFalseStart(sslSocket *ss)
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
     PORT_Assert(!ss->ssl3.hs.authCertificatePending);
     PORT_Assert(!ss->ssl3.hs.canFalseStart);
 
     if (!ss->canFalseStartCallback) {
         SSL_TRC(3, ("%d: SSL[%d]: no false start callback so no false start",
                     SSL_GETPID(), ss->fd));
     } else {
-        PRBool maybeFalseStart;
+        PRBool maybeFalseStart = PR_TRUE;
         SECStatus rv;
 
+        rv = ssl_CheckServerRandom(ss);
+        if (rv != SECSuccess) {
+            SSL_TRC(3, ("%d: SSL[%d]: no false start due to possible downgrade",
+                        SSL_GETPID(), ss->fd));
+            maybeFalseStart = PR_FALSE;
+        }
+
         /* 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->cipherDef->secret_key_size >= 10;
-        ssl_ReleaseSpecReadLock(ss);
+        if (maybeFalseStart) {
+            ssl_GetSpecReadLock(ss);
+            maybeFalseStart = ss->ssl3.cwSpec->cipherDef->secret_key_size >= 10;
+            ssl_ReleaseSpecReadLock(ss);
+        }
 
         if (!maybeFalseStart) {
             SSL_TRC(3, ("%d: SSL[%d]: no false start due to weak cipher",
                         SSL_GETPID(), ss->fd));
         } else {
             PORT_Assert((ss->ssl3.hs.preliminaryInfo & ssl_preinfo_all) ==
                         ssl_preinfo_all);
             rv = (ss->canFalseStartCallback)(ss->fd,