Do not roll back to SSL 3.0 for Google properties.

net/http/http_network_transaction_ssl_unittest.cc

Index: net/http/http_network_transaction_ssl_unittest.cc
diff --git a/net/http/http_network_transaction_ssl_unittest.cc b/net/http/http_network_transaction_ssl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..662fc497a0a9bc5915dc7525b899704922be12fb
--- /dev/null
+++ b/net/http/http_network_transaction_ssl_unittest.cc
@@ -0,0 +1,206 @@
+// 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 <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "net/base/net_util.h"
+#include "net/base/request_priority.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/http/http_auth_handler_mock.h"
+#include "net/http/http_network_session.h"
+#include "net/http/http_network_transaction.h"
+#include "net/http/http_request_info.h"
+#include "net/http/http_server_properties_impl.h"
+#include "net/http/transport_security_state.h"
+#include "net/proxy/proxy_service.h"
+#include "net/socket/socket_test_util.h"
+#include "net/ssl/ssl_config_service_defaults.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+class HttpNetworkTransactionSSLTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
+    ssl_config_ = new SSLConfigServiceDefaults;
+    session_params_.ssl_config_service = ssl_config_.get();
+
+    auth_handler_factory_.reset(new HttpAuthHandlerMock::Factory());
+    session_params_.http_auth_handler_factory = auth_handler_factory_.get();
+
+    proxy_service_.reset(ProxyService::CreateDirect());
+    session_params_.proxy_service = proxy_service_.get();
+
+    session_params_.host_resolver = &mock_resolver_;
+    session_params_.http_server_properties = &http_server_properties_;
+    session_params_.transport_security_state = &transport_security_state_;
+  }
+
+  HttpRequestInfo* GetRequestInfo(std::string url) {
+    HttpRequestInfo* request_info = new HttpRequestInfo;
+    request_info->url = GURL(url);
+    request_info->method = "GET";
+    return request_info;
+  }
+
+  SSLConfig& GetServerSSLConfig(HttpNetworkTransaction* trans) {
+    return trans->server_ssl_config_;
+  }
+
+  scoped_refptr<SSLConfigService> ssl_config_;
+  scoped_ptr<HttpAuthHandlerMock::Factory> auth_handler_factory_;
+  scoped_ptr<ProxyService> proxy_service_;
+
+  MockHostResolver mock_resolver_;
+  HttpServerPropertiesImpl http_server_properties_;
+  TransportSecurityState transport_security_state_;
+  HttpNetworkSession::Params session_params_;
+};
+
+TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedEnabled) {
+  MockClientSocketFactory mock_socket_factory;

[Ryan Sleevi, 2013/04/17 01:16:36]

Seems like all your tests just end up creating a mock_socket_factory and
assigning it to session_params_ (eg: instead of lines 64 & 79)

Should you just add this to the test harness and add it to params on line 39/40?
Then you only have to do

mock_socket_factory_.AddSSLSocketDataProvider(...)
mock_socket_factory_.AddSocketDataProvider(...)

[thaidn_google, 2013/04/17 04:34:47]

Nice tip. Thanks!

On 2013/04/17 01:16:36, Ryan Sleevi wrote:

+
+  // |ssl_data| contains the data for the first TLS 1.0 handshake that will
+  // return |ERR_SSL_PROTOCOL_ERROR| to trigger the SSL 3.0 fallback logic in
+  // |HttpNetworkTransaction|.

[Ryan Sleevi, 2013/04/17 01:16:36]

comment style nit: While the || notation isn't formalized, AFAIK, Chromium code
only uses it to refer to variable names, and not to things like type names/class
names or enum values and the like.

[thaidn_google, 2013/04/17 04:34:47]

Good to know. Thanks!

On 2013/04/17 01:16:36, Ryan Sleevi wrote:

+
+  // Note: When a connection to a server fails during a handshake,
+  // |HttpNetworkTransaction| will attempt to fallback to SSL 3.0 if the
+  // previous connection was attempted with TLS 1.0. Since that behavior is

[Ryan Sleevi, 2013/04/17 01:16:36]

comment nit: Two bits:
1) This is explaining a concept introduced on lines 66-68, but not yet
explained. It would be good to move the explanation of what is "the SSL 3.0
fallback logic" into that comment.
2) Correctness nit: It's not unconditionally going to rollback to SSL3.0 - only
if it's enabled.

An easier way to address these comment nits might be to move the test
description up to line 62 and provide the high-level summary of the test, and
then briefly explain points that matter

(for line 62)
// Tests that the HttpNetworkTransaction does not attempt to
// fall back to SSL 3.0 when a TLS 1.0 handshake fails and
// the site is pinned to the Google pin list (indicating that
// it is a Google site).

then line 66
// |ssl_data| is for the first handshake (TLS 1.0), which
// will fail for protocol reasons (eg: simulating a version
// rollback attack).
// Because SSL 3.0 fallback should never even be attempted,
// only one simulated SSL handshake is supported.

This generally applies to all tests in this file.

[thaidn_google, 2013/04/17 04:34:47]

I hope the modified comments LGTY :-).

On 2013/04/17 01:16:36, Ryan Sleevi wrote:

+  // disabled for Google properties, there is only one handshake data here.
+  SSLSocketDataProvider ssl_data(ASYNC, ERR_SSL_PROTOCOL_ERROR);
+  mock_socket_factory.AddSSLSocketDataProvider(&ssl_data);
+  net::StaticSocketDataProvider data(NULL, 0, NULL, 0);

[Ryan Sleevi, 2013/04/17 01:16:36]

style: net:: namespace issues still

[thaidn_google, 2013/04/17 04:34:47]

Done.

+  mock_socket_factory.AddSocketDataProvider(&data);
+
+  session_params_.client_socket_factory = &mock_socket_factory;
+
+  scoped_refptr<HttpNetworkSession> session(
+      new HttpNetworkSession(session_params_));
+  scoped_ptr<HttpNetworkTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session));
+  TestCompletionCallback callback;

[Ryan Sleevi, 2013/04/17 01:16:36]

style nit: Move the TestCompletionCallback declaration closer to its first use
(eg: line 90/91)

Of course, if you move lines 87-89 below, as suggested, this may be unnecessary.

[thaidn_google, 2013/04/17 04:34:47]

Done.

+
+  SSLConfig& ssl_config = GetServerSSLConfig(trans.get());
+  // Explicitly configures the maximum supported SSL version to TLS 1.0.
+  ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1;

[Ryan Sleevi, 2013/04/17 01:16:36]

Seems like this could be done in the test harness SetUp() when setting up the
SSLConfigService to be a (class local) MockSSLConfigService?

It's such a small amount of code to mock (eg: see
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config...
) that you could get away with

class TLS1OnlySSLConfigService : public SSLConfigService {
 public:
  TLS1OnlySSLConfigService() {
    ssl_config_.version_min = SSL_PROTOCOL_VERSION_SSL3;
    ssl_config_.version_max = SSL_PROTOCOL_VERSION_TLS1;
  }

  virtual void GetSSLConfig(SSLConfig* config) OVERRIDE {
    *config = ssl_config_;
  }

 private:
  SSLConfig ssl_config_;
};


I suggest this approach because your tests are also (subtlely) dependent upon
version_min being SSL_PROTOCOL_VERSION_SSL3 (to enable fallback), which it may
not always be so.

[thaidn_google, 2013/04/17 04:34:47]

Done.

+
+  // This will consume |ssl_data|.
+  int rv = callback.GetResult(
+      trans->Start(GetRequestInfo("https://www.google.com/"),
+                   callback.callback(), BoundNetLog()));
+  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // |version_max| never fallbacks to SSLv3 for Google properties.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_TLS1,
+            ssl_config.version_max);
+  EXPECT_FALSE(ssl_config.version_fallback);
+}
+
+TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedDisabled) {
+  MockClientSocketFactory mock_socket_factory;
+
+  // |ssl_data1| contains the data for the first TLS 1.0 handshake that will
+  // return |ERR_SSL_PROTOCOL_ERROR| to trigger the SSL 3.0 fallback logic in
+  // |HttpNetworkTransaction|.
+  SSLSocketDataProvider ssl_data1(ASYNC, ERR_SSL_PROTOCOL_ERROR);
+  mock_socket_factory.AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  mock_socket_factory.AddSocketDataProvider(&data1);
+
+  // |ssl_data2| contains the data for the second SSL 3.0 handshake. When a
+  // connection to a server fails during a handshake,
+  // |HttpNetworkTransaction| will attempt to fallback to SSL 3.0 if the
+  // previous connection was attempted with TLS 1.0. This is transparent to the
+  // caller of the |HttpNetworkTransaction|.

[Ryan Sleevi, 2013/04/17 01:16:36]

With the comments from line 72 incorporated (eg: including a high level
description of this test on line 102), you could be brief and just say

// |ssl_data2| contains the handshake result for an SSL 3.0
// handshake which will be attempted after the TLS 1.0
// handshake fails.

[thaidn_google, 2013/04/17 04:34:47]

Done.

+
+  // Note: if |ssl_data2| is omitted (like in
+  // |Google_SSLVersionMinPreloadedEnabled|) then this test will crash, because
+  // |HttpNetworkTransaction| will attempt to handshake two times, but there is
+  // only one handshake data.

[Ryan Sleevi, 2013/04/17 01:16:36]

comment nit: I'm generally not fond of inter-test comments cross-referencing
eachother, because it means the developer has to skip around reading code.

Further, this comment makes me think that Google_SSLVersionMinPreloadedEnabled
perhaps would be served by having an extra SSL data (that should be unconsumed -
and this can be confirmed by asking the SSLSocketDataProvider/MockSocketFactory
how many sockets it vended)

This is because if the behaviour being tested in
Google_SSLVersionMinPreloadedEnabled ever breaks (and Google properties begin
SSL fallbacks, for example), then based on your comment here, that test will
start crashing (bringing down all of net_unittests), rather than failing
gracefully.

I'm not sure if I feel strongly enough on this fact to request it - because the
developer should be running things locally first - but it's enough to highlight
how significantly nasty the failure mode is.

[thaidn_google, 2013/04/17 04:34:47]

I agree.

On 2013/04/17 01:16:36, Ryan Sleevi wrote:

+  SSLSocketDataProvider ssl_data2(ASYNC, ERR_SSL_PROTOCOL_ERROR);
+  mock_socket_factory.AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  mock_socket_factory.AddSocketDataProvider(&data2);
+
+  session_params_.client_socket_factory = &mock_socket_factory;
+
+  scoped_refptr<HttpNetworkSession> session(
+      new HttpNetworkSession(session_params_));
+  scoped_ptr<HttpNetworkTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session));
+  TestCompletionCallback callback;
+
+  SSLConfig& ssl_config = GetServerSSLConfig(trans.get());
+  // Explicitly configures the maximum supported SSL version to TLS 1.0.
+  ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1;
+
+  ssl_config.ssl_version_min_preloaded_disabled = true;
+
+  // This will consume |ssl_data1| and |ssl_data2|.
+  int rv = callback.GetResult(
+      trans->Start(GetRequestInfo("https://www.google.com/"),
+                   callback.callback(), BoundNetLog()));
+  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // |version_max| fallbacks to SSL 3.0 for Google properties when
+  // |ssl_version_min_preloaded_disabled| is true.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_SSL3,
+            ssl_config.version_max);
+  EXPECT_TRUE(ssl_config.version_fallback);
+}
+
+TEST_F(HttpNetworkTransactionSSLTest, PayPal_SSLVersionMinPreloadedDisabled) {
+  MockClientSocketFactory mock_socket_factory;
+  // |ssl_data1| contains the data for the first TLS 1.0 handshake that will
+  // return |ERR_SSL_PROTOCOL_ERROR| to trigger the SSL 3.0 fallback logic in
+  // |HttpNetworkTransaction|.
+  SSLSocketDataProvider ssl_data1(ASYNC, ERR_SSL_PROTOCOL_ERROR);
+  mock_socket_factory.AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  mock_socket_factory.AddSocketDataProvider(&data1);
+
+  // |ssl_data2| contains the data for the second SSL 3.0 handshake. When a
+  // connection to a server fails during a handshake,
+  // |HttpNetworkTransaction| will attempt to fallback to SSL 3.0 if the
+  // previous connection was attempted with TLS 1.0. This is transparent to the
+  // caller of the |HttpNetworkTransaction|.
+
+  // Note: if |ssl_data2| is omitted (like in
+  // |Google_SSLVersionMinPreloadedEnabled|) then this test will crash, because
+  // |HttpNetworkTransaction| will attempt to handshake two times, but there is
+  // only one handshake data.
+  SSLSocketDataProvider ssl_data2(ASYNC, ERR_SSL_PROTOCOL_ERROR);
+  mock_socket_factory.AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  mock_socket_factory.AddSocketDataProvider(&data2);
+
+  session_params_.client_socket_factory = &mock_socket_factory;
+
+  scoped_refptr<HttpNetworkSession> session(
+      new HttpNetworkSession(session_params_));
+  scoped_ptr<HttpNetworkTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session));
+  TestCompletionCallback callback;
+
+  SSLConfig& ssl_config = GetServerSSLConfig(trans.get());
+  // Explicitly configures the maximum supported SSL version to TLS 1.0.
+  ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1;
+
+  // This will consume |ssl_data1| and |ssl_data2|.
+  int rv = callback.GetResult(
+      trans->Start(GetRequestInfo("https://www.paypal.com/"),
+                   callback.callback(), BoundNetLog()));
+  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // |version_max| fallbacks to SSL 3.0.
+  EXPECT_EQ(SSL_PROTOCOL_VERSION_SSL3,
+            ssl_config.version_max);

[Ryan Sleevi, 2013/04/17 01:16:36]

Is wrapping really necessary here?

[thaidn_google, 2013/04/17 04:34:47]

Done.

+  EXPECT_TRUE(ssl_config.version_fallback);
+}
+
+}  // namespace net
+