From f240da54479c606020dac6a6f4b50b24b19d033a Mon Sep 17 00:00:00 2001 From: Mariusz 'koder' Chwalba Date: Tue, 27 Sep 2016 08:20:10 +0200 Subject: [PATCH 1/2] TLS/SSL: TLSSocket emits 'error' on handshake failure Fixes https://github.com/nodejs/node/issues/8803 --- lib/_tls_wrap.js | 8 +++- .../test-tls-failed-handshake-emits-error.js | 38 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-tls-failed-handshake-emits-error.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 6d07272c7c10a7..7e0d3459ee696a 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -426,7 +426,13 @@ TLSSocket.prototype._init = function(socket, wrap) { // Destroy socket if error happened before handshake's finish if (!self._secureEstablished) { - self.destroy(self._tlsError(err)); + if (!self._controlReleased) { + // When handshake fails control is not yet released, + // so self._tlsError will return null instead of actual error + self.destroy(err); + } else { + self.destroy(self._tlsError(err)); + } } else if (options.isServer && rejectUnauthorized && /peer did not return a certificate/.test(err.message)) { diff --git a/test/parallel/test-tls-failed-handshake-emits-error.js b/test/parallel/test-tls-failed-handshake-emits-error.js new file mode 100644 index 00000000000000..4144c7f5c79c49 --- /dev/null +++ b/test/parallel/test-tls-failed-handshake-emits-error.js @@ -0,0 +1,38 @@ +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} +const tls = require('tls'); +const net = require('net'); +const assert = require('assert'); + +const bonkers = Buffer.alloc(1024, 42); + +const server = net.createServer(function(c) { + setTimeout(function() { + const s = new tls.TLSSocket(c, { + isServer: true, + server: server + }); + + s.on('error', common.mustCall(function(e) { + assert.ok(e instanceof Error, + 'Instance of Error should be passed to error handler'); + assert.ok(e.message.match( + /SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/), + 'Expecting SSL unknown protocol'); + })); + + s.on('close', function() { + server.close(); + s.destroy(); + }); + }, common.platformTimeout(200)); +}).listen(0, function() { + const c = net.connect({port: this.address().port}, function() { + c.write(bonkers); + }); +}); From d3027e8a037d4d97162f9b70bf3e484aa38a3619 Mon Sep 17 00:00:00 2001 From: Mariusz 'koder' Chwalba Date: Fri, 30 Sep 2016 08:06:55 +0200 Subject: [PATCH 2/2] TLS/SSL: Removes unnecessary code branching Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and controll was not released, as it was never happening. Addedd test for tls.Server to ensure it still emits 'tlsClientError' as expected. Makes tests conform to defined linting rules. --- lib/_tls_wrap.js | 10 ++--- ...rver-failed-handshake-emits-clienterror.js | 37 +++++++++++++++++++ ...ls-socket-failed-handshake-emits-error.js} | 2 +- 3 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-tls-server-failed-handshake-emits-clienterror.js rename test/parallel/{test-tls-failed-handshake-emits-error.js => test-tls-socket-failed-handshake-emits-error.js} (92%) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 7e0d3459ee696a..f3526c70336e35 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -426,13 +426,9 @@ TLSSocket.prototype._init = function(socket, wrap) { // Destroy socket if error happened before handshake's finish if (!self._secureEstablished) { - if (!self._controlReleased) { - // When handshake fails control is not yet released, - // so self._tlsError will return null instead of actual error - self.destroy(err); - } else { - self.destroy(self._tlsError(err)); - } + // When handshake fails control is not yet released, + // so self._tlsError will return null instead of actual error + self.destroy(err); } else if (options.isServer && rejectUnauthorized && /peer did not return a certificate/.test(err.message)) { diff --git a/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js b/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js new file mode 100644 index 00000000000000..a404dc904ba7b7 --- /dev/null +++ b/test/parallel/test-tls-server-failed-handshake-emits-clienterror.js @@ -0,0 +1,37 @@ +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} +const tls = require('tls'); +const net = require('net'); +const assert = require('assert'); + +const bonkers = Buffer.alloc(1024, 42); + +let tlsClientErrorEmited = false; + +const server = tls.createServer({}) + .listen(0, function() { + const c = net.connect({ port: this.address().port }, function() { + c.write(bonkers); + }); + + }).on('tlsClientError', function(e) { + tlsClientErrorEmited = true; + assert.ok(e instanceof Error, + 'Instance of Error should be passed to error handler'); + assert.ok(e.message.match( + /SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/), + 'Expecting SSL unknown protocol'); + }); + +setTimeout(function() { + server.close(); + + assert.ok(tlsClientErrorEmited, + 'tlsClientError should be emited'); + +}, common.platformTimeout(200)); diff --git a/test/parallel/test-tls-failed-handshake-emits-error.js b/test/parallel/test-tls-socket-failed-handshake-emits-error.js similarity index 92% rename from test/parallel/test-tls-failed-handshake-emits-error.js rename to test/parallel/test-tls-socket-failed-handshake-emits-error.js index 4144c7f5c79c49..f655dc97b5a99b 100644 --- a/test/parallel/test-tls-failed-handshake-emits-error.js +++ b/test/parallel/test-tls-socket-failed-handshake-emits-error.js @@ -20,7 +20,7 @@ const server = net.createServer(function(c) { s.on('error', common.mustCall(function(e) { assert.ok(e instanceof Error, - 'Instance of Error should be passed to error handler'); + 'Instance of Error should be passed to error handler'); assert.ok(e.message.match( /SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/), 'Expecting SSL unknown protocol');