Add support for restricting the cipher suites that SSLClientSocket(Mac,NSS) use. Restricting SSLClientSocketWin is handled by the existing Windows system policy (which deals in algorithms, not cipher suites).

R=wtc
BUG=58831
TEST=SSLClientSocketTest.CipherSuiteDisables

Review URL: http://codereview.chromium.org/3845005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65773 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 242 insertions, 53 deletions

diff --git a/net/base/ssl_config_service.h b/net/base/ssl_config_service.h
index be50097..0639f48 100644
--- a/net/base/ssl_config_service.h
+++ b/net/base/ssl_config_service.h
@@ -8,6 +8,7 @@
 
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/observer_list.h"
 #include "base/ref_counted.h"
 #include "net/base/x509_certificate.h"
@@ -31,6 +32,24 @@ struct SSLConfig {
   // True if we'll do async checks for certificate provenance using DNS.
   bool dns_cert_provenance_checking_enabled;
 
+  // Cipher suites which should be explicitly prevented from being used. By
+  // default, all cipher suites supported by the underlying SSL implementation
+  // will be enabled, except for:
+  // - Null encryption cipher suites.
+  // - Weak cipher suites: < 80 bits of security strength.
+  // - FORTEZZA cipher suites (obsolete).
+  // - IDEA cipher suites (RFC 5469 explains why).
+  // - Anonymous cipher suites.
+  //
+  // Though cipher suites are sent in TLS as "uint8 CipherSuite[2]", in
+  // big-endian form, they should be declared in host byte order, with the
+  // first uint8 occupying the most significant byte.
+  // Ex: To disable TLS_RSA_WITH_RC4_128_MD5, specify 0x0004, while to
+  // disable TLS_ECDH_ECDSA_WITH_RC4_128_SHA, specify 0xC002.
+  //
+  // TODO(rsleevi): Not implemented when using OpenSSL or Schannel.
+  std::vector<uint16> disabled_cipher_suites;
+
   // True if we allow this connection to be MITM attacked. This sounds a little
   // worse than it is: large networks sometimes MITM attack all SSL connections
   // on egress. We want to know this because we might not have the end-to-end
diff --git a/net/base/ssl_config_service_mac.cc b/net/base/ssl_config_service_mac.cc
index 2ce1d5c..148bba4 100644
--- a/net/base/ssl_config_service_mac.cc
+++ b/net/base/ssl_config_service_mac.cc
@@ -97,6 +97,8 @@ bool SSLConfigServiceMac::GetSSLConfigNow(SSLConfig* config) {
                                              kTLS1EnabledDefaultValue);
   SSLConfigService::SetSSLConfigFlags(config);
 
+  // TODO(rsleevi): http://crbug.com/58831 - Implement preferences for
+  // disabling cipher suites.
   return true;
 }
 
diff --git a/net/base/ssl_config_service_win.cc b/net/base/ssl_config_service_win.cc
index debea7d..d4153c3 100644
--- a/net/base/ssl_config_service_win.cc
+++ b/net/base/ssl_config_service_win.cc
@@ -82,6 +82,13 @@ bool SSLConfigServiceWin::GetSSLConfigNow(SSLConfig* config) {
   config->tls1_enabled = ((protocols & TLS1) != 0);
   SSLConfigService::SetSSLConfigFlags(config);
 
+  // TODO(rsleevi): Possibly respect the registry keys defined in
+  // http://support.microsoft.com/kb/245030 (pre-Vista) or
+  // http://msdn.microsoft.com/en-us/library/bb870930(VS.85).aspx (post-Vista).
+  // Currently, these values are respected implicitly when using
+  // SSLClientSocketWin, but they do not propogate to SSLClientSocketNSS
+  // because we're not currently translating the keys.
+
   return true;
 }
 
diff --git a/net/net.gyp b/net/net.gyp
index 2d2aa50..5283400 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -610,6 +610,8 @@
         'socket/ssl_client_socket_pool.h',
         'socket/ssl_client_socket_win.cc',
         'socket/ssl_client_socket_win.h',
+        'socket/ssl_error_params.cc',
+        'socket/ssl_error_params.h',
         'socket/tcp_client_socket.cc',
         'socket/tcp_client_socket.h',
         'socket/tcp_client_socket_libevent.cc',
diff --git a/net/socket/ssl_client_socket_mac.cc b/net/socket/ssl_client_socket_mac.cc
index d6fca9b..6ef573c 100644
--- a/net/socket/ssl_client_socket_mac.cc
+++ b/net/socket/ssl_client_socket_mac.cc
@@ -9,6 +9,8 @@
 #include <sys/socket.h>
 #include <sys/types.h>
 
+#include <algorithm>
+
 #include "base/mac/scoped_cftyperef.h"
 #include "base/singleton.h"
 #include "base/string_util.h"
@@ -21,6 +23,7 @@
 #include "net/base/ssl_connection_status_flags.h"
 #include "net/base/ssl_info.h"
 #include "net/socket/client_socket_handle.h"
+#include "net/socket/ssl_error_params.h"
 
 // Welcome to Mac SSL. We've been waiting for you.
 //
@@ -143,6 +146,7 @@ int NetErrorFromOSStatus(OSStatus status) {
   switch (status) {
     case errSSLWouldBlock:
       return ERR_IO_PENDING;
+    case paramErr:
     case errSSLBadCipherSuite:
     case errSSLBadConfiguration:
       return ERR_INVALID_ARGUMENT;
@@ -200,13 +204,13 @@ int NetErrorFromOSStatus(OSStatus status) {
       LOG(WARNING) << "Server rejected client cert (OSStatus=" << status << ")";
       return ERR_BAD_SSL_CLIENT_AUTH_CERT;
 
+    case errSSLNegotiation:
     case errSSLPeerInsufficientSecurity:
     case errSSLPeerProtocolVersion:
       return ERR_SSL_VERSION_OR_CIPHER_MISMATCH;
 
     case errSSLBufferOverflow:
     case errSSLModuleAttach:
-    case errSSLNegotiation:
     case errSSLSessionNotFound:
     default:
       LOG(WARNING) << "Unknown error " << status <<
@@ -448,20 +452,33 @@ FNTYPE LookupFunction(CFStringRef bundleName, CFStringRef fnName) {
       CFBundleGetFunctionPointerForName(bundle, fnName));
 }
 
-// A class that wraps an array of enabled cipher suites that can be passed to
-// SSLSetEnabledCiphers.
-//
-// Used as a singleton.
+struct CipherSuiteIsDisabledFunctor {
+  explicit CipherSuiteIsDisabledFunctor(
+      const std::vector<uint16>& disabled_cipher_suites)
+      : disabled_cipher_suites_(disabled_cipher_suites) {}
+
+  // Returns true if the given |cipher_suite| appears within the set of
+  // |disabled_cipher_suites|.
+  bool operator()(SSLCipherSuite cipher_suite) const {
+    return binary_search(disabled_cipher_suites_.begin(),
+                         disabled_cipher_suites_.end(),
+                         static_cast<uint16>(cipher_suite));
+  }
+
+  const std::vector<uint16>& disabled_cipher_suites_;
+};
+
+// Class to determine what cipher suites are available and which cipher
+// suites should be enabled, based on the overall security policy.
 class EnabledCipherSuites {
  public:
-  EnabledCipherSuites();
-
-  const SSLCipherSuite* ciphers() const {
-    return ciphers_.empty() ? NULL : &ciphers_[0];
-  }
-  size_t num_ciphers() const { return ciphers_.size(); }
+  const std::vector<SSLCipherSuite>& ciphers() const { return ciphers_; }
 
  private:
+  friend struct DefaultSingletonTraits<EnabledCipherSuites>;
+  EnabledCipherSuites();
+  ~EnabledCipherSuites() {}
+
   std::vector<SSLCipherSuite> ciphers_;
 
   DISALLOW_COPY_AND_ASSIGN(EnabledCipherSuites);
@@ -520,6 +537,11 @@ SSLClientSocketMac::SSLClientSocketMac(ClientSocketHandle* transport_socket,
       ssl_context_(NULL),
       pending_send_error_(OK),
       net_log_(transport_socket->socket()->NetLog()) {
+  // Sort the list of ciphers to disable, since disabling ciphers on Mac
+  // requires subtracting from a list of enabled ciphers while maintaining
+  // ordering, as opposed to merely needing to iterate them as with NSS.
+  sort(ssl_config_.disabled_cipher_suites.begin(),
+       ssl_config_.disabled_cipher_suites.end());
 }
 
 SSLClientSocketMac::~SSLClientSocketMac() {
@@ -761,10 +783,22 @@ int SSLClientSocketMac::InitializeSSLContext() {
   if (status)
     return NetErrorFromOSStatus(status);
 
-  const EnabledCipherSuites* enabled_ciphers =
-      Singleton<EnabledCipherSuites>::get();
-  status = SSLSetEnabledCiphers(ssl_context_, enabled_ciphers->ciphers(),
-                                enabled_ciphers->num_ciphers());
+  std::vector<SSLCipherSuite> enabled_ciphers =
+      Singleton<EnabledCipherSuites>::get()->ciphers();
+
+  CipherSuiteIsDisabledFunctor is_disabled_cipher(
+      ssl_config_.disabled_cipher_suites);
+  std::vector<SSLCipherSuite>::iterator new_end =
+      std::remove_if(enabled_ciphers.begin(), enabled_ciphers.end(),
+                     is_disabled_cipher);
+  if (new_end != enabled_ciphers.end())
+    enabled_ciphers.erase(new_end, enabled_ciphers.end());
+
+  status = SSLSetEnabledCiphers(
+      ssl_context_,
+      enabled_ciphers.empty() ? NULL : &enabled_ciphers[0],
+      enabled_ciphers.size());
+
   if (status)
     return NetErrorFromOSStatus(status);
 
@@ -970,39 +1004,50 @@ int SSLClientSocketMac::DoHandshake() {
   if (client_cert_state > kSSLClientCertNone)
     client_cert_requested_ = true;
 
+  int net_error = ERR_FAILED;
   switch (status) {
     case noErr:
       return DidCompleteHandshake();
     case errSSLWouldBlock:
       next_handshake_state_ = STATE_HANDSHAKE;
-      break;
+      return ERR_IO_PENDING;
     case errSSLClosedGraceful:
       // The server unexpectedly closed on us.
-      return ERR_SSL_PROTOCOL_ERROR;
+      net_error = ERR_SSL_PROTOCOL_ERROR;
+      break;
     case errSSLClosedAbort:
     case errSSLPeerHandshakeFail:
       if (client_cert_requested_) {
-        // See if the server aborted due to client cert checking.
         if (!ssl_config_.send_client_cert) {
+          // The server aborted, likely due to requiring a client certificate
+          // and one wasn't sent.
           VLOG(1) << "Server requested SSL cert during handshake";
-          return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+          net_error = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+        } else {
+          // The server aborted, likely due to not liking the client
+          // certificate that was sent.
+          LOG(WARNING) << "Server aborted SSL handshake";
+          net_error = ERR_BAD_SSL_CLIENT_AUTH_CERT;
         }
-        LOG(WARNING) << "Server aborted SSL handshake";
-        return ERR_BAD_SSL_CLIENT_AUTH_CERT;
+        // Don't fall through - the error was intentionally remapped.
+        break;
+      }
+      // Fall through if a client cert wasn't requested.
+    default:
+      net_error = NetErrorFromOSStatus(status);
+      DCHECK(!IsCertificateError(net_error));
+      if (!ssl_config_.send_client_cert &&
+         (client_cert_state == kSSLClientCertRejected ||
+          net_error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
+        // The server unexpectedly sent a peer certificate error alert when no
+        // certificate had been sent.
+        net_error = ERR_SSL_PROTOCOL_ERROR;
       }
       break;
   }
 
-  int net_error = NetErrorFromOSStatus(status);
-  DCHECK(!IsCertificateError(net_error));
-
-  if (!ssl_config_.send_client_cert &&
-     (client_cert_state == kSSLClientCertRejected ||
-      net_error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
-    // The server unexpectedly sent a peer certificate error alert when no
-    // certificate had been sent.
-    net_error = ERR_SSL_PROTOCOL_ERROR;
-  }
+  net_log_.AddEvent(NetLog::TYPE_SSL_HANDSHAKE_ERROR,
+                    new SSLErrorParams(net_error, status));
   return net_error;
 }
 
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index ff49656..a9b8822 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -94,6 +94,7 @@
 #include "net/ocsp/nss_ocsp.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/dns_cert_provenance_check.h"
+#include "net/socket/ssl_error_params.h"
 #include "net/socket/ssl_host_info.h"
 
 static const int kRecvBufferSize = 4096;
@@ -272,28 +273,6 @@ int MapHandshakeError(PRErrorCode err) {
   }
 }
 
-// Extra parameters to attach to the NetLog when we receive an SSL error.
-class SSLErrorParams : public NetLog::EventParameters {
- public:
-  // If |ssl_lib_error| is 0, it will be ignored.
-  SSLErrorParams(int net_error, PRErrorCode ssl_lib_error)
-      : net_error_(net_error),
-        ssl_lib_error_(ssl_lib_error) {
-  }
-
-  virtual Value* ToValue() const {
-    DictionaryValue* dict = new DictionaryValue();
-    dict->SetInteger("net_error", net_error_);
-    if (ssl_lib_error_)
-      dict->SetInteger("ssl_lib_error", ssl_lib_error_);
-    return dict;
-  }
-
- private:
-  const int net_error_;
-  const PRErrorCode ssl_lib_error_;
-};
-
 // Extra parameters to attach to the NetLog when we receive an error in response
 // to a call to an NSS function.  Used instead of SSLErrorParams with
 // events of type TYPE_SSL_NSS_ERROR.  Automatically looks up last PR error.
@@ -729,6 +708,14 @@ int SSLClientSocketNSS::InitializeSSLOptions() {
     return ERR_UNEXPECTED;
   }
 
+  for (std::vector<uint16>::const_iterator it =
+           ssl_config_.disabled_cipher_suites.begin();
+       it != ssl_config_.disabled_cipher_suites.end(); ++it) {
+    // This will fail if the specified cipher is not implemented by NSS, but
+    // the failure is harmless.
+    SSL_CipherPrefSet(nss_fd_, *it, PR_FALSE);
+  }
+
 #ifdef SSL_ENABLE_SESSION_TICKETS
   // Support RFC 5077
   rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_SESSION_TICKETS, PR_TRUE);
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 5064bb3..3a1bd5ba 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -514,3 +514,78 @@ TEST_F(SSLClientSocketTest, PrematureApplicationData) {
   rv = sock->Connect(&callback);
   EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
 }
+
+#if defined(USE_OPENSSL)
+// TODO(rsleevi): Not implemented for Schannel or OpenSSL. Schannel is
+// controlled by the SSL client socket factory, rather than a define, so it
+// cannot be conditionally disabled here. As Schannel is only used when
+// performing client authentication, it will not be tested here.
+#define MAYBE_CipherSuiteDisables DISABLED_CipherSuiteDisables
+#else
+#define MAYBE_CipherSuiteDisables CipherSuiteDisables
+#endif
+TEST_F(SSLClientSocketTest, MAYBE_CipherSuiteDisables) {
+  // Rather than exhaustively disabling every RC4 ciphersuite defined at
+  // http://www.iana.org/assignments/tls-parameters/tls-parameters.xml,
+  // only disabling those cipher suites that the test server actually
+  // implements.
+  const uint16 kCiphersToDisable[] = {
+    0x0005,  // TLS_RSA_WITH_RC4_128_SHA
+  };
+
+  net::TestServer::HTTPSOptions https_options;
+  // Enable only RC4 on the test server.
+  https_options.bulk_ciphers =
+      net::TestServer::HTTPSOptions::BULK_CIPHER_RC4;
+  net::TestServer test_server(https_options, FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  net::AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
+  net::ClientSocket* transport = new net::TCPClientSocket(
+      addr, &log, net::NetLog::Source());
+  int rv = transport->Connect(&callback);
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  net::SSLConfig ssl_config;
+  for (size_t i = 0; i < arraysize(kCiphersToDisable); ++i)
+    ssl_config.disabled_cipher_suites.push_back(kCiphersToDisable[i]);
+
+  scoped_ptr<net::SSLClientSocket> sock(
+      socket_factory_->CreateSSLClientSocket(
+          transport, test_server.host_port_pair().host(),
+          ssl_config, NULL));
+
+  EXPECT_FALSE(sock->IsConnected());
+
+  rv = sock->Connect(&callback);
+  EXPECT_TRUE(net::LogContainsBeginEvent(
+      log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
+
+  // NSS has special handling that maps a handshake_failure alert received
+  // immediately after a client_hello to be a mismatched cipher suite error,
+  // leading to ERR_SSL_VERSION_OR_CIPHER_MISMATCH. When using OpenSSL or
+  // Secure Transport (OS X), the handshake_failure is bubbled up without any
+  // interpretation, leading to ERR_SSL_PROTOCOL_ERROR. Either way, a failure
+  // indicates that no cipher suite was negotiated with the test server.
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+  EXPECT_TRUE(rv == net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH ||
+              rv == net::ERR_SSL_PROTOCOL_ERROR);
+  // The exact ordering differs between SSLClientSocketNSS (which issues an
+  // extra read) and SSLClientSocketMac (which does not). Just make sure the
+  // error appears somewhere in the log.
+  net::ExpectLogContainsSomewhere(log.entries(), 0,
+                                  net::NetLog::TYPE_SSL_HANDSHAKE_ERROR,
+                                  net::NetLog::PHASE_NONE);
+
+  // We cannot test sock->IsConnected(), as the NSS implementation disconnects
+  // the socket when it encounters an error, whereas other implementations
+  // leave it connected.
+  EXPECT_TRUE(LogContainsSSLConnectEndEvent(log.entries(), -1));
+}
diff --git a/net/socket/ssl_error_params.cc b/net/socket/ssl_error_params.cc
new file mode 100644
index 0000000..c77cbec
--- /dev/null
+++ b/net/socket/ssl_error_params.cc
@@ -0,0 +1,24 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/socket/ssl_error_params.h"
+
+#include "base/values.h"
+
+namespace net {
+
+SSLErrorParams::SSLErrorParams(int net_error, int ssl_lib_error)
+    : net_error_(net_error), ssl_lib_error_(ssl_lib_error) {}
+
+SSLErrorParams::~SSLErrorParams() {}
+
+Value* SSLErrorParams::ToValue() const {
+  DictionaryValue* dict = new DictionaryValue();
+  dict->SetInteger("net_error", net_error_);
+  if (ssl_lib_error_)
+    dict->SetInteger("ssl_lib_error", ssl_lib_error_);
+  return dict;
+}
+
+}  // namespace net
diff --git a/net/socket/ssl_error_params.h b/net/socket/ssl_error_params.h
new file mode 100644
index 0000000..ae546fe
--- /dev/null
+++ b/net/socket/ssl_error_params.h
@@ -0,0 +1,28 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_SOCKET_SSL_ERROR_PARAMS_H_
+#define NET_SOCKET_SSL_ERROR_PARAMS_H_
+#pragma once
+
+#include "net/base/net_log.h"
+
+namespace net {
+
+// Extra parameters to attach to the NetLog when we receive an SSL error.
+class SSLErrorParams : public NetLog::EventParameters {
+ public:
+  SSLErrorParams(int net_error, int ssl_lib_error);
+  virtual ~SSLErrorParams();
+
+  virtual Value* ToValue() const;
+
+ private:
+  const int net_error_;
+  const int ssl_lib_error_;
+};
+
+}  // namespace net
+
+#endif  // NET_SOCKET_SSL_ERROR_PARAMS_H_