From 809b970cdfda1e25c50f292b19255f42204fa6c7 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Tue, 30 Jun 2015 15:21:44 -0700 Subject: [PATCH] cryto: don't set default ciphers if client uses 10.38 legacy cipher 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). --- lib/crypto.js | 11 ++++- lib/tls.js | 3 +- test/external/ssl-options/test.js | 75 ++++++++++++++++++++++--------- 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/lib/crypto.js b/lib/crypto.js index ee55f58b33e9..cb3ed3b46ff6 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -41,6 +41,7 @@ var constants = process.binding('constants'); var stream = require('stream'); var util = require('util'); +var tls = require('tls'); // This is here because many functions accepted binary strings without // any explicit encoding in older versions of node, and we don't want @@ -114,7 +115,6 @@ function Credentials(secureProtocol, flags, context) { exports.Credentials = Credentials; - exports.createCredentials = function(options, context) { if (!options) options = {}; @@ -136,7 +136,14 @@ exports.createCredentials = function(options, context) { if (options.ciphers) { c.context.setCiphers(options.ciphers); - } else { + } else if (!(tls.usingV1038Ciphers() && options.ciphers === undefined)) { + // Set the ciphers to the default ciphers list unless + // --enable-legacy-cipher-list=v0.10.38 was passed on the command line and + // no ciphers value was passed explicitly. In that case, we want to + // preserve the previous buggy behavior that existed in v0.10.x until + // v0.10.39, otherwise, a lot of client code might be broken. Server + // side TLS/HTTPS code always sets a default cipher list explicitly so it + // never reaches this, even for versions < v0.10.39. c.context.setCiphers(binding.DEFAULT_CIPHER_LIST); } diff --git a/lib/tls.js b/lib/tls.js index 72e625e8e6bc..ba65865f9272 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -1330,7 +1330,7 @@ function normalizeConnectArgs(listArgs) { return (cb) ? [options, cb] : [options]; } -// return true if the --enable-legacy-cipher-list command line +// Returns true if the --enable-legacy-cipher-list command line // switch, or the NODE_LEGACY_CIPHER_LIST environment variable // are set to v0.10.38 and the DEFAULT_CIPHERS equal the v0.10.38 // list. @@ -1343,6 +1343,7 @@ function usingV1038Ciphers() { } return false; } +exports.usingV1038Ciphers = usingV1038Ciphers; exports.connect = function(/* [port, host], options, cb */) { var args = normalizeConnectArgs(arguments); diff --git a/test/external/ssl-options/test.js b/test/external/ssl-options/test.js index 28274acc792c..ed99bf926611 100644 --- a/test/external/ssl-options/test.js +++ b/test/external/ssl-options/test.js @@ -162,6 +162,8 @@ function secureProtocolCompatibleWithSecureOptions(secureProtocol, secureOptions return true; } +// Returns true if the server and client setups passed as parameters +// would lead to a successful connection, false otherwise. function testSSLv2Setups(serverSetup, clientSetup) { // SSLv2 has to be explicitly specified on both sides to work if (isSsl2Protocol(serverSetup.secureProtocol) && @@ -174,17 +176,36 @@ function testSSLv2Setups(serverSetup, clientSetup) { var ssl2UsedOnBothSides = isSsl2Protocol(serverSetup.secureProtocol) && isSsl2Protocol(clientSetup.secureProtocol); - if (ssl2UsedOnBothSides && - ((serverSetup.ciphers !== RC4_MD5_CIPHER) || - (clientSetup.ciphers !== RC4_MD5_CIPHER))) { - /* - * Default ciphers are not compatible with SSLv2. Both client *and* - * server need to specify a SSLv2 compatible cipher to be able to use - * SSLv2. - */ + if (ssl2UsedOnBothSides) { + // Even when SSLv2 is specified on both sides, a SSLv2 compatible cipher + // has to be used on both sides too. + + // This is the case if for instance RC4-MD5 is passed explicitly + // as a cipher option + if (serverSetup.ciphers === RC4_MD5_CIPHER && + clientSetup.ciphers === RC4_MD5_CIPHER) + return true; + + // It is also the case if the server passes explicitly RC4-MD% + // but the client doesn't pass any cipher and passes + // --enable-legacy-cipher-list=v0.10.38 on the command line. This basically + // keeps the buggy be behavior of clients not using the default ciphers + // list when not explicitly passing any cipher, and as a result + // allowing RC4 and MD5 to be used. + if (serverSetup.ciphers === RC4_MD5_CIPHER && + clientSetup.ciphers === undefined && + clientSetup.cmdLine === '--enable-legacy-cipher-list=v0.10.38') + return true; + + // In all other cases, when using SSLv2 on both sides, + // the connection should fail. return false; } + // When the client nor the server is using SSlv2, by default the connection + // should succeed. + // Other test functions looking for other incompatibilites between SSL/TLS + // options will take care of testing them. return true; } @@ -193,6 +214,9 @@ function usesDefaultCiphers(setup) { return setup.ciphers == null; } + +// Returns true if the server and client setups passed as parameters +// would lead to a successful connection, false otherwise. function testRC4LegacyCiphers(serverSetup, clientSetup) { // To be able to use a RC4 cipher suite, either both ends specify it (like @@ -203,22 +227,31 @@ function testRC4LegacyCiphers(serverSetup, clientSetup) { // default and not RC4, so we know that we're only testing disabling/enabling // RC4. - if (serverSetup.ciphers === RC4_SHA_CIPHER) { - if (clientSetup.ciphers !== RC4_SHA_CIPHER && - (!usesDefaultCiphers(clientSetup) || - clientSetup.cmdLine !== '--enable-legacy-cipher-list=v0.10.38')) { - return false; - } - } + if (serverSetup.ciphers === RC4_SHA_CIPHER || + clientSetup.ciphers === RC4_SHA_CIPHER) { - if (clientSetup.ciphers === RC4_SHA_CIPHER) { - if (serverSetup.ciphers !== RC4_SHA_CIPHER && - (!usesDefaultCiphers(serverSetup) || - serverSetup.cmdLine !== '--enable-legacy-cipher-list=v0.10.38')) { - return false; - } + if (serverSetup.ciphers === RC4_SHA_CIPHER && + clientSetup.ciphers === RC4_SHA_CIPHER) + return true; + + if (serverSetup.ciphers === RC4_SHA_CIPHER && + usesDefaultCiphers(clientSetup) && + clientSetup.cmdLine === '--enable-legacy-cipher-list=v0.10.38') + return true; + + if (clientSetup.ciphers === RC4_SHA_CIPHER && + usesDefaultCiphers(serverSetup) && + serverSetup.cmdLine === '--enable-legacy-cipher-list=v0.10.38') + return true; + + // Otherwise, if only one end passes a RC4 cipher suite explicitly, + // connection should fail. + return false; } + // When not using explicitly a RC4 cipher suite, connection should succeed. + // Other test functions looking for other incompatibilites between SSL/TLS + // options will take care of testing them. return true; }