Bug 1166338 - Change SSL_REUSE_SERVER_ECDHE_KEY default to false. r=djackson

Differential Revision: https://phabricator.services.mozilla.com/D143514

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<TlsConversationRecorder>(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<TlsHandshakeRecorder>(
       server_, kTlsHandshakeServerKeyExchange);
+  EnableECDHEServerKeyReuse();
   Connect();
   CheckKeys();
   TlsServerKeyExchangeEcdhe dhe1;
@@ -468,6 +468,7 @@ TEST_P(TlsConnectGenericPre13, ConnectEcdheTwiceReuseKey) {
 
   // Restart
   Reset();
+  EnableECDHEServerKeyReuse();
   auto filter2 = MakeTlsFilter<TlsHandshakeRecorder>(
       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<TlsHandshakeRecorder>(
       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<TlsHandshakeRecorder>(
       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<SSLNamedGroup>& 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,