Skip to content

Commit

Permalink
tls: migrate C++ errors to internal/errors.js
Browse files Browse the repository at this point in the history
* 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: #18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
joyeecheung committed Jan 16, 2018
1 parent 9ffebea commit 1c29da8
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 21 deletions.
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

<a id="ERR_TLS_SNI_FROM_SERVER"></a>
### 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.

<a id="ERR_TLS_RENEGOTIATION_DISABLED"></a>
### ERR_TLS_RENEGOTIATION_DISABLED

Expand Down
5 changes: 5 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand Down
2 changes: 2 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
25 changes: 6 additions & 19 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,13 +225,11 @@ void TLSWrap::Receive(const FunctionCallbackInfo<Value>& args) {


void TLSWrap::Start(const FunctionCallbackInfo<Value>& 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
Expand Down Expand Up @@ -747,17 +745,13 @@ int TLSWrap::DoShutdown(ShutdownWrap* req_wrap) {


void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& 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()) {
Expand Down Expand Up @@ -785,10 +779,7 @@ void TLSWrap::EnableSessionCallbacks(
const FunctionCallbackInfo<Value>& 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<TLSWrap>::OnClientHello,
Expand Down Expand Up @@ -852,12 +843,8 @@ void TLSWrap::SetServername(const FunctionCallbackInfo<Value>& 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);

Expand Down
20 changes: 18 additions & 2 deletions test/parallel/test-tls-error-servername.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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'
});

0 comments on commit 1c29da8

Please sign in to comment.