From c359ada7fc3be2f71b501280a447aa89a996d97e Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Thu, 14 May 2015 10:38:18 +0200 Subject: [PATCH 1/3] tls: emit errors on close whilst async action When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: https://github.com/joyent/node/issues/8780 Fix: https://github.com/iojs/io.js/issues/1696 --- lib/_tls_wrap.js | 15 ++++ .../test-tls-async-cb-after-socket-end.js | 73 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 test/parallel/test-tls-async-cb-after-socket-end.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 9c934b9ffbbe3f..63f1f6ccbf17ef 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -61,6 +61,9 @@ function loadSession(self, hello, cb) { if (err) return cb(err); + if (!self._handle) + return cb(new Error('Socket is closed')); + // NOTE: That we have disabled OpenSSL's internal session storage in // `node_crypto.cc` and hence its safe to rely on getting servername only // from clienthello or this place. @@ -91,6 +94,9 @@ function loadSNI(self, servername, cb) { if (err) return cb(err); + if (!self._handle) + return cb(new Error('Socket is closed')); + // TODO(indutny): eventually disallow raw `SecureContext` if (context) self._handle.sni_context = context.context || context; @@ -127,6 +133,9 @@ function requestOCSP(self, hello, ctx, cb) { if (err) return cb(err); + if (!self._handle) + return cb(new Error('Socket is closed')); + if (response) self._handle.setOCSPResponse(response); cb(null); @@ -157,6 +166,9 @@ function oncertcb(info) { if (err) return self.destroy(err); + if (!self._handle) + return cb(new Error('Socket is closed')); + self._handle.certCbDone(); }); }); @@ -179,6 +191,9 @@ function onnewsession(key, session) { return; once = true; + if (!self._handle) + return cb(new Error('Socket is closed')); + self._handle.newSessionDone(); self._newSessionPending = false; diff --git a/test/parallel/test-tls-async-cb-after-socket-end.js b/test/parallel/test-tls-async-cb-after-socket-end.js new file mode 100644 index 00000000000000..db9db87f59087c --- /dev/null +++ b/test/parallel/test-tls-async-cb-after-socket-end.js @@ -0,0 +1,73 @@ +'use strict'; + +var common = require('../common'); + +var assert = require('assert'); +var path = require('path'); +var fs = require('fs'); +var constants = require('constants'); + +if (!common.hasCrypto) { + console.log('1..0 # Skipped: missing crypto'); + process.exit(); +} + +var tls = require('tls'); + +var options = { + secureOptions: constants.SSL_OP_NO_TICKET, + key: fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem')), + cert: fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem')) +}; + +var server = tls.createServer(options, function(c) { +}); + +var sessionCb = null; +var client = null; + +server.on('newSession', function(key, session, done) { + done(); +}); + +server.on('resumeSession', function(id, cb) { + sessionCb = cb; + + next(); +}); + +server.listen(1443, function() { + var clientOpts = { + port: 1443, + rejectUnauthorized: false, + session: false + }; + + var s1 = tls.connect(clientOpts, function() { + clientOpts.session = s1.getSession(); + console.log('1st secure'); + + s1.destroy(); + var s2 = tls.connect(clientOpts, function(s) { + console.log('2nd secure'); + + s2.destroy(); + }).on('connect', function() { + console.log('2nd connected'); + client = s2; + + next(); + }); + }); +}); + +function next() { + if (!client || !sessionCb) + return; + + client.destroy(); + setTimeout(function() { + sessionCb(); + server.close(); + }, 100); +} From c0f4ddfe97aeb4ec2ffd17763260b120dca7a7db Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Mon, 18 May 2015 20:24:19 +0200 Subject: [PATCH 2/3] 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 --- 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() { From 3533378d32b647b8fcb98a152a6ef39b16d4058b Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 19 May 2015 13:18:42 +0200 Subject: [PATCH 3/3] tls_wrap: invoke queued callbacks in DestroySSL --- src/tls_wrap.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index ca0690defbd13b..b8a648de923081 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -86,12 +86,6 @@ TLSWrap::~TLSWrap() { sni_context_.Reset(); #endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - // Move all writes to pending - MakePending(); - - // And destroy - InvokeQueued(UV_ECANCELED); - ClearError(); } @@ -770,7 +764,16 @@ void TLSWrap::EnableSessionCallbacks( void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { TLSWrap* wrap = Unwrap(args.Holder()); + + // Move all writes to pending + wrap->MakePending(); + + // And destroy + wrap->InvokeQueued(UV_ECANCELED); + + // Destroy the SSL structure and friends wrap->SSLWrap::DestroySSL(); + delete wrap->clear_in_; wrap->clear_in_ = nullptr; }