Run the read pipe in SSLClientSocket immediately after the handshake.

See https://boringssl-review.googlesource.com/c/boringssl/+/37944 for
the rationale.

Bug: 950706, 958638
Change-Id: I221b0e7f6f626968fc38e0bce84260e4141c5e44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838456Reviewed-by: Steven Valdez <svaldez@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702944}

net/socket/ssl_client_socket_impl.cc

@@ -16,6 +16,7 @@
 #include "base/containers/span.h"
 #include "base/feature_list.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/field_trial.h"
@@ -26,6 +27,7 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ -401,6 +403,7 @@ SSLClientSocketImpl::SSLClientSocketImpl(
       ssl_config_(ssl_config),
       next_handshake_state_(STATE_NONE),
       in_confirm_handshake_(false),
+      peek_complete_(false),
       disconnected_(false),
       negotiated_protocol_(kProtoUnknown),
       certificate_requested_(false),
@@ -871,8 +874,10 @@ int SSLClientSocketImpl::Init() {
   // Configure BoringSSL to allow renegotiations. Once the initial handshake
   // completes, if renegotiations are not allowed, the default reject value will
   // be restored. This is done in this order to permit a BoringSSL
-  // optimization. See https://crbug.com/boringssl/123.
-  SSL_set_renegotiate_mode(ssl_.get(), ssl_renegotiate_freely);
+  // optimization. See https://crbug.com/boringssl/123. Use
+  // ssl_renegotiate_explicit rather than ssl_renegotiate_freely so DoPeek()
+  // does not trigger renegotiations.
+  SSL_set_renegotiate_mode(ssl_.get(), ssl_renegotiate_explicit);
 
   SSL_set_shed_handshake_config(ssl_.get(), 1);
 
@@ -1085,6 +1090,25 @@ int SSLClientSocketImpl::DoHandshakeComplete(int result) {
 
   completed_connect_ = true;
   next_handshake_state_ = STATE_NONE;
+
+  // Read from the transport immediately after the handshake, whether Read() is
+  // called immediately or not. This serves several purposes:
+  //
+  // First, if this socket is preconnected and negotiates 0-RTT, the ServerHello
+  // will not be processed. See https://crbug.com/950706
+  //
+  // Second, in False Start and TLS 1.3, the tickets arrive after immediately
+  // after the handshake. This allows preconnected sockets to process the
+  // tickets sooner. This also avoids a theoretical deadlock if the tickets are
+  // too large. See
+  // https://boringssl-review.googlesource.com/c/boringssl/+/34948.
+  //
+  // TODO(https://crbug.com/958638): It is also a step in making TLS 1.3 client
+  // certificate alerts less unreliable.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&SSLClientSocketImpl::DoPeek, weak_factory_.GetWeakPtr()));
+
   return OK;
 }
 
@@ -1320,22 +1344,29 @@ int SSLClientSocketImpl::DoPayloadRead(IOBuffer* buf, int buf_len) {
   }
 
   int total_bytes_read = 0;
-  int ssl_ret;
+  int ssl_ret, ssl_err;
   do {
     ssl_ret = SSL_read(ssl_.get(), buf->data() + total_bytes_read,
                        buf_len - total_bytes_read);
-    if (ssl_ret > 0)
+    ssl_err = SSL_get_error(ssl_.get(), ssl_ret);
+    if (ssl_ret > 0) {
       total_bytes_read += ssl_ret;
+    } else if (ssl_err == SSL_ERROR_WANT_RENEGOTIATE) {
+      if (!SSL_renegotiate(ssl_.get())) {
+        ssl_err = SSL_ERROR_SSL;
+      }
+    }
     // Continue processing records as long as there is more data available
     // synchronously.
-  } while (total_bytes_read < buf_len && ssl_ret > 0 &&
-           transport_adapter_->HasPendingReadData());
+  } while (ssl_err == SSL_ERROR_WANT_RENEGOTIATE ||
+           (total_bytes_read < buf_len && ssl_ret > 0 &&
+            transport_adapter_->HasPendingReadData()));
 
   // Although only the final SSL_read call may have failed, the failure needs to
   // processed immediately, while the information still available in OpenSSL's
   // error queue.
   if (ssl_ret <= 0) {
-    pending_read_ssl_error_ = SSL_get_error(ssl_.get(), ssl_ret);
+    pending_read_ssl_error_ = ssl_err;
     if (pending_read_ssl_error_ == SSL_ERROR_ZERO_RETURN) {
       pending_read_error_ = 0;
     } else if (pending_read_ssl_error_ == SSL_ERROR_WANT_X509_LOOKUP &&
@@ -1419,6 +1450,22 @@ int SSLClientSocketImpl::DoPayloadWrite() {
   return net_error;
 }
 
+void SSLClientSocketImpl::DoPeek() {
+  if (ssl_config_.disable_post_handshake_peek_for_testing ||
+      !completed_connect_ || peek_complete_) {
+    return;
+  }
+
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+
+  char byte;
+  int rv = SSL_peek(ssl_.get(), &byte, 1);
+  int ssl_err = SSL_get_error(ssl_.get(), rv);
+  if (ssl_err != SSL_ERROR_WANT_READ && ssl_err != SSL_ERROR_WANT_WRITE) {
+    peek_complete_ = true;
+  }
+}
+
 void SSLClientSocketImpl::RetryAllOperations() {
   // SSL_do_handshake, SSL_read, and SSL_write may all be retried when blocked,
   // so retry all operations for simplicity. (Otherwise, SSL_get_error for each
@@ -1436,6 +1483,8 @@ void SSLClientSocketImpl::RetryAllOperations() {
   if (!guard.get())
     return;
 
+  DoPeek();
+
   int rv_read = ERR_IO_PENDING;
   int rv_write = ERR_IO_PENDING;
   if (user_read_buf_) {

net/socket/ssl_client_socket_impl.h

@@ -136,6 +136,7 @@ class SSLClientSocketImpl : public SSLClientSocket,
   int DoHandshakeLoop(int last_io_result);
   int DoPayloadRead(IOBuffer* buf, int buf_len);
   int DoPayloadWrite();
+  void DoPeek();
 
   // Called when an asynchronous event completes which may have blocked the
   // pending Connect, Read or Write calls, if any. Retries all state machines
@@ -272,6 +273,9 @@ class SSLClientSocketImpl : public SSLClientSocket,
   // True if we are currently confirming the handshake.
   bool in_confirm_handshake_;
 
+  // True if the post-handshake SSL_peek has completed.
+  bool peek_complete_;
+
   // True if the socket has been disconnected.
   bool disconnected_;
 

net/ssl/ssl_config.cc

@@ -21,13 +21,7 @@ SSLConfig::CertAndStatus::CertAndStatus(scoped_refptr<X509Certificate> cert_arg,
 SSLConfig::CertAndStatus::CertAndStatus(const CertAndStatus& other) = default;
 SSLConfig::CertAndStatus::~CertAndStatus() = default;
 
-SSLConfig::SSLConfig()
-    : early_data_enabled(false),
-      require_ecdhe(false),
-      ignore_certificate_errors(false),
-      disable_cert_verification_network_fetches(false),
-      renego_allowed_default(false),
-      privacy_mode(PRIVACY_MODE_DISABLED) {}
+SSLConfig::SSLConfig() = default;
 
 SSLConfig::SSLConfig(const SSLConfig& other) = default;
 

net/ssl/ssl_config.h

@@ -73,10 +73,10 @@ struct NET_EXPORT SSLConfig {
   // high-level operation.
   //
   // If unsure, do not enable this option.
-  bool early_data_enabled;
+  bool early_data_enabled = false;
 
   // If true, causes only ECDHE cipher suites to be enabled.
-  bool require_ecdhe;
+  bool require_ecdhe = false;
 
   // TODO(wtc): move the following members to a new SSLParams structure.  They
   // are not SSL configuration settings.
@@ -98,7 +98,7 @@ struct NET_EXPORT SSLConfig {
   std::vector<CertAndStatus> allowed_bad_certs;
 
   // True if all certificate errors should be ignored.
-  bool ignore_certificate_errors;
+  bool ignore_certificate_errors = false;
 
   // True if, for a single connection, any dependent network fetches should
   // be disabled. This can be used to avoid triggering re-entrancy in the
@@ -106,7 +106,7 @@ struct NET_EXPORT SSLConfig {
   // AIA, OCSP, or CRL fetches to block on retrieving the PAC script, while
   // the PAC script fetch is waiting for those dependent fetches, creating a
   // deadlock.
-  bool disable_cert_verification_network_fetches;
+  bool disable_cert_verification_network_fetches = false;
 
   // The list of application level protocols supported with ALPN (Application
   // Layer Protocol Negotiation), in decreasing order of preference.  Protocols
@@ -115,7 +115,7 @@ struct NET_EXPORT SSLConfig {
 
   // True if renegotiation should be allowed for the default application-level
   // protocol when the peer negotiates neither ALPN nor NPN.
-  bool renego_allowed_default;
+  bool renego_allowed_default = false;
 
   // The list of application-level protocols to enable renegotiation for.
   NextProtoVector renego_allowed_for_protos;
@@ -132,7 +132,12 @@ struct NET_EXPORT SSLConfig {
   // current session cache partitioning behavior will be needed to correctly
   // implement it. For now, it acts as an incomplete version of
   // PartitionSSLSessionsByNetworkIsolationKey.
-  PrivacyMode privacy_mode;
+  PrivacyMode privacy_mode = PRIVACY_MODE_DISABLED;
+
+  // True if the post-handshake peeking of the transport should be skipped. This
+  // logic ensures 0-RTT and tickets are resolved early, but can interfere with
+  // some unit tests.
+  bool disable_post_handshake_peek_for_testing = false;
 };
 
 }  // namespace net