Add support for restricting the cipher suites that SSLClientSocket(Mac,NSS) use

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65773

[Ryan Sleevi, 2010-10-17 19:37:15]

@wtc: Would you mind taking a look?

I've implemented cipher suite selection as an opt-out, rather than an opt-in, to
ensure that if/when new ciphers are added, they won't be stunted by admins who
fail to update their lists. On the other hand, having an opt-in list would give
admins greater control, since they don't have to worry about new cipher suites
being added which they would prefer disabled.

Support for SSLClientSocketWin isn't added, for two reasons: First, Schannel
doesn't support disabling individual cipher suites - it simply supports an array
of ALG_IDs to add (eg: RC4, SHA1) and a minimum/maximum "strength" size (eg:
56-256). Second, admins can already control the lists via Registry/Group Policy
settings, so it's not something that will be missed too badly.

@agl: I know you said you weren't familiar with the platform-specific parts, I
just wanted to make sure you had no concerns with the opt-out model.

http://codereview.chromium.org/3845005/diff/1/5
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/1/5#newcode150
net/socket/ssl_client_socket_mac.cc:150: case paramErr:
When digging in the Secure Transport sources, saw several places where a
paramErr can bubble up.

http://codereview.chromium.org/3845005/diff/1/5#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
The documentation in SecureTransport.h reads /* Cipher Suite negotiation
failure. */

When doing client side SSL, this is only raised when the server sends a cipher
suite that wasn't in our list - namely, it doesn't send it for handshake_failure
after the client hello (which is what NSS does)

[agl, 2010-10-18 20:40:17]

http://codereview.chromium.org/3845005/diff/1/2
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/1/2#newcode37
net/base/ssl_config_service.h:37: std::set<uint16> disabled_cipher_suites;
std::set is usually a RB tree, which is a lot of overhead when this list is
small. Because of locality, a std::vector is probably faster is the length is <
32.

[wtc, 2010-10-19 01:04:24]

http://codereview.chromium.org/3845005/diff/1/2
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/1/2#newcode37
net/base/ssl_config_service.h:37: std::set<uint16> disabled_cipher_suites;
We should think about whether we should use a blacklist
(disabled_cipher_suites) or a whitelist (enabled_cipher_suites)
here.  If using a blacklist, we should document which
cipher suites are enabled by default (anything that is
supported by the underlying SSL library and has >= 80 bits
of security).

Nit: since CipherSuite is defined as an array in TLS:
   uint8 CipherSuite[2];
you should specify byte order of the uint16 here.

http://codereview.chromium.org/3845005/diff/1/5
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/1/5#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
On 2010/10/17 19:37:16, rsleevi wrote:
>
> When doing client side SSL, this is only raised when the server sends a cipher
> suite that wasn't in our list

This sounds like a protocol error instead of SSL cipher
mismatch...

http://codereview.chromium.org/3845005/diff/1/6
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/3845005/diff/1/6#newcode842
net/socket/ssl_client_socket_nss.cc:842: for (std::set<uint16>::iterator it =
Nit: use const_iterator.

[Ryan Sleevi, 2010-10-20 13:22:04]

Thanks for the comments. I've tried to address everything in this CL. Also,
apologies that Patchset 2 also rebased to trunk - I rebased the test server
changes without thinking about how they'd bubble up into this CL.

http://codereview.chromium.org/3845005/diff/1/5
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/1/5#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
On 2010/10/19 01:04:25, wtc wrote:
> This sounds like a protocol error instead of SSL cipher
> mismatch...

The NSS behaviour is that it raises SSL_ERROR_NO_CYPHER_OVERLAP when a
handshake_failed is sent in response to the ClientHello OR when the ServerHello
contains a cipher suite that wasn't part of the ClientHello. Since both cases
are mapped to a single error code, and the _nss socket maps that to
ERR_SSL_VERSION_OR_CIPHER_MISMATCH, I was trying to maintain that behaviour in
the _mac socket, as disabling ciphers made it more likely to be hit.

http://codereview.chromium.org/3845005/diff/8001/9001
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/8001/9001#newcode51
net/base/ssl_config_service.h:51: std::vector<uint16> disabled_cipher_suites;
The only reason for the set<> was to be able to binary_search in the Mac
implementation. Instead, I've pushed the sorting into SSLClientSocketMac's
constructor. 

One alternative would have been to add accessor/mutator for this member, and
sort whenever it was set, but that seemed excessive since it only matters on
Mac. Another alternative would have been to ditch the sorting altogether, but
that would have further increased the complexity, since the lists of
enabled/disabled ciphers would have to be iterated through for every connection.

[wtc, 2010-10-25 23:45:04]

LGTM.  Please upload a new Patch Set before you check this in.
I want to review how you fix the second "BUG" below (in
ssl_client_socket_mac.cc), even if after the fact.  Thanks.

http://codereview.chromium.org/3845005/diff/8001/9001
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/8001/9001#newcode44
net/base/ssl_config_service.h:44: // big-endian form, they should be declared as
platform endian, with the
Nit: platform endian => host byte order

http://codereview.chromium.org/3845005/diff/8001/9001#newcode46
net/base/ssl_config_service.h:46: // Ex: To disable TLS_RSA_WITH_RC4_128_MD5,
specify 0x0005, while to
BUG: TLS_RSA_WITH_RC4_128_MD5 is 0x0004.

http://codereview.chromium.org/3845005/diff/8001/9001#newcode48
net/base/ssl_config_service.h:48: // platform endianness.
Delete "regardless of platform endianness", because the values
are in platform/host byte order.

http://codereview.chromium.org/3845005/diff/8001/9002
File net/base/ssl_config_service_mac.cc (right):

http://codereview.chromium.org/3845005/diff/8001/9002#newcode101
net/base/ssl_config_service_mac.cc:101: // Currently this is handled in
chrome/browser/policy, which uses
We should try very hard to avoid mentioning Chrome code
outside the network stack (the "net" directory) because the
network stack should ideally be usable by non-Chrome
applications.

http://codereview.chromium.org/3845005/diff/8001/9004
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/8001/9004#newcode410
net/socket/ssl_client_socket_mac.cc:410: // Extra parameters to attach to the
NetLog when receive an SSL error.
Nit: receive => we receive

This is almost the same as the SSLErrorParams class defined in
ssl_client_socket_nss.cc.  We should move it to a new file
ssl_error_params.h and use 'int' as the type of ssl_lib_error,
assuming both PRErrorCode and OSStatus can be cast to 'int'
safely.

http://codereview.chromium.org/3845005/diff/8001/9004#newcode489
net/socket/ssl_client_socket_mac.cc:489: class EnabledCipherSuitesSingleton {
Nit: why do you want to say "Singleton" twice in
Singleton<EnabledCipherSuitesSingleton>?

http://codereview.chromium.org/3845005/diff/8001/9004#newcode1017
net/socket/ssl_client_socket_mac.cc:1017: } else if (status == errSSLWouldBlock)
{
Nit: don't use "else" after a return statement.

http://codereview.chromium.org/3845005/diff/8001/9004#newcode1023
net/socket/ssl_client_socket_mac.cc:1023:
net_log_.AddEvent(NetLog::TYPE_SSL_HANDSHAKE_ERROR,
BUG: this is not right because we change net_error in three
cases:
Line 1035: ERR_SSL_CLIENT_AUTH_CERT_NEEDED
Line 1038: ERR_BAD_SSL_CLIENT_AUTH_CERT
Line 1050: ERR_SSL_PROTOCOL_ERROR

http://codereview.chromium.org/3845005/diff/8001/9005
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/3845005/diff/8001/9005#newcode847
net/socket/ssl_client_socket_nss.cc:847: // but it will fail gracefully.
Nit: it's not clear what you meant by "fail gracefully".  I
think it's enough to say "we ignore the failure" or "the
failure is harmless".

http://codereview.chromium.org/3845005/diff/8001/9006
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/3845005/diff/8001/9006#newcode567
net/socket/ssl_client_socket_unittest.cc:567: // controlled by the SSL factory,
rather than a define, so it cannot
Nit: SSL factory => SSL client socket factory

You may want to explain that Schannel is only for SSL client
auth, which is another reason we don't need to disable the
unit test for Schannel.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode579
net/socket/ssl_client_socket_unittest.cc:579: 0x0005, //
TLS_RSA_WITH_RC4_128_SHA
Nit: at least two spaces before '//'.

You should set up to be able to run cpplint.py, which warns
about this.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode603
net/socket/ssl_client_socket_unittest.cc:603: for (size_t i = 0; i <
arraysize(kCiphersToDisable); ++i) {
Nit: we don't use curly braces {} around one-liner for loop
bodies.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode608
net/socket/ssl_client_socket_unittest.cc:608:
socket_factory_->CreateSSLClientSocket(transport,
Nit: bad function argument formatting.  I suggest
      socket_factory_->CreateSSLClientSocket(
          transport, test_server.host_port_pair().host(),
          ssl_config, NULL));

http://codereview.chromium.org/3845005/diff/8001/9006#newcode623
net/socket/ssl_client_socket_unittest.cc:623: if (rv !=
net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH &&
I think it's better to just test for
  if (rv == net::ERR_SSL_PROTOCOL_ERROR) {
here, and remove the
    ASSERT_EQ(net::ERR_IO_PENDING, rv);
on line 625.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode635
net/socket/ssl_client_socket_unittest.cc:635: // extra read) and
SSLClientSocketMac (which does not). Just make sure the
What's the extra read?  Just curious.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode642
net/socket/ssl_client_socket_unittest.cc:642: // the socket when it encounters
an error, whereas other implementations
By "the socket", do you mean the transport socket?
It seems wrong for sock->IsConnected() to be true for any
SSLClientSocket implementation.  They are supposed to check
the old completed_handshake_ member or the new
completed_handshake() method.

[wtc, 2010-10-25 23:48:13]

One minor question.

http://codereview.chromium.org/3845005/diff/1/5
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/1/5#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
On 2010/10/17 19:37:16, rsleevi wrote:
>
> When doing client side SSL, this is only raised when the server sends a cipher
> suite that wasn't in our list

errSSLNegotiation is also raised when the server sends a
protocol version in ServerHello that the client doesn't
support, right?  See the sslVerifyProtVersion function in
libsecurity_ssl-38500/lib/sslUtils.c.

[Ryan Sleevi, 2010-10-26 03:33:52]

Thanks for reviewing. I've addressed all your feedback in the latest CL. As it
depends on the updated 3812007, there are a few changes for that rebase that
show up - I'm still trying to master git cl, so I apologize if I'm doing the CLs
wrong.

http://codereview.chromium.org/3845005/diff/1/5
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/1/5#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
On 2010/10/25 23:48:14, wtc wrote:
> errSSLNegotiation is also raised when the server sends a
> protocol version in ServerHello that the client doesn't
> support, right?  See the sslVerifyProtVersion function in
> libsecurity_ssl-38500/lib/sslUtils.c.

Sorry, I missed that in the first comment - it also is raised when version
negotiation fails - either due to being unimplemented or from the suite being
disabled.

For NSS, it raises, as best as I can tell from the sources,
SSL_ERROR_RX_MALFORMED_SERVER_HELLO if the MSB of the version is greater than it
supports, and SSL_ERROR_NO_CYPHER_OVERLAP if the LSB is greater than what it
supports (see ssl3_HandleServerHello in nss/lib/ssl/ssl3con.c). I'm not sure if
the _RX_MALFORMED_ is a bug/oversight in NSS, or if it's an intentional error
code. The code goes back to the initial CVS checkin in 2000.

My goal here was simply to keep the two implementations in sync, since now it
was possible for a unit test to cover them - and the ability to disable cipher
suites could cause this to surface. This ultimately causes the behaviours to
align with NSS, as far as I could trace the code paths of both. If it was
premature/should be separate, I'll back it out straight away.

http://codereview.chromium.org/3845005/diff/8001/9006
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/3845005/diff/8001/9006#newcode623
net/socket/ssl_client_socket_unittest.cc:623: if (rv !=
net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH &&
On 2010/10/25 23:45:04, wtc wrote:
> I think it's better to just test for
>   if (rv == net::ERR_SSL_PROTOCOL_ERROR) {
> here, and remove the
>     ASSERT_EQ(net::ERR_IO_PENDING, rv);
> on line 625.

I assume you meant ERR_IO_PENDING and updated as such.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode635
net/socket/ssl_client_socket_unittest.cc:635: // extra read) and
SSLClientSocketMac (which does not). Just make sure the
On 2010/10/25 23:45:04, wtc wrote:
> What's the extra read?  Just curious.

line 1650 of ssl_client_socket_nss.cc reads
  network_moved = DoTransportIO();

It runs after the handshake has failed in DoHandshake(), and tries to send and
receive the buffers. When the receive is called, it adds an extra event into the
net log.

Because of the very different state machine of the Mac implementation, it
doesn't have that event.

http://codereview.chromium.org/3845005/diff/8001/9006#newcode642
net/socket/ssl_client_socket_unittest.cc:642: // the socket when it encounters
an error, whereas other implementations
On 2010/10/25 23:45:04, wtc wrote:
> By "the socket", do you mean the transport socket?
> It seems wrong for sock->IsConnected() to be true for any
> SSLClientSocket implementation.  They are supposed to check
> the old completed_handshake_ member or the new
> completed_handshake() method.

I honestly didn't check. I saw the existing comments on lines 133-135, 178-180,
and 224-226 and accepted those as limitations. Each case is an SSL error, rather
than a socket error.

SVN blame traces it back to markus@chromium.org with http://crrev.com/12876 ,
which looks like it dates to when WinHTTP/WinInet were being used. Probably a
good candidate for a cleanup pass - as well as all the style nits with function
parameters in the existing tests.

[wtc, 2010-10-26 23:09:32]

LGTM.  I suggest some changes.

http://codereview.chromium.org/3845005/diff/20001/21005
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/20001/21005#newcode17
net/socket/ssl_client_socket_mac.cc:17: #include "base/values.h"
You can probably remove this header now.

http://codereview.chromium.org/3845005/diff/20001/21005#newcode204
net/socket/ssl_client_socket_mac.cc:204: case errSSLNegotiation:
This change is good.  Thanks!

http://codereview.chromium.org/3845005/diff/20001/21005#newcode1004
net/socket/ssl_client_socket_mac.cc:1004: bool closed_unexpectedly = (status ==
errSSLClosedAbort ||
This variable should be renamed, such as closed_ungracefully
or peer_aborted_handshake.  Otherwise, it contradicts the
comment on line 1008 "The server unexpectedly closed on us"
for errSSLClosedGraceful.

http://codereview.chromium.org/3845005/diff/20001/21005#newcode1010
net/socket/ssl_client_socket_mac.cc:1010: } else if (closed_unexpectedly &&
client_cert_requested_ &&
I find the new code harder to understand.  I'd like to
suggest one change:

This case and the next case on line 1016 can be combined:
  } else if (closed_unexpectedly && client_cert_requested_) {
    if (!ssl_config_.send_client_cert) {
      ...
      net_error = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
    } else {
      ...
      return ERR_BAD_SSL_CLIENT_AUTH_CERT;
    }
  } else {
  ...

[Ryan Sleevi, 2010-11-07 23:16:18]

wtc: I know you expressed concern separately as to the sorted list, I was hoping
you could go into that more? 

I moved the logic section you were concerned with back into a switch - my
concern was that it may look weird with the break appearing within the if, which
is why I added the extra comments to that fact. Other than that, were there any
remaining concerns?

[wtc, 2010-11-10 00:43:37]

LGTM.

I have a hard time reviewing this CL again in detail, so
please check it carefully yourself before checking this in.
Thanks!

I answer your question about my concern on sorting the disabled
cipher suite list in SSLClientSocketMac below.

http://codereview.chromium.org/3845005/diff/35001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/35001/net/base/ssl_config_service...
net/base/ssl_config_service.h:46: // first uint8 occupying the left-most byte.
Nit: left-most => most significant

http://codereview.chromium.org/3845005/diff/35001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3845005/diff/35001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:787:
Singleton<EnabledCipherSuites>::get()->ciphers();
My concern about the sorted disabled cipher list is really
about the complicated code (sort, remove_if, bsearch, and erase)
required to disabled cipher suites.  It shows that our API
for disabling a cipher suite has some "impedance mismatch"
with the Secure Transport API.

We can consider having the EnabledCipherSuites singleton
manage which ciphers should be disabled.  This will get rid
of the complicated code, but the downside is that we won't
be able to turn off a cipher suite just for a particular
SSL socket.  Maybe that's the least common denominator, as
I think SChannel has the same limitation?

In any case, this isn't a big deal; it's more of a personal
gut feeling.

[wtc, 2010-11-11 22:04:42]

http://codereview.chromium.org/3845005/diff/53001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/53001/net/base/ssl_config_service...
net/base/ssl_config_service.h:51: std::vector<uint16> disabled_cipher_suites;
I wonder if we should specify enabled_cipher_suites instead.
In general, a whitelist is better than a blacklist because
with a blacklist it's less clear exactly what is allowed.
We can interpret an empty enabled_cipher_suites list as
"all secure cipher suites are enabled".

I guess this depends on whether this is used for testing,
or for corporate/government regulation compliance.  For
testing, a whitelist is definitely more convenient.  For
corporate/government crypto regulation compliance, it's
less clear which is better.

[Ryan Sleevi, 2010-11-12 17:05:23]

http://codereview.chromium.org/3845005/diff/53001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/3845005/diff/53001/net/base/ssl_config_service...
net/base/ssl_config_service.h:51: std::vector<uint16> disabled_cipher_suites;
Right, this is what I was trying to address in that first message.

With opt-in, if any new suite is introduced, it will be hampered by admins who
have it in their enable list. Further, it introduces confusion if/when an
algorithm is decided insecure, but it appears in the enable list - does the
overall Chromium-dictated policy of minimum security win, and it's disabled, or
does it's presence in the enable list really mean that they want to use it
(perhaps a legacy app?)

On the positive side, with opt-in, you can also easily express ordering
preferences (which is where the disconnect for Mac comes from - the API
expresses both enabled + cipher preference order)

With opt-out, it's less clear what will actually be enabled, because it's the
intersection of [NSS supported] - [Chromium disabled] - [policy disabled]. Both
[NSS supported] and [Chromium disabled] may change between releases, so there
isn't a good measure there. It also makes it complicated to express ordering
preferences for those ciphers that remain available.

On the positive side, with opt-out, you can be very surgical with what you're
disabling, rather than having to enumerate the dozens and dozens of cipher
suites that "may" be supported.

I was planning on implementing the policy next, but it sounds like there is
still some lingering concern about the design/implementation. I just want to
confirm you're comfortable enough with this design to continue with the opt-out,
with the possibility of changing it/adding an opt-in later (whitelist +
blacklist approach) if needed.