From 1c29da8236f0017fa3a09eaeba3c48309d08375b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 11 Jan 2018 03:33:49 +0800 Subject: [PATCH] tls: migrate C++ errors to internal/errors.js * Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a server-side socket instead of returning silently * Assert on wrap_->started and wrap_->ssl instead of throwing errors since these errors indicate that the user either uses private APIs, or monkey-patches internals, or hits a bug. PR-URL: https://github.com/nodejs/node/pull/18125 Reviewed-By: Anna Henningsen Reviewed-By: Fedor Indutny Reviewed-By: James M Snell --- doc/api/errors.md | 6 ++++++ lib/_tls_wrap.js | 5 +++++ lib/internal/errors.js | 2 ++ src/tls_wrap.cc | 25 ++++++---------------- test/parallel/test-tls-error-servername.js | 20 +++++++++++++++-- 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 3f2737f4df6e74..32281dfe2d8a1f 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1530,6 +1530,12 @@ a hostname in the first parameter. An excessive amount of TLS renegotiations is detected, which is a potential vector for denial-of-service attacks. + +### ERR_TLS_SNI_FROM_SERVER + +An attempt was made to issue Server Name Indication from a TLS server-side +socket, which is only valid from a client. + ### ERR_TLS_RENEGOTIATION_DISABLED diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 67dd559c87d3f1..16d8b24c2979d4 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -660,6 +660,11 @@ TLSSocket.prototype.setServername = function(name) { if (typeof name !== 'string') { throw new errors.Error('ERR_INVALID_ARG_TYPE', 'name', 'string'); } + + if (this._tlsOptions.isServer) { + throw new errors.Error('ERR_TLS_SNI_FROM_SERVER'); + } + this._handle.setServername(name); }; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a057e628d90fcd..8bdf30a9e7f0fa 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -480,6 +480,8 @@ E('ERR_TLS_RENEGOTIATION_FAILED', 'Failed to renegotiate'); E('ERR_TLS_REQUIRED_SERVER_NAME', '"servername" is required parameter for Server.addContext'); E('ERR_TLS_SESSION_ATTACK', 'TLS session renegotiation attack detected'); +E('ERR_TLS_SNI_FROM_SERVER', + 'Cannot issue SNI from a TLS server-side socket'); E('ERR_TRANSFORM_ALREADY_TRANSFORMING', 'Calling transform done when still transforming'); E('ERR_TRANSFORM_WITH_LENGTH_0', diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index e505032345ed47..18b3cf01f406f0 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -225,13 +225,11 @@ void TLSWrap::Receive(const FunctionCallbackInfo& args) { void TLSWrap::Start(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - if (wrap->started_) - return env->ThrowError("Already started."); + CHECK(!wrap->started_); + wrap->started_ = true; // Send ClientHello handshake @@ -747,17 +745,13 @@ int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) { void TLSWrap::SetVerifyMode(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK_EQ(args.Length(), 2); CHECK(args[0]->IsBoolean()); CHECK(args[1]->IsBoolean()); - - if (wrap->ssl_ == nullptr) - return env->ThrowTypeError("SetVerifyMode after destroySSL"); + CHECK_NE(wrap->ssl_, nullptr); int verify_mode; if (wrap->is_server()) { @@ -785,10 +779,7 @@ void TLSWrap::EnableSessionCallbacks( const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - if (wrap->ssl_ == nullptr) { - return wrap->env()->ThrowTypeError( - "EnableSessionCallbacks after destroySSL"); - } + CHECK_NE(wrap->ssl_, nullptr); wrap->enable_session_callbacks(); crypto::NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength); wrap->hello_parser_.Start(SSLWrap::OnClientHello, @@ -852,12 +843,8 @@ void TLSWrap::SetServername(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 1); CHECK(args[0]->IsString()); - - if (wrap->started_) - return env->ThrowError("Already started."); - - if (!wrap->is_client()) - return; + CHECK(!wrap->started_); + CHECK(wrap->is_client()); CHECK_NE(wrap->ssl_, nullptr); diff --git a/test/parallel/test-tls-error-servername.js b/test/parallel/test-tls-error-servername.js index 5c1960fce3cd31..8f6d1c5c6d8a46 100644 --- a/test/parallel/test-tls-error-servername.js +++ b/test/parallel/test-tls-error-servername.js @@ -8,10 +8,12 @@ const fixtures = require('../common/fixtures'); if (!common.hasCrypto) common.skip('missing crypto'); -const { connect } = require('tls'); +const { connect, TLSSocket } = require('tls'); const makeDuplexPair = require('../common/duplexpair'); -const { clientSide } = makeDuplexPair(); +const { clientSide, serverSide } = makeDuplexPair(); +const key = fixtures.readKey('agent1-key.pem'); +const cert = fixtures.readKey('agent1-cert.pem'); const ca = fixtures.readKey('ca1-cert.pem'); const client = connect({ @@ -28,3 +30,17 @@ const client = connect({ message: 'The "name" argument must be of type string' }); }); + +const server = new TLSSocket(serverSide, { + isServer: true, + key, + cert, + ca +}); + +common.expectsError(() => { + server.setServername('localhost'); +}, { + code: 'ERR_TLS_SNI_FROM_SERVER', + message: 'Cannot issue SNI from a TLS server-side socket' +});