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}

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<char*>(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<StreamSocket> transport(
+      new TCPClientSocket(addr(), &log_, NetLog::Source()));
+  EXPECT_EQ(OK, callback.GetResult(transport->Connect(callback.callback())));
+  scoped_ptr<SSLClientSocket> 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) {