Do not roll back to SSL 3.0 for Google properties.

net/http/http_network_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <set>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 29 matching lines @@
 #include "net/http/http_proxy_client_socket_pool.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
 #include "net/http/http_server_properties.h"
 #include "net/http/http_status_code.h"
 #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"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/ssl_client_socket_pool.h"
 #include "net/socket/transport_client_socket_pool.h"
 #include "net/spdy/spdy_http_stream.h"
 #include "net/spdy/spdy_session.h"
 #include "net/spdy/spdy_session_pool.h"
 #include "net/ssl/ssl_cert_request_info.h"
@@ skipping 105 matching lines @@
 
 int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info,
                                   const CompletionCallback& callback,
                                   const BoundNetLog& net_log) {
   SIMPLE_STATS_COUNTER("HttpNetworkTransaction.Count");
 
   net_log_ = net_log;
   request_ = request_info;
   start_time_ = base::Time::Now();
 
   if (request_->load_flags & LOAD_DISABLE_CERT_REVOCATION_CHECKING) {

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

IMPORTANT: the name "ssl_version_min_preloaded_disabled"
suggests that we can simply change server_ssl_config_.version_min
here, as soon as request_->url is available and we can
call TransportSecurityState::IsGooglePinnedProperty().

I think this alternative implementation would be better.

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

This was my original approach. Chris pointed out that there may be MITM proxies
that only implement SSLv3. Preventing fallback would allow them to continue to
work, but setting the minimum version breaks them.

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

     server_ssl_config_.rev_checking_enabled = false;
     proxy_ssl_config_.rev_checking_enabled = false;
   }
 
   next_state_ = STATE_CREATE_STREAM;
   int rv = DoLoop(OK);
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
   return rv;
 }
@@ skipping 1023 matching lines @@
 // by the endpoint host, request_->url, rather than considering if they were
 // generated by the SSL proxy. http://crbug.com/69329
 int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
   DCHECK(request_);
   if (server_ssl_config_.send_client_cert &&
       (error == ERR_SSL_PROTOCOL_ERROR || IsClientCertificateError(error))) {
     session_->ssl_client_auth_cache()->Remove(
         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
         // 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;
+        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;
 }
 
 // This method determines whether it is safe to resend the request after an
 // IO error.  It can only be called in response to request header or body
 // write errors or response header read errors.  It should not be used in
 // other cases, such as a Connect error.
 int HttpNetworkTransaction::HandleIOError(int error) {
   // SSL errors may happen at any time during the stream and indicate issues
   // with the underlying connection. Because the peer may request
@@ skipping 185 matching lines @@
       description = base::StringPrintf("Unknown state 0x%08X (%u)", state,
                                        state);
       break;
   }
   return description;
 }
 
 #undef STATE_CASE
 
 }  // namespace net