From afcee0eb2d36f4b090d82d656531e96913ef9dde Mon Sep 17 00:00:00 2001 From: Alexey Kozy Date: Sat, 25 Apr 2026 21:32:21 -0700 Subject: [PATCH] crypto: fix TLSWrap use-after-free on pending write EncOut() passes pointers from the enc_out_ BIO internal buffer to the underlying stream's uv_write(). write_size_ is non-zero while that write is in flight. Calling ssl_.reset() frees the SSL context and its BIOs, turning those pointers into dangling references. When libuv completes the write it accesses freed memory (SIGSEGV). Use ssl_.release() instead of ssl_.reset() when write_size_ != 0 so the BIO data stays alive for the in-flight uv_write. This is a bounded leak (one SSL context per socket destroyed with in-flight writes) that prevents a segfault. Also move RemoveStreamListener() before SSL cleanup so the underlying stream cannot call back into the TLSWrap after its SSL state is gone. Refs: https://github.com/nodejs/node/issues/62393 Made-with: Cursor --- src/crypto/crypto_tls.cc | 21 +++++-- ...test-tls-destroy-ssl-with-pending-write.js | 56 +++++++++++++++++++ 2 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-tls-destroy-ssl-with-pending-write.js diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 31b7905b72d7f9..f7da1fcd3639ae 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1317,15 +1317,24 @@ void TLSWrap::Destroy() { // And destroy InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction"); - env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize); - ssl_.reset(); - - enc_in_ = nullptr; - enc_out_ = nullptr; - + // Detach from the underlying stream before releasing the SSL context. if (underlying_stream() != nullptr) underlying_stream()->RemoveStreamListener(this); + // EncOut() passes pointers into the enc_out_ BIO internal buffer to the + // underlying stream via uv_write(). write_size_ is non-zero while that + // write is in flight. Freeing the SSL context would turn those pointers + // into dangling references (use-after-free when libuv completes the + // write). Release ownership without freeing so the BIO data stays alive. + if (write_size_ != 0) { + ssl_.release(); + } else { + ssl_.reset(); + } + + env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize); + enc_in_ = nullptr; + enc_out_ = nullptr; sc_.reset(); } diff --git a/test/parallel/test-tls-destroy-ssl-with-pending-write.js b/test/parallel/test-tls-destroy-ssl-with-pending-write.js new file mode 100644 index 00000000000000..f4ed9d4844fbaf --- /dev/null +++ b/test/parallel/test-tls-destroy-ssl-with-pending-write.js @@ -0,0 +1,56 @@ +'use strict'; + +// Regression test for TLSWrap use-after-free when destroySSL() is called +// while an underlying stream write is still in flight. +// +// EncOut() passes pointers into the enc_out_ BIO's internal buffer to the +// underlying stream via uv_write(). If the SSL context is freed before that +// write completes, libuv accesses freed memory (SIGSEGV). +// +// The GC weak callback path triggers the same Destroy() code when a +// TLSSocket with pending writes is collected without an explicit destroy(). + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const tls = require('node:tls'); + +const server = tls.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}, (socket) => { + // Do not read — TCP backpressure keeps client writes pending in the + // TLSWrap's underlying stream so write_size_ stays non-zero. + socket.pause(); +}); + +server.listen(0, common.mustCall(() => { + const { port } = server.address(); + let remaining = 10; + + for (let i = 0; i < 10; i++) { + const socket = tls.connect( + { port, host: '127.0.0.1', rejectUnauthorized: false }, + common.mustCall(() => { + // Queue a write large enough to stay pending (server never reads). + socket.write(Buffer.alloc(1024 * 1024)); + + // Simulate the GC weak callback path: free the SSL context while + // the underlying stream write is still in flight. + if (socket._handle && socket._handle.destroySSL) { + socket._handle.destroySSL(); + } + + setTimeout(() => { + socket.destroy(); + if (--remaining === 0) { + server.close(); + } + }, 50); + }), + ); + socket.on('error', () => {}); + } +}));