Do not roll back to SSL 3.0 for Google properties.

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 467eb940a65d7a6687b7085b3e9257f615036ab5..506a51b1d40d3475f5b5632910156e333e6e3faf 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -47,6 +47,7 @@
 #include "net/http/http_stream_base.h"
 #include "net/http/http_stream_factory.h"
 #include "net/http/http_util.h"
+#include "net/http/transport_security_state.h"
 #include "net/http/url_security_manager.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/socks_client_socket_pool.h"
@@ -1216,11 +1217,13 @@ int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
         GetHostAndPort(request_->url));
   }
 
+  uint16 version_max = server_ssl_config_.version_max;
+
   switch (error) {
     case ERR_SSL_PROTOCOL_ERROR:
     case ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
-      if (server_ssl_config_.version_max >= SSL_PROTOCOL_VERSION_TLS1 &&
-          server_ssl_config_.version_max > server_ssl_config_.version_min) {
+      if (version_max >= SSL_PROTOCOL_VERSION_TLS1 &&
+          version_max > server_ssl_config_.version_min) {
         // This could be a TLS-intolerant server or a server that chose a
         // cipher suite defined only for higher protocol versions (such as
         // an SSL 3.0 server that chose a TLS-only cipher suite).  Fall
@@ -1231,38 +1234,38 @@ int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
         // repeat the TLS 1.0 handshake. To avoid this problem, the default
         // version_max should match the maximum protocol version supported
         // by the SSLClientSocket class.
-        uint16 version_before = server_ssl_config_.version_max;
-        server_ssl_config_.version_max--;
-        net_log_.AddEvent(
-            NetLog::TYPE_SSL_VERSION_FALLBACK,
-            base::Bind(&NetLogSSLVersionFallbackCallback,
-                       &request_->url, error, version_before,
-                       server_ssl_config_.version_max));
-        server_ssl_config_.version_fallback = true;
-        ResetConnectionAndRequestForResend();
-        error = OK;
+        version_max--;
       }
       break;
     case ERR_SSL_DECOMPRESSION_FAILURE_ALERT:
     case ERR_SSL_BAD_RECORD_MAC_ALERT:
-      if (server_ssl_config_.version_max >= SSL_PROTOCOL_VERSION_TLS1 &&
-          server_ssl_config_.version_min == SSL_PROTOCOL_VERSION_SSL3) {

[wtc, 2013/04/17 19:49:50]

BUG?: The original code uses server_ssl_config_.version_min
here. Did you intend to change it to version_max?

Since we no longer do TLS compression, this code is now
obsolete and can be simply deleted.

+      if (version_max >= SSL_PROTOCOL_VERSION_TLS1 &&
+          version_max == SSL_PROTOCOL_VERSION_SSL3) {
         // This could be a server with buggy DEFLATE support. Turn off TLS,
         // DEFLATE support and retry.
         // TODO(wtc): turn off DEFLATE support only. Do not tie it to TLS.
-        uint16 version_before = server_ssl_config_.version_max;
-        server_ssl_config_.version_max = SSL_PROTOCOL_VERSION_SSL3;
-        net_log_.AddEvent(
-            NetLog::TYPE_SSL_VERSION_FALLBACK,
-            base::Bind(&NetLogSSLVersionFallbackCallback,
-                       &request_->url, error, version_before,
-                       server_ssl_config_.version_max));
-        server_ssl_config_.version_fallback = true;
-        ResetConnectionAndRequestForResend();
-        error = OK;
+        version_max = SSL_PROTOCOL_VERSION_SSL3;
       }
       break;
   }
+
+  if (version_max != server_ssl_config_.version_max &&
+      // Do not fallback to SSL 3.0 on Google properties.

[wtc, 2013/04/17 19:49:50]

I suggest moving this comment before the if statement to
avoid mixing comments with conditional expressions.

This change is borderline "optimizing Chrome for Google
servers". I think we should clarify this in this comment.
Perhaps we will experiment with Google servers first and
expand this to other servers after we have gained enough
experience. Perhaps there is a high risk of breaking the
servers so we are uncomfortable doing this in general.

[thaidn_google, 2013/04/17 22:16:07]

Done.

+      !(version_max <= SSL_PROTOCOL_VERSION_SSL3 &&
+        !server_ssl_config_.ssl_version_min_preloaded_disabled &&

[wtc, 2013/04/17 19:49:50]

I think "hsts" should be part of this member's name. I had
no idea what "preloaded" means.

It is a little confusing to use "version_min" in this member's
name because it is actually used in conjunction with version_max
here. This is another reason to consider the alternative
implementation.

Perhaps this member should be named something like
"strict_ssl_version_fallback_disabled".

[thaidn_google, 2013/04/17 22:16:07]

I rename it to ssl3_version_fallback_enabled.

I chose the old name because it was part of a bigger plan to induce minimum SSL
version from HSTS preloaded configuration.

On 2013/04/17 19:49:50, wtc wrote:

+        TransportSecurityState::IsGooglePinnedProperty(
+            request_->url.host(), true /* include SNI */))) {
+    net_log_.AddEvent(
+        NetLog::TYPE_SSL_VERSION_FALLBACK,
+        base::Bind(&NetLogSSLVersionFallbackCallback,
+                   &request_->url, error, server_ssl_config_.version_max,
+                   version_max));
+    server_ssl_config_.version_max = version_max;
+    server_ssl_config_.version_fallback = true;
+    ResetConnectionAndRequestForResend();
+    error = OK;
+  }
+
   return error;
 }