tls: throw error on bad ciphers option

PR-URL: #21557 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
nodejs · Jul 3, 2018 · a15ea5d · a15ea5d
1 parent c267639
commit a15ea5d
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 24 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -904,7 +904,13 @@ void SecureContext::SetCiphers(const FunctionCallbackInfo<Value>& args) {
   THROW_AND_RETURN_IF_NOT_STRING(env, args[0], "Ciphers");
 
   const node::Utf8Value ciphers(args.GetIsolate(), args[0]);
-  SSL_CTX_set_cipher_list(sc->ctx_.get(), *ciphers);
+  if (!SSL_CTX_set_cipher_list(sc->ctx_.get(), *ciphers)) {
+    unsigned long err = ERR_get_error();  // NOLINT(runtime/int)
+    if (!err) {
+      return env->ThrowError("Failed to set ciphers");
+    }
+    return ThrowCryptoError(env, err);
+  }
 }
 
 

diff --git a/test/parallel/test-tls-handshake-error.js b/test/parallel/test-tls-handshake-error.js
@@ -16,17 +16,12 @@ const server = tls.createServer({
   rejectUnauthorized: true
 }, function(c) {
 }).listen(0, common.mustCall(function() {
-  const c = tls.connect({
-    port: this.address().port,
-    ciphers: 'RC4'
-  }, common.mustNotCall());
+  assert.throws(() => {
+    tls.connect({
+      port: this.address().port,
+      ciphers: 'RC4'
+    }, common.mustNotCall());
+  }, /no cipher match/i);
 
-  c.on('error', common.mustCall(function(err) {
-    assert.notStrictEqual(err.code, 'ECONNRESET');
-  }));
-
-  c.on('close', common.mustCall(function(err) {
-    assert.ok(err);
-    server.close();
-  }));
+  server.close();
 }));
diff --git a/test/parallel/test-tls-set-ciphers-error.js b/test/parallel/test-tls-set-ciphers-error.js
@@ -0,0 +1,22 @@
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+{
+  const options = {
+    key: fixtures.readKey('agent2-key.pem'),
+    cert: fixtures.readKey('agent2-cert.pem'),
+    ciphers: 'aes256-sha'
+  };
+  assert.throws(() => tls.createServer(options, common.mustNotCall()),
+                /no cipher match/i);
+  options.ciphers = 'FOOBARBAZ';
+  assert.throws(() => tls.createServer(options, common.mustNotCall()),
+                /no cipher match/i);
+}
diff --git a/test/sequential/test-tls-connect.js b/test/sequential/test-tls-connect.js
@@ -50,15 +50,12 @@ const tls = require('tls');
   const cert = fixtures.readSync('test_cert.pem');
   const key = fixtures.readSync('test_key.pem');
 
-  const conn = tls.connect({
-    cert: cert,
-    key: key,
-    port: common.PORT,
-    ciphers: 'rick-128-roll'
-  }, common.mustNotCall());
-
-  conn.on(
-    'error',
-    common.mustCall((e) => { assert.strictEqual(e.code, 'ECONNREFUSED'); })
-  );
+  assert.throws(() => {
+    tls.connect({
+      cert: cert,
+      key: key,
+      port: common.PORT,
+      ciphers: 'rick-128-roll'
+    }, common.mustNotCall());
+  }, /no cipher match/i);
 }