Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138813003:
Use cert config options in SSLServerSocketOpenSSL. (Closed)
|
Created:
5 years, 7 months ago by Sergey Ulanov Modified:
5 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse 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}
Committed: https://crrev.com/ff826d5ebbddde14ca7e813b5c86a76d7ec6519d
Cr-Commit-Position: refs/heads/master@{#329707}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 32 (9 generated)
Patchset #1 (id:1) has been deleted
sergeyu@chromium.org changed reviewers: + davidben@chromium.org
Unit test? Unfortunately, our API for controlling the SSLClientSocket ciphers to test against is pretty awful, but you can just blacklist all the ECDHE ciphers we expose. A server with require_ecdhe set should fail to handshake with ERR_SSL_VERSION_OR_CIPHER_MISMATCH. I believe this should be all the ciphers you need to blacklist on the SSLClientSocket you're testing with: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b) TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f) TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (0xcc14) TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcc13) TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA (0xc00a) TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014) TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA (0xc009) TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013) TLS_ECDHE_ECDSA_WITH_RC4_128_SHA (0xc007) TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011) I imagine we'll need to adjust it later as we add ciphers, but oh well. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:704: const uint16 id = static_cast<uint16>(SSL_CIPHER_get_id(cipher)); uint16_t, not uint16. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:710: bool disable = SSL_CIPHER_get_bits(cipher, NULL) < 80; This is unnecessary. BoringSSL doesn't expose any of those ciphers. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:711: if (!disable && ssl_config_.require_forward_secrecy) { Let's just do the rename to require_ecdhe. There's only a handful of consumers, so it shouldn't trouble the merge. https://code.google.com/p/chromium/codesearch#search/&q=require_forward_secre... https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:712: disable = !StartsWithASCII(name, "ECDHE", true); Yuck. OpenSSL doesn't give very good APIs for this, but let's do strcmp(SSL_CIPHER_get_kx_name(cipher), "ECDHE_RSA") == 0 || strcmp(SSL_CIPHER_get_kx_name(cipher), "ECDHE_ECDSA") == 0 That seems slightly more direct than trying to parse out the names like this. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:728: command.append(":!RC4"); This isn't implemented in SSLServerSocketNSS. If we're going to make that flag meaningful on the server, it should be mirrored in SSLClientSocketNSS. (Or we can leave the flag client-only for now.)
PTAL. Added unittest as you suggested https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:704: const uint16 id = static_cast<uint16>(SSL_CIPHER_get_id(cipher)); On 2015/05/12 00:00:50, David Benjamin wrote: > uint16_t, not uint16. Done. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:710: bool disable = SSL_CIPHER_get_bits(cipher, NULL) < 80; On 2015/05/12 00:00:50, David Benjamin wrote: > This is unnecessary. BoringSSL doesn't expose any of those ciphers. I copied most of this code from ssl_client_socket_openssl.cc . So perhaps this line can be removed from there too. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:711: if (!disable && ssl_config_.require_forward_secrecy) { On 2015/05/12 00:00:50, David Benjamin wrote: > Let's just do the rename to require_ecdhe. There's only a handful of consumers, > so it shouldn't trouble the merge. > > https://code.google.com/p/chromium/codesearch#search/&q=require_forward_secre... Done. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:712: disable = !StartsWithASCII(name, "ECDHE", true); On 2015/05/12 00:00:50, David Benjamin wrote: > Yuck. OpenSSL doesn't give very good APIs for this, but let's do > > strcmp(SSL_CIPHER_get_kx_name(cipher), "ECDHE_RSA") == 0 || > strcmp(SSL_CIPHER_get_kx_name(cipher), "ECDHE_ECDSA") == 0 > > That seems slightly more direct than trying to parse out the names like this. Done. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:728: command.append(":!RC4"); On 2015/05/12 00:00:50, David Benjamin wrote: > This isn't implemented in SSLServerSocketNSS. If we're going to make that flag > meaningful on the server, it should be mirrored in SSLClientSocketNSS. (Or we > can leave the flag client-only for now.) left it client-only - added !RC4 to the default command https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:699: "DEFAULT:!NULL:!aNULL:!SHA256:!SHA384:!aECDH:!AESGCM+AES256:!aPSK:!RC4"); This is the same string that's used for client sockets with "!RC4" added at the end. But can this be changed to "DEFAULT" now with boringssl?
Thanks for adding the test! Just a handful more comments. https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/20001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:710: bool disable = SSL_CIPHER_get_bits(cipher, NULL) < 80; On 2015/05/12 18:55:46, Sergey Ulanov wrote: > On 2015/05/12 00:00:50, David Benjamin wrote: > > This is unnecessary. BoringSSL doesn't expose any of those ciphers. > > I copied most of this code from ssl_client_socket_openssl.cc . So perhaps this > line can be removed from there too. Yeah, I should go prune quite a lot of that code... :-) https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:699: "DEFAULT:!NULL:!aNULL:!SHA256:!SHA384:!aECDH:!AESGCM+AES256:!aPSK:!RC4"); On 2015/05/12 18:55:47, Sergey Ulanov wrote: > This is the same string that's used for client sockets with "!RC4" added at the > end. > But can this be changed to "DEFAULT" now with boringssl? It can be pruned significantly, but we do still want to prune out !SHA256:!SHA384:!AESGCM+AES256. Probably also worth pruning out !aPSK even though BoringSSL will do that automatically by virtue of not configuring PSK options. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:699: "DEFAULT:!NULL:!aNULL:!SHA256:!SHA384:!aECDH:!AESGCM+AES256:!aPSK:!RC4"); The !RC4 shouldn't be there. It's still enabled on the NSS side. It might be worth pruning that from SSLServerSocket, but we should do that separately as it'd need to be mirrored on NSS as well. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:705: const char* name = SSL_CIPHER_get_name(cipher); Nit: This line can be moved down to the block in line 717. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:709: StringPiece kx_name(SSL_CIPHER_get_kx_name(cipher)); Shouldn't this be base::StringPiece? https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:710: disable = kx_name != "ECDHE_RSA") && kx_name != "ECDHE_ECDSA"; Stray )? https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:597: client_ssl_config_.disabled_cipher_suites = { This depends on C++11 initializer list support in std::vector. I would love for this code to use them, but we can't assume C++11 library support yet. (Hopefully this will change soonish? I hear Chrome for Android recently switched to libc++. Dunno if there are other blockers.) https://chromium-cpp.appspot.com/ https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:625: } You can replace these with client_ret = connect_callback.GetResult(client_ret); server_ret = handshake_callback.GetResult(server_ret);
https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:699: "DEFAULT:!NULL:!aNULL:!SHA256:!SHA384:!aECDH:!AESGCM+AES256:!aPSK:!RC4"); On 2015/05/12 19:05:16, David Benjamin wrote: > On 2015/05/12 18:55:47, Sergey Ulanov wrote: > > This is the same string that's used for client sockets with "!RC4" added at > the > > end. > > But can this be changed to "DEFAULT" now with boringssl? > > It can be pruned significantly, but we do still want to prune out > !SHA256:!SHA384:!AESGCM+AES256. Probably also worth pruning out !aPSK even > though BoringSSL will do that automatically by virtue of not configuring PSK > options. Done. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:699: "DEFAULT:!NULL:!aNULL:!SHA256:!SHA384:!aECDH:!AESGCM+AES256:!aPSK:!RC4"); On 2015/05/12 19:05:16, David Benjamin wrote: > The !RC4 shouldn't be there. It's still enabled on the NSS side. It might be > worth pruning that from SSLServerSocket, but we should do that separately as > it'd need to be mirrored on NSS as well. Done. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:705: const char* name = SSL_CIPHER_get_name(cipher); On 2015/05/12 19:05:16, David Benjamin wrote: > Nit: This line can be moved down to the block in line 717. Done. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:709: StringPiece kx_name(SSL_CIPHER_get_kx_name(cipher)); On 2015/05/12 19:05:16, David Benjamin wrote: > Shouldn't this be base::StringPiece? Done. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:710: disable = kx_name != "ECDHE_RSA") && kx_name != "ECDHE_ECDSA"; On 2015/05/12 19:05:16, David Benjamin wrote: > Stray )? Apparently I was trying to compile this code with use_openssl=0 :( https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:597: client_ssl_config_.disabled_cipher_suites = { On 2015/05/12 19:05:16, David Benjamin wrote: > This depends on C++11 initializer list support in std::vector. I would love for > this code to use them, but we can't assume C++11 library support yet. (Hopefully > this will change soonish? I hear Chrome for Android recently switched to libc++. > Dunno if there are other blockers.) > > https://chromium-cpp.appspot.com/ Done. Thanks for catching it - for some reason I thought it's already allowed. https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:625: } On 2015/05/12 19:05:16, David Benjamin wrote: > You can replace these with > > client_ret = connect_callback.GetResult(client_ret); > server_ret = handshake_callback.GetResult(server_ret); Done.
lgtm https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/40001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:597: client_ssl_config_.disabled_cipher_suites = { On 2015/05/12 19:42:21, Sergey Ulanov wrote: > On 2015/05/12 19:05:16, David Benjamin wrote: > > This depends on C++11 initializer list support in std::vector. I would love > for > > this code to use them, but we can't assume C++11 library support yet. > (Hopefully > > this will change soonish? I hear Chrome for Android recently switched to > libc++. > > Dunno if there are other blockers.) > > > > https://chromium-cpp.appspot.com/ > > Done. Thanks for catching it - for some reason I thought it's already allowed. Be glad you don't have to do this nonsense. :-) Looking forward to ditching it... https://boringssl.googlesource.com/boringssl/+/master/crypto/test/stl_compat.h
Oh, last minor nit: change "cert" to "cipher suite" in the description. Those are definitely things.
On 2015/05/12 19:46:48, David Benjamin wrote: > Oh, last minor nit: change "cert" to "cipher suite" in the description. Those > are definitely things. *Those are different things. I can't type.
On 2015/05/12 19:46:48, David Benjamin wrote: > Oh, last minor nit: change "cert" to "cipher suite" in the description. Those > are definitely things. Yes, done
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138813003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1138813003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138813003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d0eae58087e6f45088d6ef349d9ebaa2da450ea1 Cr-Commit-Position: refs/heads/master@{#329528}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:80001) has been created in https://codereview.chromium.org/1131573005/ by benwells@chromium.org. The reason for reverting is: 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%28v... 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..
Message was sent while issue was closed.
whesse@google.com changed reviewers: + whesse@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:624: server_ret = handshake_callback.GetResult(client_ret); Drive-by comment - should this really be client_ret, not server_ret?
Message was sent while issue was closed.
https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:624: server_ret = handshake_callback.GetResult(client_ret); On 2015/05/13 12:04:01, Bill Hesse wrote: > Drive-by comment - should this really be client_ret, not server_ret? Erf, I should have noticed that. Yes it should. Though if it fixes the leak, that still suggests an underlying bug in SSLServerSocketNSS or NSS itself (neither of which would surprise me much).
Message was sent while issue was closed.
https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:624: server_ret = handshake_callback.GetResult(client_ret); On 2015/05/13 15:09:23, David Benjamin wrote: > On 2015/05/13 12:04:01, Bill Hesse wrote: > > Drive-by comment - should this really be client_ret, not server_ret? > > Erf, I should have noticed that. Yes it should. Though if it fixes the leak, > that still suggests an underlying bug in SSLServerSocketNSS or NSS itself > (neither of which would surprise me much). (Yes it should be server_ret, that is.)
https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:624: server_ret = handshake_callback.GetResult(client_ret); On 2015/05/13 15:09:23, David Benjamin wrote: > On 2015/05/13 12:04:01, Bill Hesse wrote: > > Drive-by comment - should this really be client_ret, not server_ret? > > Erf, I should have noticed that. Yes it should. Though if it fixes the leak, > that still suggests an underlying bug in SSLServerSocketNSS or NSS itself > (neither of which would surprise me much). Fixed now and looks like it does fix the leak.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1138813003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138813003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ff826d5ebbddde14ca7e813b5c86a76d7ec6519d Cr-Commit-Position: refs/heads/master@{#329707}
Message was sent while issue was closed.
On 2015/05/13 18:18:02, Sergey Ulanov wrote: > https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... > File net/socket/ssl_server_socket_unittest.cc (right): > > https://codereview.chromium.org/1138813003/diff/80001/net/socket/ssl_server_s... > net/socket/ssl_server_socket_unittest.cc:624: server_ret = > handshake_callback.GetResult(client_ret); > On 2015/05/13 15:09:23, David Benjamin wrote: > > On 2015/05/13 12:04:01, Bill Hesse wrote: > > > Drive-by comment - should this really be client_ret, not server_ret? > > > > Erf, I should have noticed that. Yes it should. Though if it fixes the leak, > > that still suggests an underlying bug in SSLServerSocketNSS or NSS itself > > (neither of which would surprise me much). > > Fixed now and looks like it does fix the leak. Actually the problem is still there - it's detected by valgrind, but not ASan and that's why I thought it was fixed. The problem seems to be that ssl3_HandleClientHello() doesn't free sid when it returns error, particularly when it fails with SSL_ERROR_NO_CYPHER_OVERLAP. I.e. the bug is in NSS |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
