From 7573b55a1505e5ea0e2c903e15013bb409fe3c93 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 5 Mar 2019 15:00:09 +0100 Subject: [PATCH] tls: fix legacy SecurePair clienthello race window There is a time window between the first and the last step of processing the clienthello event and the SecurePair may have been destroyed during that interval. Fixes: https://github.com/nodejs/node/issues/26428 PR-URL: https://github.com/nodejs/node/pull/26452 Reviewed-By: Sam Roberts Reviewed-By: James M Snell Reviewed-By: Beth Griggs --- lib/_tls_legacy.js | 4 + ...ls-async-cb-after-socket-end-securepair.js | 80 +++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 test/parallel/test-tls-async-cb-after-socket-end-securepair.js diff --git a/lib/_tls_legacy.js b/lib/_tls_legacy.js index 52de70d4794de1..7b9e34bb847c31 100644 --- a/lib/_tls_legacy.js +++ b/lib/_tls_legacy.js @@ -659,6 +659,10 @@ function onclienthello(hello) { if (err) return self.socket.destroy(err); setImmediate(function() { + // SecurePair might have been destroyed in the time window + // between callback() and this function. + if (!self.ssl) return; + self.ssl.loadSession(session); self.ssl.endParser(); diff --git a/test/parallel/test-tls-async-cb-after-socket-end-securepair.js b/test/parallel/test-tls-async-cb-after-socket-end-securepair.js new file mode 100644 index 00000000000000..890c2b63ef5c21 --- /dev/null +++ b/test/parallel/test-tls-async-cb-after-socket-end-securepair.js @@ -0,0 +1,80 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); +const SSL_OP_NO_TICKET = require('crypto').constants.SSL_OP_NO_TICKET; +const assert = require('assert'); +const net = require('net'); +const tls = require('tls'); + +const options = { + secureOptions: SSL_OP_NO_TICKET, + key: fixtures.readSync('test_key.pem'), + cert: fixtures.readSync('test_cert.pem') +}; + +const server = net.createServer(function(socket) { + const sslcontext = tls.createSecureContext(options); + sslcontext.context.setCiphers('RC4-SHA:AES128-SHA:AES256-SHA'); + + const pair = tls.createSecurePair(sslcontext, true, false, false, { server }); + + assert.ok(pair.encrypted.writable); + assert.ok(pair.cleartext.writable); + + pair.encrypted.pipe(socket); + socket.pipe(pair.encrypted); + + pair.on('error', () => {}); // Expected, client s1 closes connection. +}); + +let sessionCb = null; +let client = null; + +server.on('newSession', common.mustCall(function(key, session, done) { + done(); +})); + +server.on('resumeSession', common.mustCall(function(id, cb) { + sessionCb = cb; + + next(); +})); + +server.listen(0, function() { + const clientOpts = { + port: this.address().port, + rejectUnauthorized: false, + session: false + }; + + const s1 = tls.connect(clientOpts, function() { + clientOpts.session = s1.getSession(); + console.log('1st secure'); + + s1.destroy(); + const 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); +}