Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

tls.Server() ignores user-supplied cipher list #2066

Closed
markcohen opened this issue Nov 10, 2011 · 5 comments
Closed

tls.Server() ignores user-supplied cipher list #2066

markcohen opened this issue Nov 10, 2011 · 5 comments
Assignees
Labels

Comments

@markcohen
Copy link

v0.6.0 - Windows - x86 - VS2010

[v0.6.0 tls.js line 859] sets the ssl_ctx cipher list to RC4-SHA:AES128-SHA:AES256-SHA

This has at least 2 side effects:

  1. It overrides any user-supplied cipher list in the high-level createServer() functions
  2. It causes HTTPS/TLS servers initiated with ECC or DH keys to silently barf on the SSL handshake, leaving users with the no-fun job of debugging the SSL protocol :)
@bnoordhuis
Copy link
Member

Thanks for the report, Mark. Quick sanity check, can you check if this patch works for you?

diff --git a/lib/tls.js b/lib/tls.js
index 21bb2af..8f83ecf 100644
--- a/lib/tls.js
+++ b/lib/tls.js
@@ -849,15 +849,13 @@ function Server(/* [options], listener */) {
     passphrase: self.passphrase,
     cert: self.cert,
     ca: self.ca,
-    ciphers: self.ciphers,
+    ciphers: self.ciphers || 'RC4-SHA:AES128-SHA:AES256-SHA',
     secureProtocol: self.secureProtocol,
     secureOptions: self.secureOptions,
     crl: self.crl,
     sessionIdContext: self.sessionIdContext
   });

-  sharedCreds.context.setCiphers('RC4-SHA:AES128-SHA:AES256-SHA');
-
   // constructor call
   net.Server.call(this, function(socket) {
     var creds = crypto.createCredentials(null, sharedCreds.context);
@@ -1017,7 +1015,6 @@ exports.connect = function(port /* host, options, cb */) {
   var socket = new net.Stream();

   var sslcontext = crypto.createCredentials(options);
-  //sslcontext.context.setCiphers('RC4-SHA:AES128-SHA:AES256-SHA');

   convertNPNProtocols(options.NPNProtocols, this);
   var pair = new SecurePair(sslcontext, false, true, false,

@markcohen
Copy link
Author

Yep, works great. Thanks.

@ghost ghost assigned bnoordhuis Nov 13, 2011
@bnoordhuis
Copy link
Member

Thanks for testing. Merged in 5e3b009.

@bminer
Copy link

bminer commented Feb 9, 2012

There is still no documentation on the 'ciphers' option. :) Could someone please add this?

@bnoordhuis
Copy link
Member

Documentation added in 38eec57.

wiedi pushed a commit to wiedi/node that referenced this issue Aug 15, 2015
Notable changes:

* buffer:
  - Due to changes in V8, it has been necessary to reimplement Buffer
    on top of V8's Uint8Array. While every effort has been made to
    maintain performance, users are likely to experience a different
    performance profile depending on how Buffer is used.
    (Trevor Norris) nodejs#1825.
  - Buffer can now take ArrayBuffers as a constructor argument
    (Trevor Norris) nodejs#2002.
  - When a single buffer is passed to Buffer.concat(), a new, copied
    Buffer object will be returned; previous behavior was to return
    the original Buffer object (Sakthipriyan Vairamani) nodejs#1937.
* build: PPC support has been added to core to allow compiling on
  pLinux BE and LE (AIX support coming soon) (Michael Dawson) nodejs#2124.
* dgram: If an error occurs within socket.send() and a callback has
  been provided, the error is only passed as the first argument to the
  callback and not emitted on the socket object; previous behavior was
  to do both (Matteo Collina & Chris Dickinson) nodejs#1796
* freelist: Deprecate the undocumented freelist core module
  (Sakthipriyan Vairamani) nodejs#2176.
* http:
  - Status codes now all use the official IANA names as per RFC7231,
    e.g. http.STATUS_CODES[414] now returns 'URI Too Long' rather than
    'Request-URI Too Large' (jomo) nodejs#1470.
  - Calling .getName() on an HTTP agent no longer returns a trailing
    colon, HTTPS agents will no longer return an extra colon near the
    middle of the string (Brendan Ashworth) nodejs#1617.
* node:
  - NODE_MODULE_VERSION has been bumped to 45 to reflect the break in
    ABI (Rod Vagg) nodejs#2096.
  - Introduce a new process.release object that contains a name
    property set to 'io.js' and sourceUrl, headersUrl and libUrl
    (Windows only) properties containing URLs for the relevant
    resources; this is intended to be used by node-gyp
    (Rod Vagg) nodejs#2154.
  - The version of node-gyp bundled with io.js now downloads and uses
    a tarball of header files from iojs.org rather than the full
    source for compiling native add-ons; it is hoped this is a
    temporary floating patch and the change will be upstreamed to
    node-gyp soon (Rod Vagg) nodejs#2066.
* repl: Persistent history is now enabled by default. The history file
  is located at ~/.node_repl_history, which can be overridden by the
  new environment variable NODE_REPL_HISTORY. This deprecates the
  previous NODE_REPL_HISTORY_FILE variable. Additionally, the format
  of the file has been changed to plain text to better handle file
  corruption. (Jeremiah Senkpiel) nodejs#2224.
* smalloc: The smalloc module has been removed as it is no longer
  possible to provide the API due to changes in V8
  (Ben Noordhuis) nodejs#2022.
* tls: Add server.getTicketKeys() and server.setTicketKeys() methods
  for TLS session key rotation (Fedor Indutny) nodejs#2227.
* v8: Upgraded to 4.4.63.26
  - ES6: Enabled computed property names
  - ES6: Array can now be subclassed in strict mode
  - ES6: Implement rest parameters in staging, use the
    --harmony-rest-parameters command line flag
  - ES6: Implement the spread operator in staging, use the
    --harmony-spreadcalls command line flag
  - Removed SetIndexedPropertiesToExternalArrayData and related APIs,
    forcing a shift to Buffer to be reimplemented based on Uint8Array
  - Introduction of Maybe and MaybeLocal C++ API for objects which may
    or may not have a value.
  - Added support for PPC

PR-URL: nodejs/node#2299
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants