From 86694861ad294a99d9b486050ed08ab34c74aff3 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Fri, 7 Aug 2026 16:24:15 +0200 Subject: [PATCH 1/2] tls: defer re-entrant calls to SSL state machine from JS Signed-off-by: Tim Perry --- src/crypto/crypto_tls.cc | 109 +++++++++++++++++- src/crypto/crypto_tls.h | 34 ++++++ .../test-tls-alpn-callback-sync-end.js | 53 +++++++++ .../test-tls-alpn-callback-sync-write.js | 49 ++++++++ test/parallel/test-tls-keylog-sync-write.js | 55 +++++++++ 5 files changed, 294 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-tls-alpn-callback-sync-end.js create mode 100644 test/parallel/test-tls-alpn-callback-sync-write.js create mode 100644 test/parallel/test-tls-keylog-sync-write.js diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 14cf5cc8c85d..5f5379b8af87 100644 --- a/src/crypto/crypto_tls.cc +++ b/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(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 bs = ArrayBuffer::NewBackingStore( + env()->isolate(), + length, + BackingStoreInitializationMode::kUninitialized); + size_t offset = 0; + for (i = 0; i < count; i++) { + memcpy( + static_cast(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 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 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 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& 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 pending = std::move(pending_shutdown_); + ShutdownWrap::FromObject(pending)->Done(UV_ECANCELED); + } + env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize); ssl_.reset(); @@ -2177,6 +2263,13 @@ 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; @@ -2184,6 +2277,10 @@ void TLSWrap::Cycle() { 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(); } diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index 2d3ccef24fac..ab6024018873 100644 --- a/src/crypto/crypto_tls.h +++ b/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 target, v8::Local unused, v8::Local 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 current_write_; BaseObjectPtr 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 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; diff --git a/test/parallel/test-tls-alpn-callback-sync-end.js b/test/parallel/test-tls-alpn-callback-sync-end.js new file mode 100644 index 000000000000..96c639983901 --- /dev/null +++ b/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'); diff --git a/test/parallel/test-tls-alpn-callback-sync-write.js b/test/parallel/test-tls-alpn-callback-sync-write.js new file mode 100644 index 000000000000..d7e2d9098c44 --- /dev/null +++ b/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()); +})); diff --git a/test/parallel/test-tls-keylog-sync-write.js b/test/parallel/test-tls-keylog-sync-write.js new file mode 100644 index 000000000000..5e806ae62e7a --- /dev/null +++ b/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()); +})); From cce7aa488a1f178d0c35daca20c0534ec1373736 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Wed, 9 Sep 2026 12:40:30 +0200 Subject: [PATCH 2/2] Defer shutdown until after newSession resolves --- src/crypto/crypto_tls.cc | 6 +- ...t-tls-alpn-callback-sync-end-newsession.js | 57 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-tls-alpn-callback-sync-end-newsession.js diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 5f5379b8af87..07c8d51ffade 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1307,8 +1307,10 @@ void TLSWrap::ScheduleDeferredCycle() { void TLSWrap::FlushPendingShutdown() { if (!pending_shutdown_ || !ssl_) return; - if (!SSL_is_init_finished(ssl_.get())) { - Debug(this, "Holding deferred shutdown, handshake still in progress"); + const bool can_send_close_notify = + SSL_is_init_finished(ssl_.get()) && !is_awaiting_new_session(); + if (!can_send_close_notify) { + Debug(this, "Holding deferred shutdown, cannot send close_notify yet"); return; } diff --git a/test/parallel/test-tls-alpn-callback-sync-end-newsession.js b/test/parallel/test-tls-alpn-callback-sync-end-newsession.js new file mode 100644 index 000000000000..b3d085192e24 --- /dev/null +++ b/test/parallel/test-tls-alpn-callback-sync-end-newsession.js @@ -0,0 +1,57 @@ +'use strict'; + +// A shutdown deferred off the SSL library's stack must not run while the +// 'newSession' callback is still outstanding. Make sure it's deferred +// correctly so the connection still cleanly closes. + +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'), + // TLSv1.3 issues its session tickets after the handshake, which is what puts + // the 'newSession' callback and the deferred shutdown in the same window. + minVersion: 'TLSv1.3', + maxVersion: 'TLSv1.3', + ALPNCallback: common.mustCall(function({ protocols }) { + this.end(); + return protocols[0]; + }), +}); + +// Answering asynchronously holds EncOut() while the shutdown is replayed. +server.on('newSession', common.mustCallAtLeast((id, data, callback) => { + setImmediate(callback); +})); + +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, + minVersion: 'TLSv1.3', + maxVersion: 'TLSv1.3', + }, common.mustCall(() => { + assert.strictEqual(client.alpnProtocol, 'a'); + })); + + // The tickets have to survive the shutdown, not be cut off by the FIN. + client.on('session', common.mustCallAtLeast()); + client.on('close', common.mustCall((hadError) => { + assert.strictEqual(hadError, false); + server.close(); + })); + client.on('error', common.mustNotCall()); +}));