Revert of Use cert config options in SSLServerSocketOpenSSL. (patchset #4...

Revert of Use cert config options in SSLServerSocketOpenSSL. (patchset #4 id:80001 of https://codereview.chromium.org/1138813003/)

Reason for revert:
It seems like this change has caused new leaks on Linux and ChromeOS.

First build it appeared: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%281%29/builds/41241

Log output:
Memcheck:Leak
fun:calloc
fun:PORT_ZAlloc_Util
fun:ConvertToSID
fun:ServerSessionIDLookup
fun:ssl3_HandleClientHello
fun:ssl3_HandleHandshakeMessage
fun:ssl3_HandleHandshake
fun:ssl3_HandleRecord
fun:ssl3_GatherCompleteHandshake
fun:SSL_ForceHandshake
fun:_ZN3net18SSLServerSocketNSS11DoHandshakeEv
fun:_ZN3net18SSLServerSocketNSS15DoHandshakeLoopEi
fun:_ZN3net18SSLServerSocketNSS21OnHandshakeIOCompleteEi
fun:_ZN3net18SSLServerSocketNSS14OnRecvCompleteEi
fun:_ZN3net18SSLServerSocketNSS18BufferRecvCompleteEi

This might be tickling some bug in underlying libraries, or it might be a problem with the change itself.

You can reproduce the leak by running valgrind and running all the SSLServerSocket tests. I didn't narrow down which test.

See https://www.chromium.org/developers/how-tos/using-valgrind for more details on using valgrind.

Original issue's description:
> Use cipher suite config options in SSLServerSocketOpenSSL.
>
> Previously SSLServerSocketOpenSSL was ignoring disabled_cipher_suites
> list and require_forward_secrecy flag from SSLConfig. Fixed
> SSLServerSocketOpenSSL to trim the list of cipher suites used in BoringSSL.
>
> BUG=481163
>
> Committed: https://crrev.com/d0eae58087e6f45088d6ef349d9ebaa2da450ea1
> Cr-Commit-Position: refs/heads/master@{#329528}

TBR=davidben@chromium.org,sergeyu@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=481163

Review URL: https://codereview.chromium.org/1131573005

Cr-Commit-Position: refs/heads/master@{#329594}

net/socket/ssl_server_socket_nss.cc

@@ -349,7 +349,7 @@ int SSLServerSocketNSS::InitializeSSLOptions() {
     return ERR_NO_SSL_VERSIONS_ENABLED;
   }
 
-  if (ssl_config_.require_ecdhe) {
+  if (ssl_config_.require_forward_secrecy) {
     const PRUint16* const ssl_ciphers = SSL_GetImplementedCiphers();
     const PRUint16 num_ciphers = SSL_GetNumImplementedCiphers();
 

net/socket/ssl_server_socket_openssl.cc

@@ -9,7 +9,6 @@
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
-#include "base/strings/string_util.h"
 #include "crypto/openssl_util.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_openssl_types.h"
@@ -686,48 +685,6 @@ int SSLServerSocketOpenSSL::Init() {
   SSL_set_mode(ssl_, mode.set_mask);
   SSL_clear_mode(ssl_, mode.clear_mask);
 
-  // Removing ciphers by ID from OpenSSL is a bit involved as we must use the
-  // textual name with SSL_set_cipher_list because there is no public API to
-  // directly remove a cipher by ID.
-  STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl_);
-  DCHECK(ciphers);
-  // See SSLConfig::disabled_cipher_suites for description of the suites
-  // disabled by default. Note that !SHA256 and !SHA384 only remove HMAC-SHA256
-  // and HMAC-SHA384 cipher suites, not GCM cipher suites with SHA256 or SHA384
-  // as the handshake hash.
-  std::string command("DEFAULT:!SHA256:!SHA384:!AESGCM+AES256:!aPSK");
-  // Walk through all the installed ciphers, seeing if any need to be
-  // appended to the cipher removal |command|.
-  for (size_t i = 0; i < sk_SSL_CIPHER_num(ciphers); ++i) {
-    const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i);
-    const uint16_t id = static_cast<uint16_t>(SSL_CIPHER_get_id(cipher));
-
-    bool disable = false;
-    if (ssl_config_.require_ecdhe) {
-      base::StringPiece kx_name(SSL_CIPHER_get_kx_name(cipher));
-      disable = kx_name != "ECDHE_RSA" && kx_name != "ECDHE_ECDSA";
-    }
-    if (!disable) {
-      disable = std::find(ssl_config_.disabled_cipher_suites.begin(),
-                          ssl_config_.disabled_cipher_suites.end(),
-                          id) != ssl_config_.disabled_cipher_suites.end();
-    }
-    if (disable) {
-      const char* name = SSL_CIPHER_get_name(cipher);
-      DVLOG(3) << "Found cipher to remove: '" << name << "', ID: " << id
-               << " strength: " << SSL_CIPHER_get_bits(cipher, NULL);
-      command.append(":!");
-      command.append(name);
-    }
-  }
-
-  int rv = SSL_set_cipher_list(ssl_, command.c_str());
-  // If this fails (rv = 0) it means there are no ciphers enabled on this SSL.
-  // This will almost certainly result in the socket failing to complete the
-  // handshake at which point the appropriate error is bubbled up to the client.
-  LOG_IF(WARNING, rv != 1) << "SSL_set_cipher_list('" << command
-                           << "') returned " << rv;
-
   return OK;
 }
 

net/socket/ssl_server_socket_unittest.cc

@@ -326,30 +326,30 @@ class SSLServerSocketTest : public PlatformTest {
     scoped_ptr<crypto::RSAPrivateKey> private_key(
         crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(key_vector));
 
-    client_ssl_config_.false_start_enabled = false;
-    client_ssl_config_.channel_id_enabled = false;
+    SSLConfig ssl_config;
+    ssl_config.false_start_enabled = false;
+    ssl_config.channel_id_enabled = false;
 
     // Certificate provided by the host doesn't need authority.
     SSLConfig::CertAndStatus cert_and_status;
     cert_and_status.cert_status = CERT_STATUS_AUTHORITY_INVALID;
     cert_and_status.der_cert = cert_der;
-    client_ssl_config_.allowed_bad_certs.push_back(cert_and_status);
+    ssl_config.allowed_bad_certs.push_back(cert_and_status);
 
     HostPortPair host_and_pair("unittest", 0);
     SSLClientSocketContext context;
     context.cert_verifier = cert_verifier_.get();
     context.transport_security_state = transport_security_state_.get();
-    client_socket_ = socket_factory_->CreateSSLClientSocket(
-        client_connection.Pass(), host_and_pair, client_ssl_config_, context);
-    server_socket_ =
-        CreateSSLServerSocket(server_socket.Pass(), cert.get(),
-                              private_key.get(), server_ssl_config_);
+    client_socket_ =
+        socket_factory_->CreateSSLClientSocket(
+            client_connection.Pass(), host_and_pair, ssl_config, context);
+    server_socket_ = CreateSSLServerSocket(
+        server_socket.Pass(),
+        cert.get(), private_key.get(), SSLConfig());
   }
 
   FakeDataChannel channel_1_;
   FakeDataChannel channel_2_;
-  SSLConfig client_ssl_config_;
-  SSLConfig server_ssl_config_;
   scoped_ptr<SSLClientSocket> client_socket_;
   scoped_ptr<SSLServerSocket> server_socket_;
   ClientSocketFactory* socket_factory_;
@@ -591,40 +591,4 @@ TEST_F(SSLServerSocketTest, ExportKeyingMaterial) {
   EXPECT_NE(0, memcmp(server_out, client_bad, sizeof(server_out)));
 }
 
-// Verifies that SSLConfig::require_ecdhe flags works properly.
-TEST_F(SSLServerSocketTest, RequireEcdheFlag) {
-  // Disable all ECDHE suites on the client side.
-  uint16_t kEcdheCiphers[] = {
-      0xc007,  // ECDHE_ECDSA_WITH_RC4_128_SHA
-      0xc009,  // ECDHE_ECDSA_WITH_AES_128_CBC_SHA
-      0xc00a,  // ECDHE_ECDSA_WITH_AES_256_CBC_SHA
-      0xc011,  // ECDHE_RSA_WITH_RC4_128_SHA
-      0xc013,  // ECDHE_RSA_WITH_AES_128_CBC_SHA
-      0xc014,  // ECDHE_RSA_WITH_AES_256_CBC_SHA
-      0xc02b,  // ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
-      0xc02f,  // ECDHE_RSA_WITH_AES_128_GCM_SHA256
-      0xcc13,  // ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
-      0xcc14,  // ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
-  };
-  client_ssl_config_.disabled_cipher_suites.assign(
-      kEcdheCiphers, kEcdheCiphers + arraysize(kEcdheCiphers));
-
-  // Require ECDHE on the server.
-  server_ssl_config_.require_ecdhe = true;
-
-  Initialize();
-
-  TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
-
-  int client_ret = client_socket_->Connect(connect_callback.callback());
-  int server_ret = server_socket_->Handshake(handshake_callback.callback());
-
-  client_ret = connect_callback.GetResult(client_ret);
-  server_ret = handshake_callback.GetResult(client_ret);
-
-  ASSERT_EQ(ERR_SSL_VERSION_OR_CIPHER_MISMATCH, client_ret);
-  ASSERT_EQ(ERR_SSL_VERSION_OR_CIPHER_MISMATCH, server_ret);
-}
-
 }  // namespace net

net/ssl/ssl_config.cc

@@ -26,7 +26,7 @@ SSLConfig::SSLConfig()
       channel_id_enabled(true),
       false_start_enabled(true),
       signed_cert_timestamps_enabled(true),
-      require_ecdhe(false),
+      require_forward_secrecy(false),
       send_client_cert(false),
       verify_ev_cert(false),
       version_fallback(false),

net/ssl/ssl_config.h

@@ -115,10 +115,10 @@ struct NET_EXPORT SSLConfig {
   // TLS extension is enabled.
   bool signed_cert_timestamps_enabled;
 
-  // If true, causes only ECDHE cipher suites to be enabled. NOTE: This only
-  // applies to server sockets currently, although that could be extended if
-  // needed.
-  bool require_ecdhe;
+  // require_forward_secrecy, if true, causes only (EC)DHE cipher suites to be
+  // enabled. NOTE: this only applies to server sockets currently, although
+  // that could be extended if needed.
+  bool require_forward_secrecy;
 
   // TODO(wtc): move the following members to a new SSLParams structure.  They
   // are not SSL configuration settings.

net/ssl/ssl_config_service.cc

@@ -96,7 +96,8 @@ void SSLConfigService::ProcessConfigUpdate(const SSLConfig& orig_config,
        new_config.disabled_cipher_suites) ||
       (orig_config.channel_id_enabled != new_config.channel_id_enabled) ||
       (orig_config.false_start_enabled != new_config.false_start_enabled) ||
-      (orig_config.require_ecdhe != new_config.require_ecdhe);
+      (orig_config.require_forward_secrecy !=
+       new_config.require_forward_secrecy);
 
   if (config_changed)
     NotifySSLConfigChange();

remoting/protocol/ssl_hmac_channel_authenticator.cc

@@ -109,7 +109,7 @@ void SslHmacChannelAuthenticator::SecureAndAuthenticate(
     }
 
     net::SSLConfig ssl_config;
-    ssl_config.require_ecdhe = true;
+    ssl_config.require_forward_secrecy = true;
 
     scoped_ptr<net::SSLServerSocket> server_socket =
         net::CreateSSLServerSocket(socket.Pass(),