Bug 1513586 - Set downgrade sentinel for client TLS versions lower than 1.2. r=mt
authorKevin Jacobs <kjacobs@mozilla.com>
Thu, 02 Jan 2020 20:46:26 +0000
changeset 15445 993717228da05c6bf468ac861f01b6849a94eae3
parent 15444 62d36f2ee1cc7d86939582af1a3440d11c2b726e
child 15446 569ca5b163e7bfac933d154c06f315d5b7d79cbb
child 15447 d41f5350554e1c89e42c52b962e89fd585bee99d
push id3625
push usermthomson@mozilla.com
push dateFri, 03 Jan 2020 05:14:23 +0000
reviewersmt
bugs1513586
Bug 1513586 - Set downgrade sentinel for client TLS versions lower than 1.2. r=mt Per-[[ https://tools.ietf.org/html/rfc8446#section-4.1.3 | RFC 8446 ]], the downgrade sentinel must be set by a TLS 1.3 server (and should be set by a TLS 1.2 server) that negotiates TLS 1.0 or 1.1. This patch corrects the behavior and adds a test. Differential Revision: https://phabricator.services.mozilla.com/D57585
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
@@ -97,16 +97,71 @@ TEST_F(TlsConnectTest, TestDisableDowngr
                            SSL_LIBRARY_VERSION_TLS_1_3);
   server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
                            SSL_LIBRARY_VERSION_TLS_1_3);
   ConnectExpectAlert(server_, kTlsAlertDecryptError);
   client_->CheckErrorCode(SSL_ERROR_DECRYPT_ERROR_ALERT);
   server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE);
 }
 
+typedef std::tuple<SSLProtocolVariant,
+                   uint16_t,  // client version
+                   uint16_t>  // server version
+    TlsDowngradeProfile;
+
+class TlsDowngradeTest
+    : public TlsConnectTestBase,
+      public ::testing::WithParamInterface<TlsDowngradeProfile> {
+ public:
+  TlsDowngradeTest()
+      : TlsConnectTestBase(std::get<0>(GetParam()), std::get<1>(GetParam())),
+        c_ver(std::get<1>(GetParam())),
+        s_ver(std::get<2>(GetParam())) {}
+
+ protected:
+  const uint16_t c_ver;
+  const uint16_t s_ver;
+};
+
+TEST_P(TlsDowngradeTest, TlsDowngradeSentinelTest) {
+  static const uint8_t tls12_downgrade_random[] = {0x44, 0x4F, 0x57, 0x4E,
+                                                   0x47, 0x52, 0x44, 0x01};
+  static const uint8_t tls1_downgrade_random[] = {0x44, 0x4F, 0x57, 0x4E,
+                                                  0x47, 0x52, 0x44, 0x00};
+  static const size_t kRandomLen = 32;
+
+  if (c_ver > s_ver) {
+    return;
+  }
+
+  client_->SetVersionRange(c_ver, c_ver);
+  server_->SetVersionRange(c_ver, s_ver);
+
+  auto sh = MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_server_hello);
+  Connect();
+  ASSERT_TRUE(sh->buffer().len() > (kRandomLen + 2));
+
+  const uint8_t* downgrade_sentinel =
+      sh->buffer().data() + 2 + kRandomLen - sizeof(tls1_downgrade_random);
+  if (c_ver < s_ver) {
+    if (c_ver == SSL_LIBRARY_VERSION_TLS_1_2) {
+      EXPECT_EQ(0, memcmp(downgrade_sentinel, tls12_downgrade_random,
+                          sizeof(tls12_downgrade_random)));
+    } else {
+      EXPECT_EQ(0, memcmp(downgrade_sentinel, tls1_downgrade_random,
+                          sizeof(tls1_downgrade_random)));
+    }
+  } else {
+    EXPECT_NE(0, memcmp(downgrade_sentinel, tls12_downgrade_random,
+                        sizeof(tls12_downgrade_random)));
+    EXPECT_NE(0, memcmp(downgrade_sentinel, tls1_downgrade_random,
+                        sizeof(tls1_downgrade_random)));
+  }
+}
+
 // TLS 1.1 clients do not check the random values, so we should
 // instead get a handshake failure alert from the server.
 TEST_F(TlsConnectTest, TestDowngradeDetectionToTls10) {
   // Setting the option here has no effect.
   client_->SetOption(SSL_ENABLE_HELLO_DOWNGRADE_CHECK, PR_TRUE);
   MakeTlsFilter<TlsClientHelloVersionSetter>(client_,
                                              SSL_LIBRARY_VERSION_TLS_1_0);
   client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0,
@@ -275,9 +330,15 @@ TEST_F(TlsConnectStreamTls13, Tls14Clien
 
 // Offer 1.3 but with ClientHello.legacy_version == SSL 3.0. This
 // causes a protocol version alert.  See RFC 8446 Appendix D.5.
 TEST_F(TlsConnectStreamTls13, Ssl30ClientHelloWithSupportedVersions) {
   MakeTlsFilter<TlsClientHelloVersionSetter>(client_, SSL_LIBRARY_VERSION_3_0);
   ConnectExpectAlert(server_, kTlsAlertProtocolVersion);
 }
 
+INSTANTIATE_TEST_CASE_P(
+    TlsDowngradeSentinelTest, TlsDowngradeTest,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
+                       TlsConnectTestBase::kTlsVAll,
+                       TlsConnectTestBase::kTlsV12Plus));
+
 }  // namespace nss_test
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -389,22 +389,22 @@ static const SSLCipher2Mech alg2Mech[] =
     { ssl_calg_fortezza, CKM_SKIPJACK_CBC64 },
     { ssl_calg_aes, CKM_AES_CBC },
     { ssl_calg_camellia, CKM_CAMELLIA_CBC },
     { ssl_calg_seed, CKM_SEED_CBC },
     { ssl_calg_aes_gcm, CKM_AES_GCM },
     { ssl_calg_chacha20, CKM_NSS_CHACHA20_POLY1305 },
 };
 
-const PRUint8 tls13_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E,
+const PRUint8 tls12_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E,
                                            0x47, 0x52, 0x44, 0x01 };
-const PRUint8 tls12_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E,
-                                           0x47, 0x52, 0x44, 0x00 };
-PR_STATIC_ASSERT(sizeof(tls13_downgrade_random) ==
-                 sizeof(tls13_downgrade_random));
+const PRUint8 tls1_downgrade_random[] = { 0x44, 0x4F, 0x57, 0x4E,
+                                          0x47, 0x52, 0x44, 0x00 };
+PR_STATIC_ASSERT(sizeof(tls12_downgrade_random) ==
+                 sizeof(tls1_downgrade_random));
 
 /* The ECCWrappedKeyInfo structure defines how various pieces of
  * information are laid out within wrappedSymmetricWrappingkey
  * for ECDH key exchange. Since wrappedSymmetricWrappingkey is
  * a 512-byte buffer (see sslimpl.h), the variable length field
  * in ECCWrappedKeyInfo can be at most (512 - 8) = 504 bytes.
  *
  * XXX For now, NSS only supports named elliptic curves of size 571 bits
@@ -6708,23 +6708,23 @@ ssl_CheckServerRandom(sslSocket *ss)
         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);
+            SSL3_RANDOM_LENGTH - sizeof(tls12_downgrade_random);
         if (!PORT_Memcmp(downgrade_sentinel,
-                         tls13_downgrade_random,
-                         sizeof(tls13_downgrade_random)) ||
+                         tls12_downgrade_random,
+                         sizeof(tls12_downgrade_random)) ||
             !PORT_Memcmp(downgrade_sentinel,
-                         tls12_downgrade_random,
-                         sizeof(tls12_downgrade_random))) {
+                         tls1_downgrade_random,
+                         sizeof(tls1_downgrade_random))) {
             return SECFailure;
         }
     }
 
     return SECSuccess;
 }
 
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
@@ -8486,30 +8486,34 @@ ssl_GenerateServerRandom(sslSocket *ss)
      * If negotiating TLS 1.1 or below, TLS 1.3 servers MUST, and TLS 1.2
      * servers SHOULD, set the last 8 bytes of their ServerHello.Random value to
      * the bytes:
      *
      *   44 4F 57 4E 47 52 44 00
      */
     PRUint8 *downgradeSentinel =
         ss->ssl3.hs.server_random +
-        SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
-
-    switch (ss->vrange.max) {
-        case SSL_LIBRARY_VERSION_TLS_1_3:
-            PORT_Memcpy(downgradeSentinel,
-                        tls13_downgrade_random, sizeof(tls13_downgrade_random));
-            break;
-        case SSL_LIBRARY_VERSION_TLS_1_2:
-            PORT_Memcpy(downgradeSentinel,
-                        tls12_downgrade_random, sizeof(tls12_downgrade_random));
-            break;
-        default:
-            /* Do not change random. */
-            break;
+        SSL3_RANDOM_LENGTH - sizeof(tls12_downgrade_random);
+
+    if (ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_2) {
+        switch (ss->version) {
+            case SSL_LIBRARY_VERSION_TLS_1_2:
+                /* vrange.max > 1.2, since we didn't early exit above. */
+                PORT_Memcpy(downgradeSentinel,
+                            tls12_downgrade_random, sizeof(tls12_downgrade_random));
+                break;
+            case SSL_LIBRARY_VERSION_TLS_1_1:
+            case SSL_LIBRARY_VERSION_TLS_1_0:
+                PORT_Memcpy(downgradeSentinel,
+                            tls1_downgrade_random, sizeof(tls1_downgrade_random));
+                break;
+            default:
+                /* Do not change random. */
+                break;
+        }
     }
 
     return SECSuccess;
 }
 
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
  * ssl3 Client Hello message.
  * Caller must hold Handshake and RecvBuf locks.