tls: fix default ciphers not used consistently

[misterdjules]

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.

[misterdjules]

/cc @joyent/node-coreteam

@jasnell This overlaps with setting default.ciphers in tls.connect, and it breaks the current behavior in v0.10 where using --enable-legacy-cipher-list=v0.10.38 makes tls.connect use an empty ciphers list. I'm not sure yet how to make both work together.

[cjihrig]

There is an extra space between } and else.

[misterdjules]

@cjihrig Good catch, thank you! Updated :)

[jasnell]

Ok, after looking this over, I would recommend changing it slightly then documenting the change in behavior in the API docs. Specifically:

  var tls = require('tls'); // this is new.. see below
  if (options.ciphers) {
    c.context.setCiphers(options.ciphers);
  } else (!tls.usingV1038Ciphers() || options.ciphers !== undefined) {
    c.context.setCiphers(binding.DEFAULT_CIPHER_LIST);
  }

Basically: options.ciphers is falsy but defined, then the default cipher list is used, otherwise, do not set it. If options.ciphers is undefined because it was not explicitly passed, then fall back to the original default behavior of the default cipher list not being used. I am adding a method (usingV1038Ciphers) to tls.js that will return true if the v0.10.39 legacy cipher list switch is turned on; I can export that from tls.js and we can add the check above so that the undefined check only applies when that specific switch is enabled.

[misterdjules]

Wouldn't

  var tls = require('tls'); // this is new.. see below
  if (options.ciphers) {
    c.context.setCiphers(options.ciphers);
  } else (!tls.usingV1038Ciphers() || options.ciphers !== undefined) {
    c.context.setCiphers(binding.DEFAULT_CIPHER_LIST);
  }

make node --enable-legacy-ciphers=v0.10.38 use a non-empty ciphers list if ciphers: null is passed, and wouldn't that be a breaking change?

[misterdjules]

@jasnell Added a second commit to this PR: misterdjules@6de65e2 to make sure that the tests suite in test/external/ssl-options accounts for the removal of RC4 from the default ciphers list and for the fix that makes clients use the default ciphers list when none is specified.

I think it would be useful to add --enable-legacy-cipher-list=v0.10.38 to the set of command line options used by this test and make sure the tests suite passes.

[jasnell]

Finally able to get back to this.. and yes, when using --enable-legacy-cipher-list=v0.10.38 and passing ciphers: null, a non-empty cipher list would be used. It allows us to differentiate the case between (a) Use the previous v0.10.38 behavior (ciphers: undefined) versus (b) Tell it explicitly to use the defaults (ciphers: null). And yes, it's a bit of a breaking change. Right now, passing undefined or null means no defaults are used. So the question then becomes: is this something that we actually need to fix in v0.10.39 at all really? Because the only way to fix it would be to introduce a breaking change.

Thinking about it further, perhaps we actually need to flip this around. Make it so that if ciphers is undefined, the defaults are used and if null is passed, the empty list is used?

[misterdjules]

@jasnell

So the question then becomes: is this something that we actually need to fix in v0.10.39 at all really?

My understanding is that the point of the recent changes in default ciphers list is to prevent clients to use unsafe ciphers when they didn't mean to.

In v0.12, specifying ciphers: null means that the default, considered safe, ciphers list is used and I believe the reason is that we consider in this case that a null ciphers list means "I'm not specifying anything, give me a default that is reasonable (as in, not known as insecure)".

Thus, if we think the semantics of the ciphers option is the same in v0.10, a null value should probably mean that the default ciphers are used, otherwise it defeats the purpose of doing the changes to the default ciphers list in the first place.

Because the only way to fix it would be to introduce a breaking change.

If I understand correctly, we're already introducing a breaking change that will affect more users: when no ciphers option is provided, the default ciphers list is now used, whereas before the empty ciphers list would be used. The point of adding --enable-legacy-ciphers-list and other command line options/environment variables is to provide a way to be backward compatible. Wouldn't these command line/environment variables provide an acceptable upgrade path for that other breaking change?

[jasnell]

@misterdjules Ok, so passing null (or explicitly falsy) would cause the default ciphers to be set. If the --enable-legacy-cipher-list=v0.10.38 option is set, then the v0.10.38 default ciphers would be used. If ciphers is undefined, however, should we set the defaults or use the empty string?

[misterdjules]

@jasnell I think our guiding principle here should be "What is safer for the users, while allowing them the same flexibility as in previous v0.10.x versions and providing them with a smooth upgrade path?".

From this perspective I would say that ciphers: undefined would also mean that a secure default ciphers list would be used, unless --enable-legacy-cipher-list=v0.10.38 or a similar command line option/environment variable is used. It would still allow users to specify ciphers: '' to mean an empty ciphers list.

I don't know yet how that would translate in code, and my original concern with these changes was that it would be somehow messy to implement properly, and error prone. But if we can have a reasonably clean implementation that we can test thoroughly, then I would think that's what we want to aim for.

[jasnell]

Ok. Sounds good. I'll code that up this afternoon.
On May 13, 2015 11:46 AM, "Julien Gilli" notifications@github.com wrote:

@jasnell https://github.com/jasnell I think our guiding principle here
should be "What is safer for the users, while allowing them the same
flexibility as in previous v0.10.x versions and providing them with a
smooth upgrade path?".

From this perspective I would say that ciphers: undefined would also mean
that a secure default ciphers list would be used, unless
--enable-legacy-cipher-list=v0.10.38 or a similar command line
option/environment variable is used. It would still allow users to
specify ciphers: '' to mean an empty ciphers list.

I don't know yet how that would translate in code, and my original concern
with these changes was that it would be somehow messy to implement
properly, and error prone. But if we can have a reasonably clean
implementation that we can test thoroughly, then I would think that's what
we want to aim for.

—
Reply to this email directly or view it on GitHub
#23947 (comment).

[misterdjules]

After some discussions with @jasnell, it seems that there are two different ways to look at this:

1. explicitly passing ciphers: null or ciphers: undefined is a bug, and thus this change would be a bug fix. If users rely on that bug to use an empty ciphers list, they should pass the empty string, or 'DEFAULT' (that still needs to be determined and tested).
2. The current implementations of tls.connect and https.get/request do not throw when ciphers: null or ciphers: undefined are passed, which means that it's a supported API. Thus, this change would be considered to be a breaking behavior change and we need to provide existing v0.10.x users with a smooth path to upgrade (maybe with a command line switch that keeps the old behavior).

Note that the only API documentation entry where the ciphers option is mentioned mentions that ciphers is a string, but I'm not sure it's enough to consider that passing ciphers: null or ciphers: undefined would be a bug.

@joyent/node-coreteam Thoughts?

[jasnell]

for my vote, I'd go with option #1. There is a slight chance that pre v0.10.39 apps can choke if they pass null or undefined but from everything I can see, this looks like a bug that just needs to be fixed.

@mhdawson .. thoughts?

[jasnell]

@misterdjules @mhdawson ... are we settled on this one?

[misterdjules]

Moving to milestone v0.10.40, as v0.10.39 was released today.

[misterdjules]

Closing in favor of #25603.