Deprecate RC4 ciphers from the default ciphers list #25603

misterdjules · 2015-06-30T22:43:18Z

This PR consolidates previous PRs from @jasnell (#14413, #14572) and myself (#23947) and allows us to have one single place where we can take a look at the current status of deprecating RC4 from the default ciphers list used by node v0.10.x, discuss it, and make changes.

The list of commits hasn't been squashed to preserve the history behind these changes. If this PR lands, its commits will be squashed. Before that, we want to make sure we understand all the implications of these changes.

jasnell · 2015-06-30T22:44:55Z

@misterdjules ... awesome, will review in detail this evening.

misterdjules · 2015-06-30T22:46:30Z

lib/crypto.js

-  if (options.ciphers) c.context.setCiphers(options.ciphers);
+  if (options.ciphers) {
+    c.context.setCiphers(options.ciphers);
+  } else if (!(tls.usingV1038Ciphers() && options.ciphers === undefined)) {


This looks like a slippery slope, but that's the current state of the discussion. Basically, I'm concerned that having to maintain this type of changes will be a major headache in the future. At the same time, the current consensus was that this would prevent a lot of TLS client code from breaking, since it currently doesn't set a default cipher suite if not passed explicitly.

@jasnell Actually, this is specific to v0.10.x versions, so we won't have to maintain it in the v0.12.x branch and later. If we're on the same page regarding this then I'm fine with it.

misterdjules · 2015-06-30T23:05:21Z

/cc @joyent/node-collaborators @joyent/node-tsc

misterdjules · 2015-06-30T23:08:15Z

Worth noting:

test/external/ssl-options/test.js passes (it was updated in this PR, so one thing we want to make sure is that the changes to these tests make sense and do not hide potential regressions).
Local make test passes on OSX with no regression.
Tests on all other supported platforms are running on Jenkins for UNIX and Windows.

misterdjules · 2015-06-30T23:15:46Z

@jasnell Thank you very much! We're getting there 👍

misterdjules · 2015-06-30T23:35:33Z

Confirmed no regression on UNIX or Windows.

jasnell · 2015-07-09T23:30:30Z

btw, given that we're now on v0.10.40, the references to V1038 should be updated.

Disable RC4 in the default cipher list Add the `--cipher-list` command line switch and `NODE_CIPHER_LIST` environment variable to completely override the default cipher list. Add the `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST` environment variable to selectively enable the default cipher list from previous node.js releases. Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs#14413

This commit squashes down several previous commits on this change to clean up before landing. There are several changes to the new cipher list command line switches: 1. It includes the type checking fix on getLegacyCiphers 2. It has the command line switches override the environment variables 3. It documents it order of precedence 4. Expands the test case to test for order of precedence

test that the type of error thrown is correct

Fix a minor typo in the tls.markdown and add additional precedence tests in test-tls-cipher-list

Per the feedback from Julien, refactor the test-tls-cipher-list test to avoid using the confusing switch statement. Add tests to ensure that using `--enable-legacy-cipher-list=v0.10.38` or `NODE_LEGACY_CIPHER_LIST=v0.10.38` disables the use of the default cipher list but setting the `--cipher-list` equal to the v0.10.38 list explicitly does not. Minor refactor to tls.js to ensure the above behavior.

Per the latest round of feedback from julien. Fix a couple of typos, add some explanatory text, refactor the test case.

Per feedback from Julien.

Refactor the default cipher list test to separate concerns a bit, make the test clearer based on Julien's feedback. Also, fix the 80-char wrap issue in the related node_crypto.cc DefaultCiphers method.

Passing null or undefined for the ciphers value of the options parameter of tls.connect and https.get/request makes node *not* use the default ciphers list. This problem had been fixed in node v0.12 with commit 5d2aef1, but for some reason the fix hasn't been backported to v0.10. This change also comes with a test that makes sure that tls/https clients that don't specify a ciphers suite (or a null or undefined one) cannot connect to a server that specifies only RC4-MD5 as the available ciphers suite. This test relies on the fact that RC4-MD5 is not available in the default ciphers suite, which is the case currently in the v0.10 branch.

Backport 408bffe from v0.12. Now that the default ciphers list is used client side even when options.ciphers is not set or set to undefined/null, and that the default ciphers list does not contain RC4 anymore, update the ssl/tls options matrix tests suite to check that a connection that uses RC4 needs both sides of the connection specifying RC4 in their allowed ciphers. Original commit message: test: fix ssl/tls options matrix test The tests suite available in test/external/ssl-options was originally written for security fixes made in the v0.10 branch. In this branch, the client's default ciphers list is compatible with SSLv2. After merging this change from v0.10 to v0.12, this tests suite was broken because commits 5d2aef1 and f4c8020 make SSL/TLS clients use a default ciphers list that is not compatible with the SSLv2 protocol. This change fixes two issues: 1) The cipher list that was setup for a given test was not passed properly to the client. 2) When either or both of clients/servers were using SSLv2, tests were expected to succeed when at least the server end was using SSLv2 compatible ciphers. Now, tests are expected to succeed only if SSLv2 compatible ciphers are used on both ends. Fixes nodejs#9020.

Update the ssl options matrix tests according to recent changes that deprecate RC4 in the default ciphers list. This change makes sure that to be able to use RC4, RC4 ciphers need to be explicitly specified by both client and server in the options, or --enable-legacy-cipher-list=v0.1038 needs to be passed by the side that doesn't explicitly pass the RC4 cipher. It also tests that the RC4-MD5 cipher suite has to be specified explicitly by both client and server in order to be used, and that it's not allowed by either side if passing undefined or null as the "ciphers" option when using tls.connect or tls.Server.

This change makes crypto.createCredentials not set default ciphers if options.ciphers is undefined and --enable-legacy-cipher-list=v0.10.38 is passed on the command line. It doesn't do anything for tls.Server, since tls.Server instances always set the default ciphers list if none is passed explicitly. For tls.connect, it means that if no ciphers is explicitly passed and --enable-legacy-cipher-list=v0.10.38 is passed on the command line, no default cipher list will be set. This is used to preserve the buggy behavior of node <= v0.10.38 and not break existing applications. With this change, tls.createSecurePair also doesn't set any default cipher if --enable-legacy-cipher-list=v0.10.38 is passed on the command line, as well as tls.Server.addContext if no credentials argument is passed. The change also updates test/external/ssl-options/test.js by adding appropriate tests (see testRC4LegacyCiphers and testSSLv2Setups).

misterdjules · 2015-07-16T22:23:26Z

lib/tls.js

@@ -1335,6 +1338,16 @@ exports.connect = function(/* [port, host], options, cb */) {
  var defaults = {
    rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED
  };
+  if (!process._usingV1040Ciphers()) {


@jasnell I don't think that this change is needed given that we have https://github.com/joyent/node/pull/25603/files#diff-437b078f0e3aa5b13790315b3813fd4fR138 too. Or am I missing something?

That would have the nice side effect of not needing to expose _usingV1040Ciphers publicly.

misterdjules · 2015-07-16T22:33:28Z

@jasnell Added another round of comments, and pushed a few cleanup commits: misterdjules@9783a82, misterdjules@bd74d90 and misterdjules@ca77bae.

/cc @joyent/node-collaborators

jasnell · 2015-08-03T19:53:45Z

@misterdjules ... sorry for the delay on reviewing. This LGTM.

jasnell · 2015-08-17T19:29:34Z

@misterdjules ... @mhdawson and I were discussing this change further. At this point, given the issues, we may want to just back off on the legacy cipher list switch and omit that entirely as it seems to be the most contentious piece. While having the legacy switch there helped us out with out particular situation, it's not critical and does increase the potential maintenance cost. So, with that in mind, let's split this into separate PRs: one that changes the default cipher list to remove RC4 in v0.10 and one that adds only the --cipher-list switch for overriding the default. I will also open a new PR against nodejs/node master that adds the --cipher-list switch to master.

thefourtheye · 2015-08-18T05:16:54Z

src/node.cc

@@ -2566,6 +2566,8 @@ static void PrintHelp() {
         "  --max-stack-size=val set max v8 stack size (bytes)\n"
         "  --enable-ssl2        enable ssl2\n"
         "  --enable-ssl3        enable ssl3\n"
+         "  --cipher-list=val    specify the default TLS cipher list\n"
+         "  --enable-legacy-cipher-list=v0.10.40 \n"


Extra space before \n

jasnell · 2015-08-18T05:23:28Z

@thefourtheye... This entire PR is likely to be refactored significantly.

jasnell · 2015-08-18T05:24:37Z

Specifically, the legacy cipher list switch will probably be dropped entirely.

thefourtheye · 2015-08-18T05:29:04Z

@jasnell Ah, good that you told me :-)

jasnell · 2015-08-24T16:47:44Z

@misterdjules ... the --tls-cipher-list switch has landed in nodejs/node master. Let's close this PR here and open a new one that just does the RC4 deprecation. We can backport the --tls-cipher-list change separately. We can drop the legacy switch entirely at this point. Going to go ahead and close but reopen if you feel it's necessary

Nodejs-Jenkins added pr v0.10 labels Jun 30, 2015

misterdjules added in progress and removed pr v0.10 labels Jun 30, 2015

Nodejs-Jenkins added pr v0.10 labels Jun 30, 2015

misterdjules added tls crypto labels Jun 30, 2015

misterdjules added this to the 0.10.40 milestone Jun 30, 2015

misterdjules added the P-1 label Jun 30, 2015

This was referenced Jun 30, 2015

tls: fix default ciphers not used consistently #23947

Closed

tls: throw an error on getLegacyCipher #14572

Closed

misterdjules reviewed Jun 30, 2015
View reviewed changes

misterdjules modified the milestones: 0.10.40, 0.10.41 Jul 10, 2015

jasnell and others added 11 commits July 16, 2015 14:23

test: test-tls-cipher-list test error type

ead952d

test that the type of error thrown is correct

test: fix typo in tls.markdown doc and additional tests

8687086

Fix a minor typo in the tls.markdown and add additional precedence tests in test-tls-cipher-list

test: additional refinements in the test

dfead0d

Per the latest round of feedback from julien. Fix a couple of typos, add some explanatory text, refactor the test case.

test: additional round of edits on the test-tls-cipher-list.js

d852445

Per feedback from Julien.

test: refactor default cipher list test

014912c

Refactor the default cipher list test to separate concerns a bit, make the test clearer based on Julien's feedback. Also, fix the 80-char wrap issue in the related node_crypto.cc DefaultCiphers method.

misterdjules force-pushed the RC4-not-in-default-ciphers-0-10 branch from d81412e to 4c46465 Compare July 16, 2015 21:23

Julien Gilli added 2 commits July 16, 2015 14:46

tls,crypto: small refactoring for legacy ciphers

9783a82

tls,crypto: update default cipher list version numbers

bd74d90

misterdjules force-pushed the RC4-not-in-default-ciphers-0-10 branch from a40c58d to bd74d90 Compare July 16, 2015 21:46

misterdjules reviewed Jul 16, 2015
View reviewed changes

src: fix warnings about cipher lists

ca77bae

thefourtheye reviewed Aug 18, 2015
View reviewed changes

jasnell closed this Aug 24, 2015

orangemocha removed the in progress label Aug 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate RC4 ciphers from the default ciphers list #25603

Deprecate RC4 ciphers from the default ciphers list #25603

misterdjules commented Jun 30, 2015

jasnell commented Jun 30, 2015

misterdjules Jun 30, 2015

misterdjules Jul 16, 2015

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

jasnell commented Jul 9, 2015

misterdjules Jul 16, 2015

misterdjules commented Jul 16, 2015

jasnell commented Aug 3, 2015

jasnell commented Aug 17, 2015

thefourtheye Aug 18, 2015

jasnell commented Aug 18, 2015

jasnell commented Aug 18, 2015

thefourtheye commented Aug 18, 2015

jasnell commented Aug 24, 2015

Deprecate RC4 ciphers from the default ciphers list #25603

Deprecate RC4 ciphers from the default ciphers list #25603

Conversation

misterdjules commented Jun 30, 2015

jasnell commented Jun 30, 2015

misterdjules Jun 30, 2015

Choose a reason for hiding this comment

misterdjules Jul 16, 2015

Choose a reason for hiding this comment

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

misterdjules commented Jun 30, 2015

jasnell commented Jul 9, 2015

misterdjules Jul 16, 2015

Choose a reason for hiding this comment

misterdjules commented Jul 16, 2015

jasnell commented Aug 3, 2015

jasnell commented Aug 17, 2015

thefourtheye Aug 18, 2015

Choose a reason for hiding this comment

jasnell commented Aug 18, 2015

jasnell commented Aug 18, 2015

thefourtheye commented Aug 18, 2015

jasnell commented Aug 24, 2015