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: #18125 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Jan 16, 2018 · 1c29da8 · 1c29da8
1 parent 9ffebea
commit 1c29da8
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 21 deletions.
diff --git 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.
 
+<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
 

diff --git 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
@@ -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
@@ -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
@@ -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()) {
@@ -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,
@@ -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);
 

diff --git 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'
+});