From 75930bb38c734fa0f8f4dbedb4187ce61dd08e3d Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Mon, 18 May 2015 20:24:19 +0200 Subject: [PATCH] tls: prevent use-after-free * Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: https://github.com/joyent/node/issues/8780 Fix: https://github.com/iojs/io.js/issues/1696 PR-URL: https://github.com/nodejs/io.js/pull/1702 Reviewed-By: Trevor Norris --- lib/_tls_wrap.js | 7 ++++++- src/tls_wrap.cc | 13 +++++++++++++ test/parallel/test-tls-js-stream.js | 1 + 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 63f1f6ccbf17ef..ede98ee4bf1985 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -305,13 +305,18 @@ TLSSocket.prototype._wrapHandle = function(handle) { }); this.on('close', function() { - this._destroySSL(); + // Make sure we are not doing it on OpenSSL's stack + setImmediate(destroySSL, this); res = null; }); return res; }; +function destroySSL(self) { + self._destroySSL(); +} + TLSSocket.prototype._destroySSL = function _destroySSL() { if (!this.ssl) return; this.ssl.destroySSL(); diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 30768640eb99e2..ca0690defbd13b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -337,6 +337,9 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) { return; } + if (wrap->ssl_ == nullptr) + return; + // Commit NodeBIO::FromBIO(wrap->enc_out_)->Read(nullptr, wrap->write_size_); @@ -554,6 +557,7 @@ int TLSWrap::DoWrite(WriteWrap* w, size_t count, uv_stream_t* send_handle) { CHECK_EQ(send_handle, nullptr); + CHECK_NE(ssl_, nullptr); bool empty = true; @@ -627,6 +631,11 @@ void TLSWrap::OnAfterWriteImpl(WriteWrap* w, void* ctx) { void TLSWrap::OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) { TLSWrap* wrap = static_cast(ctx); + if (wrap->ssl_ == nullptr) { + *buf = uv_buf_init(nullptr, 0); + return; + } + size_t size = 0; buf->base = NodeBIO::FromBIO(wrap->enc_in_)->PeekWritable(&size); buf->len = size; @@ -747,6 +756,10 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { void TLSWrap::EnableSessionCallbacks( const FunctionCallbackInfo& args) { TLSWrap* wrap = Unwrap(args.Holder()); + if (wrap->ssl_ == nullptr) { + return wrap->env()->ThrowTypeError( + "EnableSessionCallbacks after destroySSL"); + } wrap->enable_session_callbacks(); NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength); wrap->hello_parser_.Start(SSLWrap::OnClientHello, diff --git a/test/parallel/test-tls-js-stream.js b/test/parallel/test-tls-js-stream.js index e156f446f57de9..292bd4fd9123dd 100644 --- a/test/parallel/test-tls-js-stream.js +++ b/test/parallel/test-tls-js-stream.js @@ -61,6 +61,7 @@ var server = tls.createServer({ socket.end('hello'); socket.resume(); + socket.destroy(); }); socket.once('close', function() {