From 305539fe9805c4504cb0d66089fca1a5450aa7b4 Mon Sep 17 00:00:00 2001 From: Kevin Jacobs Date: Thu, 2 Jan 2020 20:46:26 +0000 Subject: [PATCH] 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 --HG-- extra : moz-landing-system : lando --- gtests/ssl_gtest/ssl_version_unittest.cc | 61 ++++++++++++++++++++++++ lib/ssl/ssl3con.c | 52 ++++++++++---------- 2 files changed, 89 insertions(+), 24 deletions(-) diff --git a/gtests/ssl_gtest/ssl_version_unittest.cc b/gtests/ssl_gtest/ssl_version_unittest.cc index 3255bd512c..699edc64f5 100644 --- a/gtests/ssl_gtest/ssl_version_unittest.cc +++ b/gtests/ssl_gtest/ssl_version_unittest.cc @@ -102,6 +102,61 @@ TEST_F(TlsConnectTest, TestDisableDowngradeDetection) { server_->CheckErrorCode(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE); } +typedef std::tuple // server version + TlsDowngradeProfile; + +class TlsDowngradeTest + : public TlsConnectTestBase, + public ::testing::WithParamInterface { + 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(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) { @@ -280,4 +335,10 @@ TEST_F(TlsConnectStreamTls13, Ssl30ClientHelloWithSupportedVersions) { ConnectExpectAlert(server_, kTlsAlertProtocolVersion); } +INSTANTIATE_TEST_CASE_P( + TlsDowngradeSentinelTest, TlsDowngradeTest, + ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream, + TlsConnectTestBase::kTlsVAll, + TlsConnectTestBase::kTlsV12Plus)); + } // namespace nss_test diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index f3c723bbc2..60b247fd7c 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -394,12 +394,12 @@ static const SSLCipher2Mech alg2Mech[] = { { ssl_calg_chacha20, CKM_NSS_CHACHA20_POLY1305 }, }; -const PRUint8 tls13_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)); + 0x47, 0x52, 0x44, 0x01 }; +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 @@ -6713,13 +6713,13 @@ ssl_CheckServerRandom(sslSocket *ss) /* 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)) || - !PORT_Memcmp(downgrade_sentinel, tls12_downgrade_random, - sizeof(tls12_downgrade_random))) { + sizeof(tls12_downgrade_random)) || + !PORT_Memcmp(downgrade_sentinel, + tls1_downgrade_random, + sizeof(tls1_downgrade_random))) { return SECFailure; } } @@ -8491,20 +8491,24 @@ ssl_GenerateServerRandom(sslSocket *ss) */ 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;