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

net/http/http_network_transaction.cc

 // Copyright (c) 2010 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/compiler_specific.h"
@@ skipping 536 matching lines @@
     next_state_ = STATE_INIT_STREAM;
     DCHECK(stream_.get());
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     result = HandleCertificateRequest(result);
   } else if (result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
     // Return OK and let the caller read the proxy's error page
     next_state_ = STATE_NONE;
     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;
 }
 
 int HttpNetworkTransaction::DoInitStream() {
   DCHECK(stream_.get());
   next_state_ = STATE_INIT_STREAM_COMPLETE;
   return stream_->InitializeStream(request_, net_log_, &io_callback_);
 }
@@ skipping 122 matching lines @@
     result = ERR_CERT_ERROR_IN_SSL_RENEGOTIATION;
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     // TODO(wtc): Need a test case for this code path!
     DCHECK(stream_.get());
     DCHECK(is_https_request());
     response_.cert_request_info = new SSLCertRequestInfo;
     stream_->GetSSLCertRequestInfo(response_.cert_request_info);
     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)
     return HandleIOError(result);
 
   if (result == ERR_CONNECTION_CLOSED && ShouldResendRequest(result)) {
     ResetConnectionAndRequestForResend();
     return OK;
   }
 
@@ skipping 291 matching lines @@
   // SSL session cache.
   ssl_config_.client_cert = client_cert;
   ssl_config_.send_client_cert = true;
   next_state_ = STATE_CREATE_STREAM;
   // Reset the other member variables.
   // Note: this is necessary only with SSL renegotiation.
   ResetStateForRestart();
   return OK;
 }
 
+// TODO(rch): This does not currently handle errors when an SSL proxy is
+// being used (http://crbug.com/66424)

[wtc, 2011/01/10 23:40:58]

You should point out what the issue is.  I believe the issue
is that GetHostAndPort(request_->url) is the wrong auth target
for an SSL proxy.

+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;
+  }
+  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) {

[wtc, 2011/01/10 23:40:58]

1. Having HandleIOError call HandleSSLHandshakeError means
we may be calling HandleSSLHandshakeError where it's not
necessary, for example, in DoInitStreamComplete.  This is
probably OK.  I just wanted to make sure you considered this
point.

Or do we actually need to call HandleSSLHandshakeError in
DoInitStreamComplete because we removed the
HandleSSLHandshakeError call from
HttpStreamRequest::DoInitConnectionComplete?

2. If HandleIOError calls HandleSSLHandshakeError, then the
function name "HandleIOError" is no longer accurate.  The
function should be renamed HandleSSLHandshakeAndIOError
(or a better name).

[Ryan Sleevi, 2011/01/11 01:37:47]

If IO can happen, then a check for SSL errors needs to happen, since an SSL
renegotiation may be initiated at any time by the peer. In practice, AFAICT, no
IO happens for the InitStream scenario, but given that HandleIOError is checking
for IO-layer errors, then it's appropriate to also be checking for issues at the
SSL transport layer.
I would have thought that, from the perspective of HttpNetworkTransaction, SSL
errors are no different than IO errors. That is, all SSL handshake errors (or
SSL errors, for that matter), are just a different type of IO error. This is
because, at any time in the Read stream, an SSL handshake error may occur. 

The main reason that I split it into two methods, rather than sticking all of
the logic in HandleIOError(), is that the comment of HandleIOError was
unambiguously clear that it should not be called in the Connect() case. Since a
non-false-start connection may complete as part of DoCreateStreamComplete() -
that is, an underlying call to Connect() - I split it into the two methods so
that the former could be called for both Connect() [which can be hit when false
start is disabled] and Read/Write [which can be hit either when using False
Start or the peer requests renegotiation]

+  // Handle possible SSL handshake errors that may occur during send/receive
+  // due to either False Start or Snap Start.
+  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
     // portion of our request) only to find a connection error when we try to
     // read from (or finish writing to) the socket.
     case ERR_CONNECTION_RESET:
     case ERR_CONNECTION_CLOSED:
     case ERR_CONNECTION_ABORTED:
       if (ShouldResendRequest(error)) {
         ResetConnectionAndRequestForResend();
         error = OK;
       }
       break;
     case ERR_SSL_SNAP_START_NPN_MISPREDICTION:

[wtc, 2011/01/10 23:40:58]

Please move this case into HandleSSLHandshakeError.

       // 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;
   }
@@ skipping 147 matching lines @@
     default:
       return priority;
   }
 }
 
 
 
 #undef STATE_CASE
 
 }  // namespace net