diff --git a/doc/api/tls.markdown b/doc/api/tls.markdown index 49b37106e278..4511ccbc88c4 100644 --- a/doc/api/tls.markdown +++ b/doc/api/tls.markdown @@ -164,6 +164,34 @@ Reverting back to the defaults used by older releases can weaken the security of your applications. The legacy cipher suites should only be used if absolutely necessary. +NOTE: Due to an error in Node.js v0.10.38, the default cipher list only applied +to servers using TLS. The default cipher list would _not_ be used by clients. +This behavior has been changed in v0.10.39 and the default cipher list is now +used by both the server and client when using TLS. However, when using +`--enable-legacy-cipher-list=v0.10.38`, Node.js is reverted back to the +v0.10.38 behavior of only using the default cipher list on the server. + +### Cipher List Precedence + +Note that the `--enable-legacy-cipher-list`, `NODE_LEGACY_CIPHER_LIST`, +`--cipher-list` and `NODE_CIPHER_LIST` options are mutually exclusive. + +If the `NODE_CIPHER_LIST` and `NODE_LEGACY_CIPHER_LIST` environment variables +are both specified, the `NODE_LEGACY_CIPHER_LIST` setting will take precedence. + +The `--cipher-list` and `--enable-legacy-cipher-list` command line options +will override the environment variables. If both happen to be specified, the +right-most (second one specified) will take precedence. For instance, in the +example: + + node --cipher-list=ABC --enable-legacy-cipher-list=v0.10.38 + +The v0.10.38 default cipher list will be used. + + node --enable-legacy-cipher-list=v0.10.38 --cipher-list=ABC + +The custom cipher list will be used. + ## tls.getCiphers() Returns an array with the names of the supported SSL ciphers. @@ -174,6 +202,18 @@ Example: console.log(ciphers); // ['AES128-SHA', 'AES256-SHA', ...] +## tls.getLegacyCiphers(version) + +Returns a default cipher list used in a previous version of Node.js. The +version parameter must be a string whose value identifies previous Node.js +release version. The only value currently supported is `v0.10.38`. + +A TypeError will be thrown if: (a) the `version` is any type other than a +string, (b) the `version` parameter is not specified, or (c) additional +parameters are passed in. An Error will be thrown if the `version` parameter is +passed in as a string but the value does not correlate to any known Node.js +release for which a default cipher list is available. + ## tls.createServer(options, [secureConnectionListener]) Creates a new [tls.Server][]. The `connectionListener` argument is diff --git a/lib/tls.js b/lib/tls.js index 9f53ad82aef8..72e625e8e6bc 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -1330,6 +1330,20 @@ function normalizeConnectArgs(listArgs) { return (cb) ? [options, cb] : [options]; } +// return 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. +function usingV1038Ciphers() { + var argv = process.execArgv; + if ((argv.indexOf('--enable-legacy-cipher-list=v0.10.38') > -1 || + process.env['NODE_LEGACY_CIPHER_LIST'] === 'v0.10.38') && + DEFAULT_CIPHERS === _crypto.getLegacyCiphers('v0.10.38')) { + return true; + } + return false; +} + exports.connect = function(/* [port, host], options, cb */) { var args = normalizeConnectArgs(arguments); var options = args[0]; @@ -1338,7 +1352,14 @@ exports.connect = function(/* [port, host], options, cb */) { var defaults = { rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED }; - if (DEFAULT_CIPHERS != _crypto.getLegacyCiphers('v0.10.38')) { + if (!usingV1038Ciphers()) { + // only set the default ciphers if we are _not_ using the + // v0.10.38 legacy cipher list. Node v0.10.38 had a bug + // that failed to set the default ciphers on the default + // options. This has been fixed in v0.10.39 and above. + // However, when the user explicitly tells node to revert + // back to using the v0.10.38 cipher list, node should + // revert back to the original v0.10.38 behavior. defaults.ciphers = DEFAULT_CIPHERS; } options = util._extend(defaults, options || {}); diff --git a/src/node.cc b/src/node.cc index 81e123e571f0..4b3aa6117300 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2588,7 +2588,8 @@ static void PrintHelp() { // Parse node command line arguments. static void ParseArgs(int argc, char **argv) { int i; - bool using_legacy_cipher_list = false; + int cipher_list_options = 0; + bool using_cipher_list_option = false; // TODO use parse opts for (i = 1; i < argc; i++) { @@ -2658,14 +2659,11 @@ static void ParseArgs(int argc, char **argv) { argv[i] = const_cast(""); throw_deprecation = true; } else if (strncmp(arg, "--cipher-list=", 14) == 0) { - if (!using_legacy_cipher_list) { - DEFAULT_CIPHER_LIST = arg + 14; - } + DEFAULT_CIPHER_LIST = arg + 14; argv[i] = const_cast(""); } else if (strncmp(arg, "--enable-legacy-cipher-list=", 28) == 0) { const char * legacy_list = legacy_cipher_list(arg+28); if (legacy_list != NULL) { - using_legacy_cipher_list = true; DEFAULT_CIPHER_LIST = legacy_list; } else { fprintf(stderr, "Error: An unknown legacy cipher list was specified\n"); @@ -2948,6 +2946,26 @@ char** Init(int argc, char *argv[]) { // Make inherited handles noninheritable. uv_disable_stdio_inheritance(); + // set the cipher list from the environment variable first, + // the command line switch will override if specified. + const char * cipher_list = getenv("NODE_CIPHER_LIST"); + if (cipher_list != NULL) { + DEFAULT_CIPHER_LIST = cipher_list; + } + + // Setting NODE_LEGACY_CIPHER_LIST will override the NODE_CIPHER_LIST + const char * leg_cipher_id = getenv("NODE_LEGACY_CIPHER_LIST"); + if (leg_cipher_id != NULL) { + const char * leg_cipher_list = + legacy_cipher_list(leg_cipher_id); + if (leg_cipher_list != NULL) { + DEFAULT_CIPHER_LIST = leg_cipher_list; + } else { + fprintf(stderr, "Error: An unknown legacy cipher list was specified\n"); + exit(9); + } + } + // Parse a few arguments which are specific to Node. node::ParseArgs(argc, argv); // Parse the rest of the args (up to the 'option_end_index' (where '--' was @@ -2966,25 +2984,6 @@ char** Init(int argc, char *argv[]) { v8argv[option_end_index + 1] = const_cast("v8debug"); } - const char * cipher_list = getenv("NODE_CIPHER_LIST"); - if (cipher_list != NULL) { - DEFAULT_CIPHER_LIST = cipher_list; - } - // Allow the NODE_LEGACY_CIPHER_LIST envar to override the other - // cipher list options. NODE_LEGACY_CIPHER_LIST=v0.10.38 will use - // the cipher list from v0.10.38 - const char * leg_cipher_id = getenv("NODE_LEGACY_CIPHER_LIST"); - if (leg_cipher_id != NULL) { - const char * leg_cipher_list = - legacy_cipher_list(leg_cipher_id); - if (leg_cipher_list != NULL) { - DEFAULT_CIPHER_LIST = leg_cipher_list; - } else { - fprintf(stderr, "Error: An unknown legacy cipher list was specified\n"); - exit(9); - } - } - // For the normal stack which moves from high to low addresses when frames // are pushed, we can compute the limit as stack_size bytes below the // the address of a stack variable (e.g. &stack_var) as an approximation diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c1e943fef49f..c87bac125c58 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4199,12 +4199,20 @@ const char* ToCString(const node::Utf8Value& value) { Handle DefaultCiphers(const Arguments& args) { HandleScope scope; + unsigned int len = args.Length(); + if (len != 1 || !args[0]->IsString()) { + return ThrowException( + Exception::TypeError( + String::New("A single string parameter is required"))); + } node::Utf8Value key(args[0]); const char * list = legacy_cipher_list(ToCString(key)); - if (list == NULL) { - list = DEFAULT_CIPHER_LIST_HEAD; + if (list != NULL) { + return scope.Close(v8::String::New(list)); + } else { + return ThrowException(Exception::Error(String::New( + "Unknown legacy cipher list"))); } - return scope.Close(v8::String::New(list)); } Handle GetCiphers(const Arguments& args) { diff --git a/test/simple/test-tls-cipher-list.js b/test/simple/test-tls-cipher-list.js index ac2169537b81..e559b5be2de4 100644 --- a/test/simple/test-tls-cipher-list.js +++ b/test/simple/test-tls-cipher-list.js @@ -23,32 +23,17 @@ var spawn = require('child_process').spawn; var assert = require('assert'); var tls = require('tls'); var crypto = process.binding('crypto'); +var common = require('../common'); +var fs = require('fs'); -function doTest(checklist, env, useswitch) { - var options; - if (env && useswitch === 1) { - options = {env:env}; - } - var args = ['-e', 'console.log(process.binding(\'crypto\').DEFAULT_CIPHER_LIST)']; - - switch(useswitch) { - case 1: - // Test --cipher-test - args.unshift('--cipher-list=' + env); - break; - case 2: - // Test --enable-legacy-cipher-list - args.unshift('--enable-legacy-cipher-list=' + env); - break; - case 3: - // Test NODE_LEGACY_CIPHER_LIST - if (env) options = {env:{"NODE_LEGACY_CIPHER_LIST": env}}; - break; - default: - // Test NODE_CIPHER_LIST - if (env) options = {env:env}; - } +var V1038Ciphers = tls.getLegacyCiphers('v0.10.38'); +function doTest(checklist, additional_args, env) { + var options; + if (env) options = {env:env}; + additional_args = additional_args || []; + var args = additional_args.concat([ + '-e', 'console.log(process.binding(\'crypto\').DEFAULT_CIPHER_LIST)']); var out = ''; spawn(process.execPath, args, options). stdout. @@ -60,11 +45,229 @@ function doTest(checklist, env, useswitch) { }); } +// test that the command line switches takes precedence +// over the environment variables +function doTestPrecedence() { + // test that --cipher-list takes precedence over NODE_CIPHER_LIST + doTest('ABC', ['--cipher-list=ABC'], {'NODE_CIPHER_LIST': 'XYZ'}); + + // test that --enable-legacy-cipher-list takes precedence + // over NODE_CIPHER_LIST + doTest(V1038Ciphers, + ['--enable-legacy-cipher-list=v0.10.38'], + {'NODE_CIPHER_LIST': 'XYZ'}); + + // test that --cipher-list takes precedence over NODE_LEGACY_CIPHER_LIST + doTest('ABC', + ['--cipher-list=ABC'], + {'NODE_LEGACY_CIPHER_LIST': 'v0.10.38'}); + + // test that --enable-legacy-cipher-list takes precence over both envars + // note: in this release, there's only one legal value for the legacy + // switch so this test is largely a non-op. When multiple values + // are supported, this test should be changed to test that the + // command line switch actually does override + doTest(V1038Ciphers, + ['--enable-legacy-cipher-list=v0.10.38'], + { + 'NODE_LEGACY_CIPHER_LIST': 'v0.10.38', + 'NODE_CIPHER_LIST': 'XYZ' + }); + + // test the right-most command line option takes precedence + doTest(V1038Ciphers, + [ + '--cipher-list=XYZ', + '--enable-legacy-cipher-list=v0.10.38' + ]); + + // test the right-most command line option takes precedence + doTest('XYZ', + [ + '--enable-legacy-cipher-list=v0.10.38', + '--cipher-list=XYZ' + ]); + + // test the right-most command line option takes precedence + doTest('XYZ', + [ + '--cipher-list=ABC', + '--enable-legacy-cipher-list=v0.10.38', + '--cipher-list=XYZ' + ]); + + // test that NODE_LEGACY_CIPHER_LIST takes precedence over + // NODE_CIPHER_LIST + doTest(V1038Ciphers, [], + { + 'NODE_LEGACY_CIPHER_LIST': 'v0.10.38', + 'NODE_CIPHER_LIST': 'ABC' + }); +} + +// Start running the tests... doTest(crypto.DEFAULT_CIPHER_LIST); // test the default -doTest('ABC', {'NODE_CIPHER_LIST':'ABC'}); // test the envar -doTest('ABC', 'ABC', 1); // test the --cipher-list switch -['v0.10.38'].forEach(function(ver) { - doTest(tls.getLegacyCiphers(ver), ver, 2); - doTest(tls.getLegacyCiphers(ver), ver, 3); +// Test the NODE_CIPHER_LIST environment variable +doTest('ABC', [], {'NODE_CIPHER_LIST':'ABC'}); + +// Test the --cipher-list command line switch +doTest('ABC', ['--cipher-list=ABC']); + +// Test the --enable-legacy-cipher-list and NODE_LEGACY_CIPHER_LIST envar +['v0.10.38'].forEach(function(arg) { + var checklist = tls.getLegacyCiphers(arg); + // command line switch + doTest(checklist, ['--enable-legacy-cipher-list=' + arg]); + // environment variable + doTest(checklist, [], {'NODE_LEGACY_CIPHER_LIST': arg}); +}); + +// Test the precedence order for the various options +doTestPrecedence(); + +// Test that we throw properly +// invalid value +assert.throws(function() {tls.getLegacyCiphers('foo');}, Error); +// no parameters +assert.throws(function() {tls.getLegacyCiphers();}, TypeError); +// not a string parameter +assert.throws(function() {tls.getLegacyCiphers(1);}, TypeError); +// too many parameters +assert.throws(function() {tls.getLegacyCiphers('abc', 'extra');}, TypeError); +// ah, just right +assert.doesNotThrow(function() {tls.getLegacyCiphers('v0.10.38');}); + +// Test to ensure default ciphers are not set when v0.10.38 legacy cipher +// switch is used. This is a bit involved... we need to first set up the +// TLS server, then spawn a second node instance using the v0.10.38 cipher, +// then connect and check to make sure the options are correct. Since there +// is no direct way of testing it, an alternate createCredentials shim is +// created that intercepts the call to createCredentials and checks the +// output. The following server code was adopted from +// test-tls-connect-simple. This spins up a server to verify that the +// connection is still able to function with the default ciphers not set +// on the client side. + +// note that the following function is written out to a string and +// passed in as an argument to a child node instance. +var fail_if_default_ciphers_set = ( + function() { + var tls = require('tls'); + var orig_createCredentials = require('crypto').createCredentials; + var used_monkey_patch = false; + require('crypto').createCredentials = function(options) { + used_monkey_patch = true; + // since node was started with the --enable-legacy-cipher-list + // switch equal to v0.10.38, the options.ciphers should be + // undefined. If it's not undefined, we have a problem and + // the test fails + if (options.ciphers !== undefined) { + console.error(options.ciphers); + process.exit(1); + } + return orig_createCredentials(options); + }; + var socket = tls.connect({ + port: 0, + rejectUnauthorized: false + }, function() { + socket.end(); + if (!used_monkey_patch) { + console.error('monkey patched createCredentials not used.'); + process.exit(1); + } + }); + } +).toString(); + +// Verifies that the default cipher list is set. +// like fail_if_default_ciphers_set, this is serialized +// out to a string and passed to a new node instance +var fail_if_default_ciphers_not_set = ( + function() { + var tls = require('tls'); + var orig_createCredentials = require('crypto').createCredentials; + var used_monkey_patch = false; + require('crypto').createCredentials = function(options) { + used_monkey_patch = true; + // node is not started with --enable-legacy-cipher-list + if (!options.ciphers) { + console.error('default ciphers are not set'); + process.exit(1); + } + return orig_createCredentials(options); + }; + var socket = tls.connect({ + port: 0, + rejectUnauthorized: false + }, function() { + socket.end(); + if (!used_monkey_patch) { + console.error('monkey patched createCredentials not used.'); + process.exit(1); + } + }); + } +).toString(); + + +var test_count = 0; + +function doDefaultCipherTest(test, additional_args, env) { + var options = {}; + if (env) options.env = env; + var err = ''; + additional_args = additional_args || []; + var args = additional_args.concat([ + '-e', require('util').format('(%s)()', test). + replace('port: 0', + 'port: ' + common.PORT) + ]); + var child = spawn(process.execPath, args, options); + // if the child process writes to stderr, report it + // as a failure. This will capture the error in the + // tls connection also, which is what we want. We + // want to be able to verify that changes to the + // default cipher list being set or not will not impact + // the actual connection being made. + child.stderr. + on('data', function(data) { + err += data; + }). + on('end', function() { + if (err !== '') { + assert.fail(err.substr(0,err.length-1)); + } + }); +} + +var options = { + key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem') +}; +var server = tls.Server(options, function(socket) { + test_count++; + if (test_count === 4) server.close(); +}); +server.listen(common.PORT, function() { + // checks to make sure the default ciphers are *not* set + // because the --enable-legacy-cipher-list switch is set to + // v0.10.38 + doDefaultCipherTest(fail_if_default_ciphers_set, + ['--enable-legacy-cipher-list=v0.10.38']); + + // checks to make sure the default ciphers are *not* set + // because the NODE_LEGACY_CIPHER_LIST envar is set to v0.10.38 + doDefaultCipherTest(fail_if_default_ciphers_set, + [], {'NODE_LEGACY_CIPHER_LIST': 'v0.10.38'}); + + // this variant checks to ensure that the default cipher list IS set + doDefaultCipherTest(fail_if_default_ciphers_not_set, [], {}); + + // test that setting the cipher list explicitly to the v0.10.38 + // string without using the legacy cipher switch causes the + // default ciphers to be set. + doDefaultCipherTest(fail_if_default_ciphers_not_set, + ['--cipher-list=' + V1038Ciphers], {}); });