tls: defer re-entrant calls to SSL state machine from JS

src/crypto/crypto_tls.cc

@@ -870,7 +870,10 @@ void TLSWrap::ClearOut() {
   char out[kClearOutChunkSize];
   int read;
   for (;;) {
-    read = SSL_read(ssl_.get(), out, sizeof(out));
+    {
+      SSLLibraryCallScope ssl_library_call_scope(this);
+      read = SSL_read(ssl_.get(), out, sizeof(out));
+    }
     Debug(this, "Read %d bytes of cleartext output", read);
 
     if (read <= 0)
@@ -991,7 +994,11 @@ void TLSWrap::ClearIn() {
   MarkPopErrorOnReturn mark_pop_error_on_return;
 
   NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(bs->ByteLength());
-  int written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength());
+  int written;
+  {
+    SSLLibraryCallScope ssl_library_call_scope(this);
+    written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength());
+  }
   Debug(this, "Writing %zu bytes, written = %d", bs->ByteLength(), written);
   CHECK(written == -1 || written == static_cast<int>(bs->ByteLength()));
 
@@ -1094,6 +1101,33 @@ int TLSWrap::DoWrite(WriteWrap* w,
     }
   }
 
+  // If we got here from a call inside the OpenSSL/BoringSSL stack, we need to
+  // defer reentrant write calls:
+  if (in_ssl_library_call()) {
+    Debug(this, "Deferring write issued from the SSL library's stack");
+    CHECK(!current_write_);
+    current_write_.reset(w->GetAsyncWrap());
+
+    if (length > 0) {
+      CHECK(!pending_cleartext_input_ ||
+            pending_cleartext_input_->ByteLength() == 0);
+      std::unique_ptr<BackingStore> bs = ArrayBuffer::NewBackingStore(
+          env()->isolate(),
+          length,
+          BackingStoreInitializationMode::kUninitialized);
+      size_t offset = 0;
+      for (i = 0; i < count; i++) {
+        memcpy(
+            static_cast<char*>(bs->Data()) + offset, bufs[i].base, bufs[i].len);
+        offset += bufs[i].len;
+      }
+      pending_cleartext_input_ = std::move(bs);
+    }
+
+    ScheduleDeferredCycle();
+    return 0;
+  }
+
   // We want to trigger a Write() on the underlying stream to drive the stream
   // system, but don't want to encrypt empty buffers into a TLS frame, so see
   // if we can find something to Write().
@@ -1159,12 +1193,18 @@ int TLSWrap::DoWrite(WriteWrap* w,
     }
 
     NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length);
-    written = SSL_write(ssl_.get(), bs->Data(), length);
+    {
+      SSLLibraryCallScope ssl_library_call_scope(this);
+      written = SSL_write(ssl_.get(), bs->Data(), length);
+    }
   } else {
     // Only one buffer: try to write directly, only store if it fails
     uv_buf_t* buf = &bufs[nonempty_i];
     NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len);
-    written = SSL_write(ssl_.get(), buf->base, buf->len);
+    {
+      SSLLibraryCallScope ssl_library_call_scope(this);
+      written = SSL_write(ssl_.get(), buf->base, buf->len);
+    }
 
     if (written == -1) {
       bs = ArrayBuffer::NewBackingStore(
@@ -1251,16 +1291,49 @@ ShutdownWrap* TLSWrap::CreateShutdownWrap(Local<Object> req_wrap_object) {
 
 int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) {
   Debug(this, "DoShutdown()");
+
+  // We must not call SSL_shutdown from inside the TLS library stack, so
+  // defer if required:
+  if (in_ssl_library_call()) {
+    Debug(this, "Deferring shutdown issued from the SSL library's stack");
+    CHECK(!pending_shutdown_);
+    pending_shutdown_.reset(req_wrap->GetAsyncWrap());
+    ScheduleDeferredCycle();
+    return 0;
+  }
+
   MarkPopErrorOnReturn mark_pop_error_on_return;
 
-  if (ssl_ && SSL_shutdown(ssl_.get()) == 0)
-    SSL_shutdown(ssl_.get());
+  if (ssl_) {
+    SSLLibraryCallScope ssl_library_call_scope(this);
+    if (SSL_shutdown(ssl_.get()) == 0) SSL_shutdown(ssl_.get());
+  }
 
   shutdown_ = true;
   EncOut();
   return underlying_stream()->DoShutdown(req_wrap);
 }
 
+void TLSWrap::ScheduleDeferredCycle() {
+  if (deferred_cycle_scheduled_) return;
+  deferred_cycle_scheduled_ = true;
+
+  BaseObjectPtr<TLSWrap> strong_ref{this};
+  env()->SetImmediate([this, strong_ref](Environment* env) {
+    deferred_cycle_scheduled_ = false;
+    if (ssl_) Cycle();
+  });
+}
+
+void TLSWrap::FlushPendingShutdown() {
+  if (!pending_shutdown_ || !ssl_) return;
+
+  BaseObjectPtr<AsyncWrap> pending = std::move(pending_shutdown_);
+  ShutdownWrap* req_wrap = ShutdownWrap::FromObject(pending);
+  int err = DoShutdown(req_wrap);
+  if (err != 0) req_wrap->Done(err);
+}
+
 void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& args) {
   TLSWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.This());
@@ -1357,6 +1430,13 @@ void TLSWrap::Destroy() {
   // And destroy
   InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");
 
+  // A shutdown held back off the SSL library's stack will never be replayed
+  // now, so complete it here rather than leaving the stream waiting on it.
+  if (pending_shutdown_) {
+    BaseObjectPtr<AsyncWrap> pending = std::move(pending_shutdown_);
+    ShutdownWrap::FromObject(pending)->Done(UV_ECANCELED);
+  }
+
   env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize);
   ssl_.reset();
 
@@ -2193,13 +2273,24 @@ void TLSWrap::WritesIssuedByPrevListenerDone(
 }
 
 void TLSWrap::Cycle() {
+  // With no loop to extend, cycling now would re-enter the SSL library.
+  if (cycle_depth_ == 0 && in_ssl_library_call()) {
+    Debug(this, "Deferring cycle requested from the SSL library's stack");
+    ScheduleDeferredCycle();
+    return;
+  }
+
   // Prevent recursion
   if (++cycle_depth_ > 1)
     return;
 
   for (; cycle_depth_ > 0; cycle_depth_--) {
     ClearIn();
     ClearOut();
+    // ClearOut() could defer a write/shutdown, so we ClearIn() again now
+    // to avoid needing a second pass:
+    ClearIn();
+    FlushPendingShutdown();
     // EncIn() doesn't exist, it happens via stream listener callbacks.
     EncOut();
   }

src/crypto/crypto_tls.h

@@ -54,6 +54,23 @@ class TLSWrap : public AsyncWrap,
 
   enum class UnderlyingStreamWriteStatus { kHasActive, kVacancy };
 
+  // The SSL library's state machine is not reentrant. Node holds this scope
+  // across every call into it, so that JS the SSL library invokes on its own
+  // stack is recognised and kept from re-entering.
+  class SSLLibraryCallScope {
+   public:
+    explicit SSLLibraryCallScope(TLSWrap* wrap) : wrap_(wrap) {
+      wrap_->ssl_library_call_depth_++;
+    }
+    ~SSLLibraryCallScope() { wrap_->ssl_library_call_depth_--; }
+
+    SSLLibraryCallScope(const SSLLibraryCallScope&) = delete;
+    SSLLibraryCallScope& operator=(const SSLLibraryCallScope&) = delete;
+
+   private:
+    TLSWrap* wrap_;
+  };
+
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
                          v8::Local<v8::Context> context,
@@ -182,6 +199,16 @@ class TLSWrap : public AsyncWrap,
   // underlying stream even if there is no clear text to read or write.
   void Cycle();
 
+  inline bool in_ssl_library_call() const {
+    return ssl_library_call_depth_ > 0;
+  }
+
+  // Setup Cycle() after a library call, to flush anything DoWrite() held back
+  void ScheduleDeferredCycle();
+
+  // Flush a shutdown held back by DoShutdown(), if there is one.
+  void FlushPendingShutdown();
+
   // Implement StreamListener:
   // Returns buf that points into enc_in_.
   uv_buf_t OnStreamAlloc(size_t size) override;
@@ -282,6 +309,9 @@ class TLSWrap : public AsyncWrap,
   size_t write_size_ = 0;
   BaseObjectPtr<AsyncWrap> current_write_;
   BaseObjectPtr<AsyncWrap> current_empty_write_;
+  // Set when DoShutdown() was called while the SSL library was on the stack,
+  // and so has yet to send close_notify.
+  BaseObjectPtr<AsyncWrap> pending_shutdown_;
   std::string error_;
 
   bool session_callbacks_ = false;
@@ -295,6 +325,7 @@ class TLSWrap : public AsyncWrap,
   bool shutdown_ = false;
   bool cert_cb_running_ = false;
   bool eof_ = false;
+  bool deferred_cycle_scheduled_ = false;
 
   // TODO(@jasnell): These state flags should be revisited.
   // The established_ flag indicates that the handshake is
@@ -307,6 +338,9 @@ class TLSWrap : public AsyncWrap,
 
   int cycle_depth_ = 0;
 
+  // Nesting depth of calls into the SSL library. See SSLLibraryCallScope.
+  int ssl_library_call_depth_ = 0;
+
   // SSL_set_cert_cb
   CertCb cert_cb_ = nullptr;
   void* cert_cb_arg_ = nullptr;

test/parallel/test-tls-alpn-callback-sync-end.js

@@ -0,0 +1,46 @@
+'use strict';
+
+// Ending a server TLSSocket synchronously from inside an ALPNCallback must
+// finish the handshake and then shut the connection down cleanly, rather than
+// dropping the underlying socket part way through it.
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const tls = require('tls');
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+  ALPNCallback: common.mustCall(function({ protocols }) {
+    this.end();
+    return protocols[0];
+  }),
+});
+
+server.on('tlsClientError', common.mustNotCall());
+server.on('secureConnection', common.mustCall((socket) => {
+  socket.on('error', common.mustNotCall());
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = tls.connect({
+    port: server.address().port,
+    ALPNProtocols: ['a'],
+    rejectUnauthorized: false,
+  }, common.mustCall(() => {
+    assert.strictEqual(client.alpnProtocol, 'a');
+  }));
+
+  // A clean close_notify, not a truncated connection.
+  client.on('end', common.mustCall());
+  client.on('close', common.mustCall((hadError) => {
+    assert.strictEqual(hadError, false);
+    server.close();
+  }));
+  client.on('error', common.mustNotCall());
+}));

test/parallel/test-tls-alpn-callback-sync-write.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// Writing to a server TLSSocket synchronously from inside an ALPNCallback,
+// which the TLS library invokes on its own stack mid-handshake, must not break
+// the connection; the data must be delivered once the handshake ends.
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const tls = require('tls');
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+  ALPNCallback: common.mustCall(function({ protocols }) {
+    // The write cannot complete until the handshake does, but it must be
+    // accepted and eventually flushed rather than dropped or encrypted into
+    // the middle of the handshake.
+    this.write('from-mid-handshake', common.mustCall());
+    return protocols[0];
+  }),
+});
+
+server.on('tlsClientError', common.mustNotCall());
+server.on('secureConnection', common.mustCall((socket) => {
+  assert.strictEqual(socket.alpnProtocol, 'a');
+  socket.on('error', common.mustNotCall());
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = tls.connect({
+    port: server.address().port,
+    ALPNProtocols: ['a', 'b'],
+    rejectUnauthorized: false,
+  }, common.mustCall(() => {
+    assert.strictEqual(client.alpnProtocol, 'a');
+
+    client.on('data', common.mustCall((data) => {
+      assert.strictEqual(data.toString(), 'from-mid-handshake');
+      client.end();
+      server.close();
+    }));
+  }));
+  client.on('error', common.mustNotCall());
+}));

test/parallel/test-tls-keylog-sync-write.js

@@ -0,0 +1,55 @@
+'use strict';
+
+// The 'keylog' event is emitted from the TLS library's own stack, part way
+// through the handshake. Writing to the socket from the handler must not
+// corrupt the connection; the data must arrive intact.
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const tls = require('tls');
+
+const PAYLOAD = 'from-keylog';
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+}, common.mustCall((socket) => {
+  socket.on('error', common.mustNotCall());
+
+  const onPayload = common.mustCall(() => {
+    assert.strictEqual(received, PAYLOAD);
+    socket.end();
+    server.close();
+  });
+
+  let received = '';
+  socket.on('data', (data) => {
+    received += data;
+    if (received.length >= PAYLOAD.length) onPayload();
+  });
+}));
+
+server.on('tlsClientError', common.mustNotCall());
+
+server.listen(0, common.mustCall(() => {
+  const client = tls.connect({
+    port: server.address().port,
+    rejectUnauthorized: false,
+  });
+
+  // 'keylog' fires once per secret derived, so the count is version dependent.
+  // Write from the first one only, to keep what the server expects exact.
+  let written = false;
+  client.on('keylog', common.mustCallAtLeast(() => {
+    if (written) return;
+    written = true;
+    client.write(PAYLOAD, common.mustCall());
+  }));
+
+  client.on('error', common.mustNotCall());
+}));