From b9283cf9d17a51f9654b438216ecb743ed69a7ce Mon Sep 17 00:00:00 2001 From: Timothy J Fontaine Date: Wed, 22 Oct 2014 10:27:56 -0700 Subject: [PATCH] tls: honorCipherOrder should not degrade defaults Specifying honorCipherOrder should not change the SSLv2/SSLv3 defaults for a TLS server. Use secureOptions logic in both lib/tls.js and lib/crypto.js --- lib/crypto.js | 44 +++--- lib/tls.js | 12 +- ...test-tls-honorcipherorder-secureOptions.js | 131 ++++++++++++++++++ 3 files changed, 167 insertions(+), 20 deletions(-) create mode 100644 test/simple/test-tls-honorcipherorder-secureOptions.js diff --git a/lib/crypto.js b/lib/crypto.js index f88c55d0a2f..597d196f2f2 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -61,6 +61,31 @@ var StringDecoder = require('string_decoder').StringDecoder; var CONTEXT_DEFAULT_OPTIONS = undefined; +function getSecureOptions(secureProtocol, secureOptions) { + if (CONTEXT_DEFAULT_OPTIONS === undefined) { + CONTEXT_DEFAULT_OPTIONS = 0; + + if (!binding.SSL3_ENABLE) + CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3; + + if (!binding.SSL2_ENABLE) + CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2; + } + + if (secureOptions === undefined) { + if (secureProtocol === undefined || + secureProtocol === 'SSLv23_method' || + secureProtocol === 'SSLv23_server_method' || + secureProtocol === 'SSLv23_client_method') { + secureOptions |= CONTEXT_DEFAULT_OPTIONS; + } + } + + return secureOptions; +} +exports._getSecureOptions = getSecureOptions; + + function Credentials(secureProtocol, flags, context) { if (!(this instanceof Credentials)) { return new Credentials(secureProtocol, flags, context); @@ -82,24 +107,7 @@ function Credentials(secureProtocol, flags, context) { } } - if (CONTEXT_DEFAULT_OPTIONS === undefined) { - CONTEXT_DEFAULT_OPTIONS = 0; - - if (!binding.SSL3_ENABLE) - CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3; - - if (!binding.SSL2_ENABLE) - CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2; - } - - if (flags === undefined) { - if (secureProtocol === undefined || - secureProtocol === 'SSLv23_method' || - secureProtocol === 'SSLv23_server_method' || - secureProtocol === 'SSLv23_client_method') { - flags |= CONTEXT_DEFAULT_OPTIONS; - } - } + flags = getSecureOptions(secureProtocol, flags); this.context.setOptions(flags); } diff --git a/lib/tls.js b/lib/tls.js index 392f7ad2ba0..adc8efa634d 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -1239,11 +1239,16 @@ Server.prototype.setOptions = function(options) { if (options.secureProtocol) this.secureProtocol = options.secureProtocol; if (options.crl) this.crl = options.crl; if (options.ciphers) this.ciphers = options.ciphers; - var secureOptions = options.secureOptions || 0; + + var secureOptions = crypto._getSecureOptions(options.secureProtocol, + options.secureOptions); + if (options.honorCipherOrder) { secureOptions |= constants.SSL_OP_CIPHER_SERVER_PREFERENCE; } - if (secureOptions) this.secureOptions = secureOptions; + + this.secureOptions = secureOptions; + if (options.NPNProtocols) convertNPNProtocols(options.NPNProtocols, this); if (options.SNICallback) { this.SNICallback = options.SNICallback; @@ -1326,6 +1331,9 @@ exports.connect = function(/* [port, host], options, cb */) { }; options = util._extend(defaults, options || {}); + options.secureOptions = crypto._getSecureOptions(options.secureProtocol, + options.secureOptions); + var socket = options.socket ? options.socket : new net.Stream(); var sslcontext = crypto.createCredentials(options); diff --git a/test/simple/test-tls-honorcipherorder-secureOptions.js b/test/simple/test-tls-honorcipherorder-secureOptions.js new file mode 100644 index 00000000000..e70cfb1ef4a --- /dev/null +++ b/test/simple/test-tls-honorcipherorder-secureOptions.js @@ -0,0 +1,131 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); +var tls = require('tls'); +var fs = require('fs'); +var nconns = 0; +var SSL_Method = 'SSLv23_method'; +var localhost = '127.0.0.1'; +var opCipher = process.binding('constants').SSL_OP_CIPHER_SERVER_PREFERENCE; + +/* + * This test is to make sure we are preserving secureOptions that are passed + * to the server. + * + * Also that if honorCipherOrder is passed we are preserving that in the + * options. + * + * And that if we are passing in secureOptions no new options (aside from the + * honorCipherOrder case) are added to the secureOptions + */ + + +process.on('exit', function() { + assert.equal(nconns, 6); +}); + +function test(honorCipherOrder, clientCipher, expectedCipher, secureOptions, cb) { + var soptions = { + secureProtocol: SSL_Method, + key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem'), + ciphers: 'AES256-SHA:RC4-SHA:DES-CBC-SHA', + secureOptions: secureOptions, + honorCipherOrder: !!honorCipherOrder + }; + + var server = tls.createServer(soptions, function(cleartextStream) { + nconns++; + }); + + if (!!honorCipherOrder) { + assert.strictEqual(server.secureOptions & opCipher, opCipher, 'we should preserve cipher preference'); + } + + if (secureOptions) { + var expectedSecureOpts = secureOptions; + if (!!honorCipherOrder) expectedSecureOpts |= opCipher; + + assert.strictEqual(server.secureOptions & expectedSecureOpts, + expectedSecureOpts, 'we should preserve secureOptions'); + assert.strictEqual(server.secureOptions & ~expectedSecureOpts, + 0, + 'we should not add extra options'); + } + + server.listen(common.PORT, localhost, function() { + var coptions = { + rejectUnauthorized: false, + secureProtocol: SSL_Method + }; + if (clientCipher) { + coptions.ciphers = clientCipher; + } + var client = tls.connect(common.PORT, localhost, coptions, function() { + var cipher = client.getCipher(); + client.end(); + server.close(); + assert.equal(cipher.name, expectedCipher); + if (cb) cb(); + }); + }); +} + +test1(); + +function test1() { + // Client has the preference of cipher suites by default + test(false, 'DES-CBC-SHA:RC4-SHA:AES256-SHA','DES-CBC-SHA', 0, test2); +} + +function test2() { + // Server has the preference of cipher suites where AES256-SHA is in + // the first. + test(true, 'DES-CBC-SHA:RC4-SHA:AES256-SHA', 'AES256-SHA', 0, test3); +} + +function test3() { + // Server has the preference of cipher suites. RC4-SHA is given + // higher priority over DES-CBC-SHA among client cipher suites. + test(true, 'DES-CBC-SHA:RC4-SHA', 'RC4-SHA', 0, test4); +} + +function test4() { + // As client has only one cipher, server has no choice in regardless + // of honorCipherOrder. + test(true, 'DES-CBC-SHA', 'DES-CBC-SHA', 0, test5); +} + +function test5() { + test(false, + 'DES-CBC-SHA', + 'DES-CBC-SHA', + process.binding('constants').SSL_OP_SINGLE_DH_USE, test6); +} + +function test6() { + test(true, + 'DES-CBC-SHA', + 'DES-CBC-SHA', + process.binding('constants').SSL_OP_SINGLE_DH_USE); +}