net: False Start only for NPN capable servers.

net/socket/ssl_client_socket_nss.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.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 98 matching lines @@
 #include <Security/SecBase.h>
 #include <Security/SecCertificate.h>
 #include <Security/SecIdentity.h>
 #include "base/mac/mac_logging.h"
 #elif defined(USE_NSS)
 #include <dlfcn.h>
 #endif
 
 static const int kRecvBufferSize = 4096;
 
-// kCorkTimeoutMs is the number of milliseconds for which we'll wait for a
-// Write to an SSL socket which we're False Starting. Since corking stops the
-// Finished message from being sent, the server sees an incomplete handshake
-// and some will time out such sockets quite aggressively.
-static const int kCorkTimeoutMs = 200;

[Ryan Sleevi, 2012/04/09 22:42:56]

This seems unrelated to the description. That is, I can see the cork being
useful for false-start servers, based on how this is implemented within NSS and
perceived by the socket / HttpStream. Intentional?

[agl, 2012/04/09 23:05:31]

The cork aims to put the CKX, CCS, Finished and Application Data in a single TCP
packet. This, it was hoped, would make False Start intolerance more
deterministic. It doesn't do anything for False Start capable servers except,
possibly save a packet. I don't think it's worth the code complexity for that.

-
 #if defined(OS_WIN)
 // CERT_OCSP_RESPONSE_PROP_ID is only implemented on Vista+, but it can be
 // set on Windows XP without error. There is some overhead from the server
 // sending the OCSP response if it supports the extension, for the subset of
 // XP clients who will request it but be unable to use it, but this is an
 // acceptable trade-off for simplicity of implementation.
 static bool IsOCSPStaplingSupported() {
   return true;
 }
 #elif defined(USE_NSS)
@@ skipping 295 matching lines @@
 
 }  // namespace
 
 SSLClientSocketNSS::SSLClientSocketNSS(ClientSocketHandle* transport_socket,
                                        const HostPortPair& host_and_port,
                                        const SSLConfig& ssl_config,
                                        SSLHostInfo* ssl_host_info,
                                        const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
-      corked_(false),
       transport_(transport_socket),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       user_read_buf_len_(0),
       user_write_buf_len_(0),
       server_cert_nss_(NULL),
       server_cert_verify_result_(NULL),
       ssl_connection_status_(0),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
@@ skipping 333 matching lines @@
   DCHECK(completed_handshake_);
   DCHECK(next_handshake_state_ == STATE_NONE);
   DCHECK(user_write_callback_.is_null());
   DCHECK(user_connect_callback_.is_null());
   DCHECK(!user_write_buf_);
   DCHECK(nss_bufs_);
 
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
 
-  if (corked_) {
-    corked_ = false;
-    uncork_timer_.Reset();
-  }
   int rv = DoWriteLoop(OK);
 
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
   } else {
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
   }
   LeaveFunction(rv);
   return rv;
@@ skipping 102 matching lines @@
 #ifdef SSL_ENABLE_DEFLATE
   // Some web servers have been found to break if TLS is used *or* if DEFLATE
   // is advertised. Thus, if TLS is disabled (probably because we are doing
   // SSLv3 fallback), we disable DEFLATE also.
   // See http://crbug.com/31628
   rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_DEFLATE, ssl_config_.tls1_enabled);
   if (rv != SECSuccess)
     LogFailedNSSFunction(net_log_, "SSL_OptionSet", "SSL_ENABLE_DEFLATE");
 #endif
 
-  PRBool false_start_enabled =
-      ssl_config_.false_start_enabled &&
-      !SSLConfigService::IsKnownFalseStartIncompatibleServer(
-          host_and_port_.host());
 #ifdef SSL_ENABLE_FALSE_START
-  rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_FALSE_START, false_start_enabled);
+  rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_FALSE_START,
+                     ssl_config_.false_start_enabled);
   if (rv != SECSuccess)
     LogFailedNSSFunction(net_log_, "SSL_OptionSet", "SSL_ENABLE_FALSE_START");
 #endif
 
 #ifdef SSL_ENABLE_RENEGOTIATION
   // We allow servers to request renegotiation. Since we're a client,
   // prohibiting this is rather a waste of time. Only servers are in a
   // position to prevent renegotiation attacks.
   // http://extendedsubset.com/?p=8
 
   rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_RENEGOTIATION,
                      SSL_RENEGOTIATE_TRANSITIONAL);
   if (rv != SECSuccess) {
     LogFailedNSSFunction(
         net_log_, "SSL_OptionSet", "SSL_ENABLE_RENEGOTIATION");
   }
 #endif  // SSL_ENABLE_RENEGOTIATION
 
   if (!ssl_config_.next_protos.empty()) {
     rv = SSL_SetNextProtoCallback(
         nss_fd_, SSLClientSocketNSS::NextProtoCallback, this);
     if (rv != SECSuccess)
       LogFailedNSSFunction(net_log_, "SSL_SetNextProtoCallback", "");
   }
 
 #ifdef SSL_CBC_RANDOM_IV
-  rv = SSL_OptionSet(nss_fd_, SSL_CBC_RANDOM_IV, false_start_enabled);
+  rv = SSL_OptionSet(nss_fd_, SSL_CBC_RANDOM_IV,
+                     ssl_config_.false_start_enabled);

[Ryan Sleevi, 2012/04/09 22:42:56]

Did you mean to make this unconditionally true? Or perhaps contingent on the
policy flag?

Otherwise, it would seem like you're still equating the 1/n-1 fix with false
start enable/disable (even if now it's most-always-on)

[agl, 2012/04/09 23:05:31]

|false_start_enabled| now means that we *can* do False Start if the server
supports NPN. It's also the switch that the policy for
kDisableSSLRecordSplitting toggles, so we do still want to tie them together.
Untangling them would be a good thing, but something for a different CL I feel.

   if (rv != SECSuccess)
     LogFailedNSSFunction(net_log_, "SSL_OptionSet", "SSL_CBC_RANDOM_IV");
 #endif
 
 #ifdef SSL_ENABLE_OCSP_STAPLING
   if (IsOCSPStaplingSupported()) {
     rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_OCSP_STAPLING, PR_TRUE);
     if (rv != SECSuccess) {
       LogFailedNSSFunction(net_log_, "SSL_OptionSet",
                            "SSL_ENABLE_OCSP_STAPLING");
@@ skipping 995 matching lines @@
       return;
 
     state->certs.push_back(std::string(
           reinterpret_cast<char*>(certs[i]->derCert.data),
           certs[i]->derCert.len));
   }
 
   ssl_host_info_->Persist();
 }
 
-void SSLClientSocketNSS::UncorkAfterTimeout() {
-  corked_ = false;
-  int nsent;
-  do {
-    nsent = BufferSend();
-  } while (nsent > 0);
-}
-
 // Do as much network I/O as possible between the buffer and the
 // transport socket. Return true if some I/O performed, false
 // otherwise (error or ERR_IO_PENDING).
 bool SSLClientSocketNSS::DoTransportIO() {
   EnterFunction("");
   bool network_moved = false;
   if (nss_bufs_ != NULL) {
     int rv;
     // Read and write as much data as we can. The loop is neccessary
     // because Write() may return synchronously.
@@ skipping 16 matching lines @@
   if (transport_send_busy_)
     return ERR_IO_PENDING;
 
   EnterFunction("");
   const char* buf1;
   const char* buf2;
   unsigned int len1, len2;
   memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2);
   const unsigned int len = len1 + len2;
 
-  if (corked_ && len < kRecvBufferSize / 2)
-    return 0;
-
   int rv = 0;
   if (len) {
     scoped_refptr<IOBuffer> send_buffer(new IOBuffer(len));
     memcpy(send_buffer->data(), buf1, len1);
     memcpy(send_buffer->data() + len1, buf2, len2);
     rv = transport_->socket()->Write(
         send_buffer, len,
         base::Bind(&SSLClientSocketNSS::BufferSendComplete,
                    base::Unretained(this)));
     if (rv == ERR_IO_PENDING) {
@@ skipping 93 matching lines @@
 // static
 // NSS calls this if an incoming certificate needs to be verified.
 // Do nothing but return SECSuccess.
 // This is called only in full handshake mode.
 // Peer certificate is retrieved in HandshakeCallback() later, which is called
 // in full handshake mode or in resumption handshake mode.
 SECStatus SSLClientSocketNSS::OwnAuthCertHandler(void* arg,
                                                  PRFileDesc* socket,
                                                  PRBool checksig,
                                                  PRBool is_server) {
-#ifdef SSL_ENABLE_FALSE_START
-  // In the event that we are False Starting this connection, we wish to send
-  // out the Finished message and first application data record in the same
-  // packet. This prevents non-determinism when talking to False Start
-  // intolerant servers which, otherwise, might see the two messages in
-  // different reads or not, depending on network conditions.
-  PRBool false_start = 0;
-  SECStatus rv = SSL_OptionGet(socket, SSL_ENABLE_FALSE_START, &false_start);
-  DCHECK_EQ(SECSuccess, rv);
-
-  SSLClientSocketNSS* that = reinterpret_cast<SSLClientSocketNSS*>(arg);
-  CERTCertificate* cert = SSL_PeerCertificate(that->nss_fd_);
-  if (cert) {
-    char* common_name = CERT_GetCommonName(&cert->issuer);
-    if (common_name) {
-      if (false_start && strcmp(common_name, "ESET_RootSslCert") == 0) {
-        // ESET anti-virus is capable of intercepting HTTPS connections on
-        // Windows.  However, it is False Start intolerant and causes the
-        // connections to hang forever. We detect ESET by the issuer of the
-        // leaf certificate and set a flag to return a specific error, giving
-        // the user instructions for reconfiguring ESET.
-        that->eset_mitm_detected_ = true;
-      }
-      if (false_start &&
-          strcmp(common_name, "ContentWatch Root Certificate Authority") == 0) {
-        // This is NetNanny. NetNanny are updating their product so we
-        // silently disable False Start for now.
-        rv = SSL_OptionSet(socket, SSL_ENABLE_FALSE_START, PR_FALSE);
-        DCHECK_EQ(SECSuccess, rv);
-        false_start = 0;
-      }
-      PORT_Free(common_name);
-    }
-    CERT_DestroyCertificate(cert);
-  }
-
-  if (false_start && !that->handshake_callback_called_) {
-    that->corked_ = true;
-    that->uncork_timer_.Start(FROM_HERE,
-        base::TimeDelta::FromMilliseconds(kCorkTimeoutMs),
-        that, &SSLClientSocketNSS::UncorkAfterTimeout);
-  }
-#endif
-
   // Tell NSS to not verify the certificate.
   return SECSuccess;
 }
 
 // static
 bool SSLClientSocketNSS::DomainBoundCertNegotiated(PRFileDesc* socket) {
   PRBool xtn_negotiated = PR_FALSE;
   SECStatus rv = SSL_HandshakeNegotiatedExtension(
       socket, ssl_ob_cert_xtn, &xtn_negotiated);
   DCHECK_EQ(SECSuccess, rv);
@@ skipping 528 matching lines @@
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 ServerBoundCertService* SSLClientSocketNSS::GetServerBoundCertService() const {
   return server_bound_cert_service_;
 }
 
 }  // namespace net