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..1b79ea5b5c4b8c67a6fc5695b925bd2b61c2a608 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,53 +1217,45 @@ int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
         GetHostAndPort(request_->url));
   }
 
-  switch (error) {
-    case ERR_SSL_PROTOCOL_ERROR:
-    case ERR_SSL_VERSION_OR_CIPHER_MISMATCH:

[wtc, 2013/04/17 23:22:05]

It may be clearer to retain the original switch statement.

[thaidn_google, 2013/04/18 00:05:50]

Done.

-      if (server_ssl_config_.version_max >= SSL_PROTOCOL_VERSION_TLS1 &&
-          server_ssl_config_.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
-        // back to the next lower version and retry.
-        // NOTE: if the SSLClientSocket class doesn't support TLS 1.1,
-        // specifying TLS 1.1 in version_max will result in a TLS 1.0
-        // handshake, so falling back from TLS 1.1 to TLS 1.0 will simply
-        // 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;
-      }
-      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) {
-        // 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;
-      }
-      break;
+  uint16 version_max = server_ssl_config_.version_max;
+
+  if ((error == ERR_SSL_PROTOCOL_ERROR ||
+       error == ERR_SSL_VERSION_OR_CIPHER_MISMATCH) &&
+      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
+    // back to the next lower version and retry.
+    // NOTE: if the SSLClientSocket class doesn't support TLS 1.1,
+    // specifying TLS 1.1 in version_max will result in a TLS 1.0
+    // handshake, so falling back from TLS 1.1 to TLS 1.0 will simply
+    // 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.
+    version_max--;
+
+    // While SSL 3.0 fallback should be eliminated because of security reasons,
+    // there is a high risk of breaking the servers if this is done in general.
+    // For now SSL 3.0 fallback is disabled for Google servers first, and will
+    // be expanded to other servers after enough experiences have been gained
+    // showing that this experiment works well with today's Internet.
+    if (version_max == SSL_PROTOCOL_VERSION_SSL3 &&

[wtc, 2013/04/17 23:22:05]

BUG?: I think this should be
    if (version_max > SSL_PROTOCOL_VERSION_SSL3 ||

[thaidn_google, 2013/04/18 00:05:50]

No, it isn't a bug (otherwise tests have failed).

The statements inside the if clause will cause the fallback. So we need to test
two things:

* The protocol that we are going to use for the fallback would be
SSL_PROTOCOL_VERSION_SSL3; and

* Either fallback is enabled or this is not a Google site.

On 2013/04/17 23:22:05, wtc wrote:

[wtc, 2013/04/18 18:23:09]

The unit test HTTPSRequestTest.TLSv1Fallback in
net/url_request/url_request_unittest.cc should have caught
this. Do you know why that test didn't fail? Perhaps you
didn't run all the tests in net_unittests?

+        (server_ssl_config_.ssl3_version_fallback_enabled ||
+         !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;
 }