CASSCPP-16 Fix TLS 1.3 handshake race with in-flight Finished write

src/socket.cpp

@@ -225,6 +225,14 @@ void SslSocketHandler::on_read(Socket* socket, ssize_t nread, const uv_buf_t* bu
 
 uv_tcp_t* SocketWriteBase::tcp() { return &socket_->tcp_; }
 
+SocketWriteBase::SocketWriteBase(Socket* socket)
+    : socket_(socket)
+    , is_flushed_(false)
+    , handler_generation_(socket->handler_generation()) {
+  req_.data = this;
+  buffers_.reserve(MIN_BUFFERS_SIZE);
+}
+
 void SocketWriteBase::on_close() {
   for (RequestVec::iterator i = requests_.begin(), end = requests_.end(); i != end; ++i) {
     (*i)->on_close();
@@ -267,7 +275,9 @@ void SocketWriteBase::handle_write(uv_write_t* req, int status) {
 
   socket->pending_writes_.remove(this);
 
-  if (socket->free_writes_.size() < socket->max_reusable_write_objects_) {
+  // Don't recycle a write created under a handler that's since been replaced.
+  if (handler_generation_ == socket->handler_generation_ &&
+      socket->free_writes_.size() < socket->max_reusable_write_objects_) {
     clear();
     socket->free_writes_.push_back(this);
   } else {
@@ -278,7 +288,8 @@ void SocketWriteBase::handle_write(uv_write_t* req, int status) {
 }
 
 Socket::Socket(const Address& address, size_t max_reusable_write_objects)
-    : is_defunct_(false)
+    : handler_generation_(0)
+    , is_defunct_(false)
     , max_reusable_write_objects_(max_reusable_write_objects)
     , address_(address) {
   tcp_.data = this;
@@ -288,6 +299,7 @@ Socket::~Socket() { cleanup_free_writes(); }
 
 void Socket::set_handler(SocketHandlerBase* handler) {
   handler_.reset(handler);
+  ++handler_generation_;
   cleanup_free_writes();
   free_writes_.clear();
   if (handler_) {

src/socket.hpp

@@ -206,12 +206,7 @@ class SocketWriteBase
    *
    * @param The socket handling the write.
    */
-  SocketWriteBase(Socket* socket)
-      : socket_(socket)
-      , is_flushed_(false) {
-    req_.data = this;
-    buffers_.reserve(MIN_BUFFERS_SIZE);
-  }
+  SocketWriteBase(Socket* socket);
 
   virtual ~SocketWriteBase() {}
 
@@ -260,6 +255,8 @@ class SocketWriteBase
   Socket* socket_;
   uv_write_t req_;
   bool is_flushed_;
+  // Socket's handler generation when this write was created, to avoid reuse under a new handler.
+  size_t handler_generation_;
   BufferVec buffers_;
   RequestVec requests_;
 };
@@ -298,6 +295,11 @@ class Socket : public RefCounted<Socket> {
    */
   void set_handler(SocketHandlerBase* handler);
 
+  /**
+   * The number of times the socket's handler has been set.
+   */
+  size_t handler_generation() const { return handler_generation_; }
+
   /**
    * Write a request to the socket and coalesce with outstanding requests. This
    * method doesn't flush.
@@ -367,6 +369,7 @@ class Socket : public RefCounted<Socket> {
 
   uv_tcp_t tcp_;
   ScopedPtr<SocketHandlerBase> handler_;
+  size_t handler_generation_;
 
   SocketWriteBase::List pending_writes_;
   SocketWriteVec free_writes_;

src/socket_connector.cpp

@@ -65,6 +65,11 @@ class SslHandshakeHandler : public SocketHandler {
     delete request;
     if (status != 0) {
       connector_->on_error(SocketConnector::SOCKET_ERROR_WRITE, "Write error");
+      return;
+    }
+    // TLS 1.3 can finish the handshake while this write was still queued.
+    if (connector_->ssl_session_->is_handshake_done()) {
+      connector_->ssl_handshake_finish();
     }
   }
 
@@ -207,21 +212,26 @@ void SocketConnector::ssl_handshake() {
     }
   }
 
-  // Write any outgoing data created by the handshake process.
+  // Write any outgoing data created by the handshake process. Finishing is
+  // deferred to on_write() if the handshake is already done (e.g. TLS 1.3).
   char buf[SSL_HANDSHAKE_MAX_BUFFER_SIZE];
   size_t size = ssl_session_->outgoing().read(buf, SSL_HANDSHAKE_MAX_BUFFER_SIZE);
   if (size > 0) {
     socket_->write_and_flush(new BufferSocketRequest(Buffer(buf, size)));
-  } else if (ssl_session_->is_handshake_done()) { // If the handshake process is done then verify
-                                                  // the certificate and finish.
-    ssl_session_->verify();
-    if (ssl_session_->has_error()) {
-      on_error(SOCKET_ERROR_SSL_VERIFY,
-               "Error verifying peer certificate: " + ssl_session_->error_message());
-      return;
-    }
-    finish();
+  } else if (ssl_session_->is_handshake_done()) {
+    ssl_handshake_finish();
+  }
+}
+
+void SocketConnector::ssl_handshake_finish() {
+  // If the handshake process is done then verify the certificate and finish.
+  ssl_session_->verify();
+  if (ssl_session_->has_error()) {
+    on_error(SOCKET_ERROR_SSL_VERIFY,
+             "Error verifying peer certificate: " + ssl_session_->error_message());
+    return;
   }
+  finish();
 }
 
 void SocketConnector::finish() {

src/socket_connector.hpp

@@ -138,6 +138,7 @@ class SocketConnector : public RefCounted<SocketConnector> {
 private:
   void internal_connect(uv_loop_t* loop);
   void ssl_handshake();
+  void ssl_handshake_finish();
   void finish();
 
   void on_error(SocketError code, const String& message);

src/ssl/ssl_openssl_impl.cpp

@@ -541,8 +541,8 @@ OpenSslContext::OpenSslContext()
   SSL_CTX_set_cert_store(ssl_ctx_, trusted_store_);
   SSL_CTX_set_verify(ssl_ctx_, SSL_VERIFY_NONE, ssl_no_verify_callback);
 #if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
-  // Limit to TLS 1.2 for now. TLS 1.3 has broken the handshake code.
-  SSL_CTX_set_max_proto_version(ssl_ctx_, TLS1_2_VERSION);
+  // Allow up to TLS 1.3.
+  SSL_CTX_set_max_proto_version(ssl_ctx_, TLS1_3_VERSION);
 #endif
 #if DEBUG_SSL
   SSL_CTX_set_info_callback(ssl_ctx_, ssl_info_callback);