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

Signed-off-by: Tim Perry <pimterry@gmail.com>

Author: pimterry

src/crypto/crypto_tls.cc

@@ -849,7 +849,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)
@@ -970,7 +973,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()));
 
@@ -1073,6 +1080,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().
@@ -1138,12 +1172,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(
@@ -1229,16 +1269,55 @@ 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());
+    flags_.shutdown = true;
+    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());
+  }
 
   flags_.shutdown = true;
   EncOut();
   return underlying_stream()->DoShutdown(req_wrap);
 }
 
+void TLSWrap::ScheduleDeferredCycle() {
+  if (flags_.deferred_cycle_scheduled) return;
+  flags_.deferred_cycle_scheduled = true;
+
+  BaseObjectPtr<TLSWrap> strong_ref{this};
+  env()->SetImmediate([this, strong_ref](Environment* env) {
+    flags_.deferred_cycle_scheduled = false;
+    if (ssl_) Cycle();
+  });
+}
+
+void TLSWrap::FlushPendingShutdown() {
+  if (!pending_shutdown_ || !ssl_) return;
+
+  if (!SSL_is_init_finished(ssl_.get())) {
+    Debug(this, "Holding deferred shutdown, handshake still in progress");
+    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());
@@ -1335,6 +1414,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();
 
@@ -2177,13 +2263,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,
@@ -194,6 +211,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;
@@ -296,6 +323,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_;
 
   // TODO(@jasnell): These state flags should be revisited.
@@ -316,6 +346,7 @@ class TLSWrap : public AsyncWrap,
     bool shutdown : 1;
     bool cert_cb_running : 1;
     bool eof : 1;
+    bool deferred_cycle_scheduled : 1;
     bool established : 1;
     bool write_callback_scheduled : 1;
     bool has_active_write_issued_by_prev_listener : 1;
@@ -325,6 +356,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,53 @@
+'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');
+
+function test(maxVersion) {
+  const server = tls.createServer({
+    key: fixtures.readKey('agent1-key.pem'),
+    cert: fixtures.readKey('agent1-cert.pem'),
+    maxVersion,
+    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,
+      maxVersion,
+    }, 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('TLSv1.2');
+test('TLSv1.3');

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