From 23a28289346da1301d063658a5f05f2b6a488a3e Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Fri, 7 Aug 2026 16:24:15 +0200 Subject: [PATCH] tls: defer re-entrant calls to SSL state machine from JS Signed-off-by: Tim Perry --- src/crypto/crypto_tls.cc | 103 +++++++++++++++++- src/crypto/crypto_tls.h | 34 ++++++ .../test-tls-alpn-callback-sync-end.js | 46 ++++++++ .../test-tls-alpn-callback-sync-write.js | 49 +++++++++ test/parallel/test-tls-keylog-sync-write.js | 55 ++++++++++ 5 files changed, 281 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 8ef74aee2d0e..8dad063531f0 100644 --- a/src/crypto/crypto_tls.cc +++ b/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(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 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(). @@ -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 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 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 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()); @@ -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 pending = std::move(pending_shutdown_); + ShutdownWrap::FromObject(pending)->Done(UV_ECANCELED); + } + env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize); ssl_.reset(); @@ -2193,6 +2273,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; @@ -2200,6 +2287,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 a5ded3392915..5f4c9eb50172 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, @@ -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 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_; 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; 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..b46c8bcd1592 --- /dev/null +++ b/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()); +})); 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()); +}));