From adfae9856f8e4e3aa35729a99be885be273f942a Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Thu, 21 Apr 2022 08:51:23 +0000 Subject: Bug 1166338 - Change SSL_REUSE_SERVER_ECDHE_KEY default to false. r=djackson Differential Revision: https://phabricator.services.mozilla.com/D143514 --- fuzz/tls_server_target.cc | 3 --- gtests/ssl_gtest/ssl_fuzz_unittest.cc | 3 --- gtests/ssl_gtest/ssl_resumption_unittest.cc | 5 ++--- gtests/ssl_gtest/tls_agent.cc | 4 ++-- gtests/ssl_gtest/tls_agent.h | 2 +- gtests/ssl_gtest/tls_connect.cc | 4 ++-- gtests/ssl_gtest/tls_connect.h | 2 +- lib/ssl/ssl.h | 2 +- lib/ssl/sslsock.c | 2 +- 9 files changed, 10 insertions(+), 17 deletions(-) diff --git a/fuzz/tls_server_target.cc b/fuzz/tls_server_target.cc index 41a55541c..900214329 100644 --- a/fuzz/tls_server_target.cc +++ b/fuzz/tls_server_target.cc @@ -47,9 +47,6 @@ static void SetSocketOptions(PRFileDesc* fd, SECStatus rv = SSL_OptionSet(fd, SSL_NO_CACHE, config->EnableCache()); assert(rv == SECSuccess); - rv = SSL_OptionSet(fd, SSL_REUSE_SERVER_ECDHE_KEY, false); - assert(rv == SECSuccess); - rv = SSL_OptionSet(fd, SSL_ENABLE_EXTENDED_MASTER_SECRET, config->EnableExtendedMasterSecret()); assert(rv == SECSuccess); diff --git a/gtests/ssl_gtest/ssl_fuzz_unittest.cc b/gtests/ssl_gtest/ssl_fuzz_unittest.cc index 4718dca17..ef6f7602c 100644 --- a/gtests/ssl_gtest/ssl_fuzz_unittest.cc +++ b/gtests/ssl_gtest/ssl_fuzz_unittest.cc @@ -57,7 +57,6 @@ FUZZ_P(TlsFuzzTest, DeterministicExporter) { Reset(); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); - DisableECDHEServerKeyReuse(); // Reset the RNG state. EXPECT_EQ(SECSuccess, RNG_RandomUpdate(NULL, 0)); @@ -71,7 +70,6 @@ FUZZ_P(TlsFuzzTest, DeterministicExporter) { Reset(); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); - DisableECDHEServerKeyReuse(); // Reset the RNG state. EXPECT_EQ(SECSuccess, RNG_RandomUpdate(NULL, 0)); @@ -97,7 +95,6 @@ FUZZ_P(TlsFuzzTest, DeterministicTranscript) { for (size_t i = 0; i < 5; i++) { Reset(); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); - DisableECDHEServerKeyReuse(); DataBuffer buffer; MakeTlsFilter(client_, buffer); diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc index 53cddfe74..2e23fc096 100644 --- a/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -457,10 +457,10 @@ TEST_P(TlsConnectGeneric, ServerSNICertTypeSwitch) { EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert)); } -// Prior to TLS 1.3, we were not fully ephemeral; though 1.3 fixes that TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) { auto filter = MakeTlsFilter( server_, kTlsHandshakeServerKeyExchange); + EnableECDHEServerKeyReuse(); Connect(); CheckKeys(); TlsServerKeyExchangeEcdhe dhe1; @@ -468,6 +468,7 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) { // Restart Reset(); + EnableECDHEServerKeyReuse(); auto filter2 = MakeTlsFilter( server_, kTlsHandshakeServerKeyExchange); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); @@ -485,7 +486,6 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) { // This test parses the ServerKeyExchange, which isn't in 1.3 TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceNewKey) { - server_->SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); auto filter = MakeTlsFilter( server_, kTlsHandshakeServerKeyExchange); Connect(); @@ -495,7 +495,6 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceNewKey) { // Restart Reset(); - server_->SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); auto filter2 = MakeTlsFilter( server_, kTlsHandshakeServerKeyExchange); ConfigureSessionCache(RESUME_NONE, RESUME_NONE); diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc index cfc1320b2..650be8449 100644 --- a/gtests/ssl_gtest/tls_agent.cc +++ b/gtests/ssl_gtest/tls_agent.cc @@ -1172,9 +1172,9 @@ void TlsAgent::ConfigureSessionCache(SessionResumptionMode mode) { mode & RESUME_TICKET ? PR_TRUE : PR_FALSE); } -void TlsAgent::DisableECDHEServerKeyReuse() { +void TlsAgent::EnableECDHEServerKeyReuse() { ASSERT_EQ(TlsAgent::SERVER, role_); - SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_FALSE); + SetOption(SSL_REUSE_SERVER_ECDHE_KEY, PR_TRUE); } static const std::string kTlsRolesAllArr[] = {"CLIENT", "SERVER"}; diff --git a/gtests/ssl_gtest/tls_agent.h b/gtests/ssl_gtest/tls_agent.h index 21a6c0475..8b54155a5 100644 --- a/gtests/ssl_gtest/tls_agent.h +++ b/gtests/ssl_gtest/tls_agent.h @@ -193,7 +193,7 @@ class TlsAgent : public PollTarget { void SetDowngradeCheckVersion(uint16_t version); void CheckSecretsDestroyed(); void ConfigNamedGroups(const std::vector& groups); - void DisableECDHEServerKeyReuse(); + void EnableECDHEServerKeyReuse(); bool GetPeerChainLength(size_t* count); void CheckCipherSuite(uint16_t cipher_suite); void SetResumptionTokenCallback(); diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc index 50a4d9661..fd10e34a7 100644 --- a/gtests/ssl_gtest/tls_connect.cc +++ b/gtests/ssl_gtest/tls_connect.cc @@ -900,8 +900,8 @@ void TlsConnectTestBase::CheckEarlyDataAccepted() { server_->CheckEarlyDataAccepted(expect_early_data_accepted_); } -void TlsConnectTestBase::DisableECDHEServerKeyReuse() { - server_->DisableECDHEServerKeyReuse(); +void TlsConnectTestBase::EnableECDHEServerKeyReuse() { + server_->EnableECDHEServerKeyReuse(); } void TlsConnectTestBase::SkipVersionChecks() { diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h index d4d42a596..6a4795f83 100644 --- a/gtests/ssl_gtest/tls_connect.h +++ b/gtests/ssl_gtest/tls_connect.h @@ -134,7 +134,7 @@ class TlsConnectTestBase : public ::testing::Test { void Receive(size_t amount); void ExpectExtendedMasterSecret(bool expected); void ExpectEarlyDataAccepted(bool expected); - void DisableECDHEServerKeyReuse(); + void EnableECDHEServerKeyReuse(); void SkipVersionChecks(); // Move the DTLS timers for both endpoints to pop the next timer. diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h index e68e24d50..90d44f11f 100644 --- a/lib/ssl/ssl.h +++ b/lib/ssl/ssl.h @@ -175,7 +175,7 @@ SSL_IMPORT PRFileDesc *DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd); /* SSL_REUSE_SERVER_ECDHE_KEY controls whether the ECDHE server key is * reused for multiple handshakes or generated each time. - * SSL_REUSE_SERVER_ECDHE_KEY is currently enabled by default. + * SSL_REUSE_SERVER_ECDHE_KEY is currently disabled by default. * This socket option is for ECDHE, only. It is unrelated to DHE. */ #define SSL_REUSE_SERVER_ECDHE_KEY 27 diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c index cfd9e5a9a..6fc70500d 100644 --- a/lib/ssl/sslsock.c +++ b/lib/ssl/sslsock.c @@ -79,7 +79,7 @@ static sslOptions ssl_defaults = { .enableOCSPStapling = PR_FALSE, .enableDelegatedCredentials = PR_FALSE, .enableALPN = PR_TRUE, - .reuseServerECDHEKey = PR_TRUE, + .reuseServerECDHEKey = PR_FALSE, .enableFallbackSCSV = PR_FALSE, .enableServerDhe = PR_TRUE, .enableExtendedMS = PR_TRUE, -- cgit v1.2.1