net: Ensure that when using False Start + client auth, bad client certificates are not cached.

If an SSL handshake fails when client certificates are used, ensure that
the client certificate selected is removed from the SSL client auth
cache. This ensures that the user is prompted to select a certificate
again, as the cause of the failure may have been due to selecting the
wrong certificate or selecting no certificate when one is required.

The existing logic worked when TLS False Start was disabled, but could
fail when False Start was used or when the peer requests renegotiation.
This changes ensures the client certificate is removed from the cache by
moving the cache removal layer from the HttpStreamRequest to the
HttpNetworkTransaction.

Patch by: Ryan Sleevi

BUG=66424
TEST=HttpNetworkTransactionTest.ClientAuthCertCache*

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71071 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 260 insertions, 62 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 64a4fa7..51d962f 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -554,6 +554,10 @@ int HttpNetworkTransaction::DoCreateStreamComplete(int result) {
     return OK;
   }
 
+  // Handle possible handshake errors that may have occurred if the stream
+  // used SSL for one or more of the layers.
+  result = HandleSSLHandshakeError(result);
+
   // At this point we are done with the stream_request_.
   stream_request_.reset();
   return result;
@@ -696,23 +700,6 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
     result = HandleCertificateRequest(result);
     if (result == OK)
       return result;
-  } else if ((result == ERR_SSL_DECOMPRESSION_FAILURE_ALERT ||
-              result == ERR_SSL_BAD_RECORD_MAC_ALERT) &&
-             ssl_config_.tls1_enabled &&
-             !SSLConfigService::IsKnownStrictTLSServer(request_->url.host())) {
-    // Some buggy servers select DEFLATE compression when offered and then
-    // fail to ever decompress anything. They will send a fatal alert telling
-    // us this. Normally we would pick this up during the handshake because
-    // our Finished message is compressed and we'll never get the server's
-    // Finished if it fails to process ours.
-    //
-    // However, with False Start, we'll believe that the handshake is
-    // complete as soon as we've /sent/ our Finished message. In this case,
-    // we only find out that the server is buggy here, when we try to read
-    // the initial reply.
-    session_->http_stream_factory()->AddTLSIntolerantServer(request_->url);
-    ResetConnectionAndRequestForResend();
-    return OK;
   }
 
   if (result < 0 && result != ERR_CONNECTION_CLOSED)
@@ -1024,11 +1011,61 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   return OK;
 }
 
+// TODO(rch): This does not correctly handle errors when an SSL proxy is
+// being used, as all of the errors are handled as if they were generated
+// by the endpoint host, request_->url, rather than considering if they were
+// generated by the SSL proxy. http://crbug.com/66424
+int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
+  DCHECK(request_);
+  if (ssl_config_.send_client_cert &&
+      (error == ERR_SSL_PROTOCOL_ERROR ||
+       error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
+    session_->ssl_client_auth_cache()->Remove(
+        GetHostAndPort(request_->url));
+  }
+
+  switch (error) {
+    case ERR_SSL_PROTOCOL_ERROR:
+    case ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
+    case ERR_SSL_DECOMPRESSION_FAILURE_ALERT:
+    case ERR_SSL_BAD_RECORD_MAC_ALERT:
+      if (ssl_config_.tls1_enabled &&
+          !SSLConfigService::IsKnownStrictTLSServer(request_->url.host())) {
+        // This could be a TLS-intolerant server, an SSL 3.0 server that
+        // chose a TLS-only cipher suite or a server with buggy DEFLATE
+        // support. Turn off TLS 1.0, DEFLATE support and retry.
+        session_->http_stream_factory()->AddTLSIntolerantServer(request_->url);
+        ResetConnectionAndRequestForResend();
+        error = OK;
+      }
+      break;
+    case ERR_SSL_SNAP_START_NPN_MISPREDICTION:
+      // This means that we tried to Snap Start a connection, but we
+      // mispredicted the NPN result. This isn't a problem from the point of
+      // view of the SSL layer because the server will ignore the application
+      // data in the Snap Start extension. However, at the HTTP layer, we have
+      // already decided that it's a HTTP or SPDY connection and it's easier to
+      // abort and start again.
+      ResetConnectionAndRequestForResend();
+      error = OK;
+      break;
+  }
+  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
+  // renegotiation at any time, check and handle any possible SSL handshake
+  // related errors. In addition to renegotiation, TLS False/Snap Start may
+  // cause SSL handshake errors to be delayed until the first Read on the
+  // underlying connection.
+  error = HandleSSLHandshakeError(error);
+
   switch (error) {
     // If we try to reuse a connection that the server is in the process of
     // closing, we may end up successfully writing out our request (or a
@@ -1042,16 +1079,6 @@ int HttpNetworkTransaction::HandleIOError(int error) {
         error = OK;
       }
       break;
-    case ERR_SSL_SNAP_START_NPN_MISPREDICTION:
-      // This means that we tried to Snap Start a connection, but we
-      // mispredicted the NPN result. This isn't a problem from the point of
-      // view of the SSL layer because the server will ignore the application
-      // data in the Snap Start extension. However, at the HTTP layer, we have
-      // already decided that it's a HTTP or SPDY connection and it's easier to
-      // abort and start again.
-      ResetConnectionAndRequestForResend();
-      error = OK;
-      break;
   }
   return error;
 }
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index dfcee41f..bac1451 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -137,6 +137,11 @@ class HttpNetworkTransaction : public HttpTransaction,
   // Called to handle a client certificate request.
   int HandleCertificateRequest(int error);
 
+  // Called to possibly recover from an SSL handshake error.  Sets next_state_
+  // and returns OK if recovering from the error.  Otherwise, the same error
+  // code is returned.
+  int HandleSSLHandshakeError(int error);
+
   // Called to possibly recover from the given error.  Sets next_state_ and
   // returns OK if recovering from the error.  Otherwise, the same error code
   // is returned.
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 260de13..5c9f0a2 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -20,6 +20,7 @@
 #include "net/base/net_log.h"
 #include "net/base/net_log_unittest.h"
 #include "net/base/request_priority.h"
+#include "net/base/ssl_cert_request_info.h"
 #include "net/base/ssl_config_service_defaults.h"
 #include "net/base/ssl_info.h"
 #include "net/base/test_completion_callback.h"
@@ -8207,4 +8208,203 @@ TEST_F(HttpNetworkTransactionTest, NPNMispredict) {
   EXPECT_EQ("hello world", contents);
 }
 
+// Ensure that a client certificate is removed from the SSL client auth
+// cache when:
+//  1) No proxy is involved.
+//  2) TLS False Start is disabled.
+//  3) The initial TLS handshake requests a client certificate.
+//  4) The client supplies an invalid/unacceptable certificate.
+TEST_F(HttpNetworkTransactionTest, ClientAuthCertCache_Direct_NoFalseStart) {
+  SessionDependencies session_deps;
+
+  scoped_refptr<SSLCertRequestInfo> cert_request(new SSLCertRequestInfo());
+  cert_request->host_and_port = "www.example.com:443";
+
+  // [ssl_]data1 contains the data for the first SSL handshake. When a
+  // CertificateRequest is received for the first time, the handshake will
+  // be aborted to allow the caller to provide a certificate.
+  SSLSocketDataProvider ssl_data1(true /* async */,
+                                  net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
+  ssl_data1.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data1);
+
+  // [ssl_]data2 contains the data for the second SSL handshake. When TLS
+  // False Start is not being used, the result of the SSL handshake will be
+  // returned as part of the SSLClientSocket::Connect() call. This test
+  // matches the result of a server sending a handshake_failure alert,
+  // rather than a Finished message, because it requires a client
+  // certificate and none was supplied.
+  SSLSocketDataProvider ssl_data2(true /* async */,
+                                  net::ERR_SSL_PROTOCOL_ERROR);
+  ssl_data2.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data2);
+  net::StaticSocketDataProvider data2(NULL, 0, NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data2);
+
+  // [ssl_]data3 contains the data for the third SSL handshake. When a
+  // connection to a server fails during an SSL handshake,
+  // HttpNetworkTransaction will attempt to fallback to SSLv3 if the initial
+  // connection was attempted with TLSv1. This is transparent to the caller
+  // of the HttpNetworkTransaction. Because this test failure is due to
+  // requiring a client certificate, this fallback handshake should also
+  // fail.
+  SSLSocketDataProvider ssl_data3(true /* async */,
+                                  net::ERR_SSL_PROTOCOL_ERROR);
+  ssl_data3.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data3);
+  net::StaticSocketDataProvider data3(NULL, 0, NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data3);
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(new HttpNetworkTransaction(session));
+
+  net::HttpRequestInfo request_info;
+  request_info.url = GURL("https://www.example.com/");
+  request_info.method = "GET";
+  request_info.load_flags = net::LOAD_NORMAL;
+
+  // Begin the SSL handshake with the peer. This consumes ssl_data1.
+  TestCompletionCallback callback;
+  int rv = trans->Start(&request_info, &callback, net::BoundNetLog());
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+  // Complete the SSL handshake, which should abort due to requiring a
+  // client certificate.
+  rv = callback.WaitForResult();
+  ASSERT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, rv);
+
+  // Indicate that no certificate should be supplied. From the perspective
+  // of SSLClientCertCache, NULL is just as meaningful as a real
+  // certificate, so this is the same as supply a
+  // legitimate-but-unacceptable certificate.
+  rv = trans->RestartWithCertificate(NULL, &callback);
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+  // Ensure the certificate was added to the client auth cache before
+  // allowing the connection to continue restarting.
+  scoped_refptr<X509Certificate> client_cert;
+  ASSERT_TRUE(session->ssl_client_auth_cache()->Lookup("www.example.com:443",
+                                                       &client_cert));
+  ASSERT_EQ(NULL, client_cert.get());
+
+  // Restart the handshake. This will consume ssl_data2, which fails, and
+  // then consume ssl_data3, which should also fail. The result code is
+  // checked against what ssl_data3 should return.
+  rv = callback.WaitForResult();
+  ASSERT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // Ensure that the client certificate is removed from the cache on a
+  // handshake failure.
+  ASSERT_FALSE(session->ssl_client_auth_cache()->Lookup("www.example.com:443",
+                                                        &client_cert));
+}
+
+// Ensure that a client certificate is removed from the SSL client auth
+// cache when:
+//  1) No proxy is involved.
+//  2) TLS False Start is enabled.
+//  3) The initial TLS handshake requests a client certificate.
+//  4) The client supplies an invalid/unacceptable certificate.
+TEST_F(HttpNetworkTransactionTest, ClientAuthCertCache_Direct_FalseStart) {
+  SessionDependencies session_deps;
+
+  scoped_refptr<SSLCertRequestInfo> cert_request(new SSLCertRequestInfo());
+  cert_request->host_and_port = "www.example.com:443";
+
+  // When TLS False Start is used, SSLClientSocket::Connect() calls will
+  // return successfully after reading up to the peer's Certificate message.
+  // This is to allow the caller to call SSLClientSocket::Write(), which can
+  // enqueue application data to be sent in the same packet as the
+  // ChangeCipherSpec and Finished messages.
+  // The actual handshake will be finished when SSLClientSocket::Read() is
+  // called, which expects to process the peer's ChangeCipherSpec and
+  // Finished messages. If there was an error negotiating with the peer,
+  // such as due to the peer requiring a client certificate when none was
+  // supplied, the alert sent by the peer won't be processed until Read() is
+  // called.
+
+  // Like the non-False Start case, when a client certificate is requested by
+  // the peer, the handshake is aborted during the Connect() call.
+  // [ssl_]data1 represents the initial SSL handshake with the peer.
+  SSLSocketDataProvider ssl_data1(true /* async */,
+                                  net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
+  ssl_data1.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data1);
+  net::StaticSocketDataProvider data1(NULL, 0, NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data1);
+
+  // When a client certificate is supplied, Connect() will not be aborted
+  // when the peer requests the certificate. Instead, the handshake will
+  // artificially succeed, allowing the caller to write the HTTP request to
+  // the socket. The handshake messages are not processed until Read() is
+  // called, which then detects that the handshake was aborted, due to the
+  // peer sending a handshake_failure because it requires a client
+  // certificate.
+  SSLSocketDataProvider ssl_data2(true /* async */, net::OK);
+  ssl_data2.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data2);
+  net::MockRead data2_reads[] = {
+    net::MockRead(true /* async */, net::ERR_SSL_PROTOCOL_ERROR),
+  };
+  net::StaticSocketDataProvider data2(
+      data2_reads, arraysize(data2_reads), NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data2);
+
+  // As described in ClientAuthCertCache_Direct_NoFalseStart, [ssl_]data3 is
+  // the data for the SSL handshake once the TLSv1 connection falls back to
+  // SSLv3. It has the same behaviour as [ssl_]data2.
+  SSLSocketDataProvider ssl_data3(true /* async */, net::OK);
+  ssl_data3.cert_request_info = cert_request.get();
+  session_deps.socket_factory.AddSSLSocketDataProvider(&ssl_data3);
+  net::StaticSocketDataProvider data3(
+      data2_reads, arraysize(data2_reads), NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data3);
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+  scoped_ptr<HttpNetworkTransaction> trans(new HttpNetworkTransaction(session));
+
+  net::HttpRequestInfo request_info;
+  request_info.url = GURL("https://www.example.com/");
+  request_info.method = "GET";
+  request_info.load_flags = net::LOAD_NORMAL;
+
+  // Begin the initial SSL handshake.
+  TestCompletionCallback callback;
+  int rv = trans->Start(&request_info, &callback, net::BoundNetLog());
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+  // Complete the SSL handshake, which should abort due to requiring a
+  // client certificate.
+  rv = callback.WaitForResult();
+  ASSERT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED, rv);
+
+  // Indicate that no certificate should be supplied. From the perspective
+  // of SSLClientCertCache, NULL is just as meaningful as a real
+  // certificate, so this is the same as supply a
+  // legitimate-but-unacceptable certificate.
+  rv = trans->RestartWithCertificate(NULL, &callback);
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+  // Ensure the certificate was added to the client auth cache before
+  // allowing the connection to continue restarting.
+  scoped_refptr<X509Certificate> client_cert;
+  ASSERT_TRUE(session->ssl_client_auth_cache()->Lookup("www.example.com:443",
+                                                       &client_cert));
+  ASSERT_EQ(NULL, client_cert.get());
+
+
+  // Restart the handshake. This will consume ssl_data2, which fails, and
+  // then consume ssl_data3, which should also fail. The result code is
+  // checked against what ssl_data3 should return.
+  rv = callback.WaitForResult();
+  ASSERT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv);
+
+  // Ensure that the client certificate is removed from the cache on a
+  // handshake failure.
+  ASSERT_FALSE(session->ssl_client_auth_cache()->Lookup("www.example.com:443",
+                                                        &client_cert));
+}
+
 }  // namespace net
diff --git a/net/http/http_stream_request.cc b/net/http/http_stream_request.cc
index c031145..ea0b588 100644
--- a/net/http/http_stream_request.cc
+++ b/net/http/http_stream_request.cc
@@ -761,7 +761,7 @@ int HttpStreamRequest::DoInitConnectionComplete(int result) {
       }
     }
     if (result < 0)
-      return HandleSSLHandshakeError(result);
+      return result;
   }
 
   next_state_ = STATE_CREATE_STREAM;
@@ -1043,35 +1043,6 @@ int HttpStreamRequest::HandleCertificateError(int error) {
   return error;
 }
 
-int HttpStreamRequest::HandleSSLHandshakeError(int error) {
-  if (ssl_config()->send_client_cert &&
-      (error == ERR_SSL_PROTOCOL_ERROR ||
-       error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
-    session_->ssl_client_auth_cache()->Remove(
-        GetHostAndPort(request_info().url));
-  }
-
-  switch (error) {
-    case ERR_SSL_PROTOCOL_ERROR:
-    case ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
-    case ERR_SSL_DECOMPRESSION_FAILURE_ALERT:
-    case ERR_SSL_BAD_RECORD_MAC_ALERT:
-      if (ssl_config()->tls1_enabled &&
-          !SSLConfigService::IsKnownStrictTLSServer(
-          request_info().url.host())) {
-        // This could be a TLS-intolerant server, an SSL 3.0 server that
-        // chose a TLS-only cipher suite or a server with buggy DEFLATE
-        // support. Turn off TLS 1.0, DEFLATE support and retry.
-        factory_->AddTLSIntolerantServer(request_info().url);
-        next_state_ = STATE_INIT_CONNECTION;
-        DCHECK(!connection_.get() || !connection_->socket());
-        error = OK;
-      }
-      break;
-  }
-  return error;
-}
-
 void HttpStreamRequest::SwitchToSpdyMode() {
   if (HttpStreamFactory::spdy_enabled())
     using_spdy_ = true;
diff --git a/net/http/http_stream_request.h b/net/http/http_stream_request.h
index 62ba5d8..fca2332 100644
--- a/net/http/http_stream_request.h
+++ b/net/http/http_stream_request.h
@@ -172,11 +172,6 @@ class HttpStreamRequest : public StreamRequest {
   // Called to handle a client certificate request.
   int HandleCertificateRequest(int error);
 
-  // Called to possibly recover from an SSL handshake error.  Sets next_state_
-  // and returns OK if recovering from the error.  Otherwise, the same error
-  // code is returned.
-  int HandleSSLHandshakeError(int error);
-
   // Moves this stream request into SPDY mode.
   void SwitchToSpdyMode();