# HG changeset patch
# User Martin Thomson <martin.thomson@gmail.com>
# Date 1535458477 -7200
# Tue Aug 28 14:14:37 2018 +0200
# Branch NSS_3_36_BRANCH
# Node ID 14bfa8390396e18ba5b35c7fb299a2c2023f6448
# Parent 42bc6956fda39f6afe81b8de7afb542f3216bc7e
Bug 1483128 - Move random generation, r?ekr
Summary: This is the simpler fix. It's making the bug pretty obvious though.
Reviewers: ekr, kaie
Subscribers: HubertKario, mt, ekr, beurdouche, kaie, jcj, ueno, wtc, rrelyea
Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3
Bug #: 1483128
Differential Revision: https://phabricator.services.mozilla.com/D4282
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -8082,14 +8082,6 @@ ssl3_HandleClientHello(sslSocket *ss, PR
}
}
- /* Generate the Server Random now so it is available
- * when we process the ClientKeyShare in TLS 1.3 */
- rv = ssl3_GetNewRandom(ss->ssl3.hs.server_random);
- if (rv != SECSuccess) {
- errCode = SSL_ERROR_GENERATE_RANDOM_FAILURE;
- goto loser;
- }
-
#ifndef TLS_1_3_DRAFT_VERSION
/*
* [draft-ietf-tls-tls13-11 Section 6.3.1.1].
@@ -8878,6 +8870,7 @@ ssl_ConstructServerHello(sslSocket *ss,
SECStatus rv;
SSL3ProtocolVersion version;
sslSessionID *sid = ss->sec.ci.sid;
+ const PRUint8 *random;
if (IS_DTLS(ss) && ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
version = dtls_TLSVersionToDTLSVersion(ss->version);
@@ -8889,9 +8882,17 @@ ssl_ConstructServerHello(sslSocket *ss,
if (rv != SECSuccess) {
return SECFailure;
}
- /* Random already generated in ssl3_HandleClientHello */
- rv = sslBuffer_Append(messageBuf, helloRetry ? ssl_hello_retry_random : ss->ssl3.hs.server_random,
- SSL3_RANDOM_LENGTH);
+
+ if (helloRetry) {
+ random = ssl_hello_retry_random;
+ } else {
+ rv = ssl3_GetNewRandom(ss->ssl3.hs.server_random);
+ if (rv != SECSuccess) {
+ return SECFailure;
+ }
+ random = ss->ssl3.hs.server_random;
+ }
+ rv = sslBuffer_Append(messageBuf, random, SSL3_RANDOM_LENGTH);
if (rv != SECSuccess) {
return SECFailure;
}
# HG changeset patch
# User Martin Thomson <martin.thomson@gmail.com>
# Date 1535458545 -7200
# Tue Aug 28 14:15:45 2018 +0200
# Node ID eee3954f57355ad04bc32f1c2dfe25d7e13a3382
# Parent 4c7ffcfd43f613eb08ee7b4a75dbeb1a7fb540ce
Bug 1483128 - Test that randoms aren't fixed, r?ekr
Summary:
We can't easily test that ClientHello.random and ServerHello.random are truly
random in these tests, but we can catch mistakes the likes of which produced
this bug. This just runs a few handshakes and tests that none of the random
values are equal to any other, or they are equal to zero.
Reviewers: ekr
Subscribers: mt, ekr, beurdouche, kaie, jcj, ueno, rrelyea, wtc, HubertKario
Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3
Bug #: 1483128
Differential Revision: https://phabricator.services.mozilla.com/D4413
diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -541,6 +541,47 @@ TEST_F(TlsConnectTest, OneNRecordSplitti
EXPECT_EQ(ExpectedCbcLen(20), records->record(2).buffer.len());
}
+// We can't test for randomness easily here, but we can test that we don't
+// produce a zero value, or produce the same value twice. There are 5 values
+// here: two ClientHello.random, two ServerHello.random, and one zero value.
+// Matrix them and fail if any are the same.
+TEST_P(TlsConnectGeneric, CheckRandoms) {
+ ConfigureSessionCache(RESUME_NONE, RESUME_NONE);
+
+ static const size_t random_len = 32;
+ uint8_t crandom1[random_len], srandom1[random_len];
+ uint8_t z[random_len] = {0};
+
+ auto ch = MakeTlsFilter<TlsHandshakeRecorder>(client_, ssl_hs_client_hello);
+ auto sh = MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_server_hello);
+ Connect();
+ ASSERT_TRUE(ch->buffer().len() > (random_len + 2));
+ ASSERT_TRUE(sh->buffer().len() > (random_len + 2));
+ memcpy(crandom1, ch->buffer().data() + 2, random_len);
+ memcpy(srandom1, sh->buffer().data() + 2, random_len);
+ EXPECT_NE(0, memcmp(crandom1, srandom1, random_len));
+ EXPECT_NE(0, memcmp(crandom1, z, random_len));
+ EXPECT_NE(0, memcmp(srandom1, z, random_len));
+
+ Reset();
+ ch = MakeTlsFilter<TlsHandshakeRecorder>(client_, ssl_hs_client_hello);
+ sh = MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_server_hello);
+ Connect();
+ ASSERT_TRUE(ch->buffer().len() > (random_len + 2));
+ ASSERT_TRUE(sh->buffer().len() > (random_len + 2));
+ const uint8_t* crandom2 = ch->buffer().data() + 2;
+ const uint8_t* srandom2 = sh->buffer().data() + 2;
+
+ EXPECT_NE(0, memcmp(crandom2, srandom2, random_len));
+ EXPECT_NE(0, memcmp(crandom2, z, random_len));
+ EXPECT_NE(0, memcmp(srandom2, z, random_len));
+
+ EXPECT_NE(0, memcmp(crandom1, crandom2, random_len));
+ EXPECT_NE(0, memcmp(crandom1, srandom2, random_len));
+ EXPECT_NE(0, memcmp(srandom1, crandom2, random_len));
+ EXPECT_NE(0, memcmp(srandom1, srandom2, random_len));
+}
+
INSTANTIATE_TEST_CASE_P(
GenericStream, TlsConnectGeneric,
::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
diff --git a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
--- a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
+++ b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
@@ -350,6 +350,30 @@ TEST_P(SSLv2ClientHelloTest, RequireSafe
Connect();
}
+TEST_P(SSLv2ClientHelloTest, CheckServerRandom) {
+ ConfigureSessionCache(RESUME_NONE, RESUME_NONE);
+ SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA);
+
+ static const size_t random_len = 32;
+ uint8_t srandom1[random_len];
+ uint8_t z[random_len] = {0};
+
+ auto sh = MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_server_hello);
+ Connect();
+ ASSERT_TRUE(sh->buffer().len() > (random_len + 2));
+ memcpy(srandom1, sh->buffer().data() + 2, random_len);
+ EXPECT_NE(0, memcmp(srandom1, z, random_len));
+
+ Reset();
+ sh = MakeTlsFilter<TlsHandshakeRecorder>(server_, ssl_hs_server_hello);
+ Connect();
+ ASSERT_TRUE(sh->buffer().len() > (random_len + 2));
+ const uint8_t* srandom2 = sh->buffer().data() + 2;
+
+ EXPECT_NE(0, memcmp(srandom2, z, random_len));
+ EXPECT_NE(0, memcmp(srandom1, srandom2, random_len));
+}
+
// Connect to the server with TLS 1.1, signalling that this is a fallback from
// a higher version. As the server doesn't support anything higher than TLS 1.1
// it must accept the connection.