Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1513586 - Set downgrade sentinel for client TLS versions lower th…
…an 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
  • Loading branch information
Kevin Jacobs committed Jan 2, 2020
1 parent 5d110bb commit 305539f
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 24 deletions.
61 changes: 61 additions & 0 deletions gtests/ssl_gtest/ssl_version_unittest.cc
Expand Up @@ -102,6 +102,61 @@ TEST_F(TlsConnectTest, TestDisableDowngradeDetection) {
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) {
Expand Down Expand Up @@ -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
52 changes: 28 additions & 24 deletions lib/ssl/ssl3con.c
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 305539f

Please sign in to comment.