From 21ea1b4ef8d6c111605866108c7dac1b31440114 Mon Sep 17 00:00:00 2001 From: davidben Date: Mon, 23 Feb 2015 10:00:37 -0800 Subject: Shard the SSL session cache by version fallback. This addresses two issues: - NSS clamps client_version to the session version. This means that a successful fallback connection is effectively cached, despite our fallback being stateless. This causing our metrics to be under-reported and, more problematic, makes spurious fallbacks stick. - BoringSSL does not clamp, but many versions of OpenSSL on the server will happily resume older sessions at newer protocol versions, rather than doing a full handshake at the newer protocol version. This means a successful spurious fallback causes us later resume with a weaker cipher than we should. Moreover, this mismatch is forbidden by every other client implementation. The metrics are reporting 0.06% of connections on beta channel hit this case. I expect it to go down after this change. Note: this will also increase traffic to version-intolerant servers on NSS ports. But that's only Linux/CrOS/iOS now and the BoringSSL switch did the same thing by losing the version clamp. BUG=459690,441456 Review URL: https://codereview.chromium.org/947603002 Cr-Commit-Position: refs/heads/master@{#317605} --- net/socket/ssl_client_socket_nss.cc | 28 +++++++++--- net/socket/ssl_client_socket_openssl.cc | 21 +++++++++ net/socket/ssl_client_socket_unittest.cc | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc index 1cc156b..18f100a 100644 --- a/net/socket/ssl_client_socket_nss.cc +++ b/net/socket/ssl_client_socket_nss.cc @@ -3356,13 +3356,29 @@ int SSLClientSocketNSS::InitializeSSLPeerName() { // SSL tunnel through a proxy -- GetPeerName returns the proxy's address // rather than the destination server's address in that case. std::string peer_id = host_and_port_.ToString(); - // If the ssl_session_cache_shard_ is non-empty, we append it to the peer id. - // This will cause session cache misses between sockets with different values - // of ssl_session_cache_shard_ and this is used to partition the session cache - // for incognito mode. - if (!ssl_session_cache_shard_.empty()) { - peer_id += "/" + ssl_session_cache_shard_; + // Append |ssl_session_cache_shard_| to the peer id. This is used to partition + // the session cache for incognito mode. + peer_id += "/" + ssl_session_cache_shard_; + peer_id += "/"; + // Shard the session cache based on maximum protocol version. This causes + // fallback connections to use a separate session cache. + switch (ssl_config_.version_max) { + case SSL_PROTOCOL_VERSION_SSL3: + peer_id += "ssl3"; + break; + case SSL_PROTOCOL_VERSION_TLS1: + peer_id += "tls1"; + break; + case SSL_PROTOCOL_VERSION_TLS1_1: + peer_id += "tls1.1"; + break; + case SSL_PROTOCOL_VERSION_TLS1_2: + peer_id += "tls1.2"; + break; + default: + NOTREACHED(); } + SECStatus rv = SSL_SetSockPeerID(nss_fd_, const_cast(peer_id.c_str())); if (rv != SECSuccess) LogFailedNSSFunction(net_log_, "SSL_SetSockPeerID", peer_id.c_str()); diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc index f166b69..0389375 100644 --- a/net/socket/ssl_client_socket_openssl.cc +++ b/net/socket/ssl_client_socket_openssl.cc @@ -402,6 +402,27 @@ std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const { std::string result = host_and_port_.ToString(); result.append("/"); result.append(ssl_session_cache_shard_); + + // Shard the session cache based on maximum protocol version. This causes + // fallback connections to use a separate session cache. + result.append("/"); + switch (ssl_config_.version_max) { + case SSL_PROTOCOL_VERSION_SSL3: + result.append("ssl3"); + break; + case SSL_PROTOCOL_VERSION_TLS1: + result.append("tls1"); + break; + case SSL_PROTOCOL_VERSION_TLS1_1: + result.append("tls1.1"); + break; + case SSL_PROTOCOL_VERSION_TLS1_2: + result.append("tls1.2"); + break; + default: + NOTREACHED(); + } + return result; } diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc index 4486810..2390bd5 100644 --- a/net/socket/ssl_client_socket_unittest.cc +++ b/net/socket/ssl_client_socket_unittest.cc @@ -29,6 +29,8 @@ #include "net/ssl/default_channel_id_store.h" #include "net/ssl/ssl_cert_request_info.h" #include "net/ssl/ssl_config_service.h" +#include "net/ssl/ssl_connection_status_flags.h" +#include "net/ssl/ssl_info.h" #include "net/test/cert_test_util.h" #include "net/test/spawned_test_server/spawned_test_server.h" #include "testing/gmock/include/gmock/gmock.h" @@ -2813,6 +2815,77 @@ TEST_F(SSLClientSocketTest, ReuseStates) { // attempt to read one byte extra. } +// Tests that session caches are sharded by max_version. +TEST_F(SSLClientSocketTest, FallbackShardSessionCache) { + SpawnedTestServer::SSLOptions ssl_options; + ASSERT_TRUE(StartTestServer(ssl_options)); + + // Prepare a normal and fallback SSL config. + SSLConfig ssl_config; + SSLConfig fallback_ssl_config; + fallback_ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1; + fallback_ssl_config.version_fallback = true; + + // Connect with a fallback config from the test server to add an entry to the + // session cache. + TestCompletionCallback callback; + scoped_ptr transport( + new TCPClientSocket(addr(), &log_, NetLog::Source())); + EXPECT_EQ(OK, callback.GetResult(transport->Connect(callback.callback()))); + scoped_ptr sock(CreateSSLClientSocket( + transport.Pass(), test_server()->host_port_pair(), fallback_ssl_config)); + EXPECT_EQ(OK, callback.GetResult(sock->Connect(callback.callback()))); + SSLInfo ssl_info; + EXPECT_TRUE(sock->GetSSLInfo(&ssl_info)); + EXPECT_EQ(SSLInfo::HANDSHAKE_FULL, ssl_info.handshake_type); + EXPECT_EQ(SSL_CONNECTION_VERSION_TLS1, + SSLConnectionStatusToVersion(ssl_info.connection_status)); + + // A non-fallback connection needs a full handshake. + transport.reset(new TCPClientSocket(addr(), &log_, NetLog::Source())); + EXPECT_EQ(OK, callback.GetResult(transport->Connect(callback.callback()))); + sock = CreateSSLClientSocket(transport.Pass(), + test_server()->host_port_pair(), ssl_config); + EXPECT_EQ(OK, callback.GetResult(sock->Connect(callback.callback()))); + EXPECT_TRUE(sock->GetSSLInfo(&ssl_info)); + EXPECT_EQ(SSLInfo::HANDSHAKE_FULL, ssl_info.handshake_type); + // This does not check for equality because TLS 1.2 support is conditional on + // system NSS features. + EXPECT_LT(SSL_CONNECTION_VERSION_TLS1, + SSLConnectionStatusToVersion(ssl_info.connection_status)); + + // Note: if the server (correctly) declines to resume a TLS 1.0 session at TLS + // 1.2, the above test would not be sufficient to prove the session caches are + // sharded. Implementations vary here, so, to avoid being sensitive to this, + // attempt to resume with two more connections. + + // The non-fallback connection added a > TLS 1.0 entry to the session cache. + transport.reset(new TCPClientSocket(addr(), &log_, NetLog::Source())); + EXPECT_EQ(OK, callback.GetResult(transport->Connect(callback.callback()))); + sock = CreateSSLClientSocket(transport.Pass(), + test_server()->host_port_pair(), ssl_config); + EXPECT_EQ(OK, callback.GetResult(sock->Connect(callback.callback()))); + EXPECT_TRUE(sock->GetSSLInfo(&ssl_info)); + EXPECT_EQ(SSLInfo::HANDSHAKE_RESUME, ssl_info.handshake_type); + // This does not check for equality because TLS 1.2 support is conditional on + // system NSS features. + EXPECT_LT(SSL_CONNECTION_VERSION_TLS1, + SSLConnectionStatusToVersion(ssl_info.connection_status)); + + // The fallback connection still resumes from its session cache. It cannot + // offer the > TLS 1.0 session, so this must have been the session from the + // first fallback connection. + transport.reset(new TCPClientSocket(addr(), &log_, NetLog::Source())); + EXPECT_EQ(OK, callback.GetResult(transport->Connect(callback.callback()))); + sock = CreateSSLClientSocket( + transport.Pass(), test_server()->host_port_pair(), fallback_ssl_config); + EXPECT_EQ(OK, callback.GetResult(sock->Connect(callback.callback()))); + EXPECT_TRUE(sock->GetSSLInfo(&ssl_info)); + EXPECT_EQ(SSLInfo::HANDSHAKE_RESUME, ssl_info.handshake_type); + EXPECT_EQ(SSL_CONNECTION_VERSION_TLS1, + SSLConnectionStatusToVersion(ssl_info.connection_status)); +} + #if defined(USE_OPENSSL) TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithFailure) { -- cgit v1.1