From d3ebad2d6d6e16387f3775abd07252dbb6844d1b Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Sun, 6 May 2018 13:52:34 +0900 Subject: [PATCH] tls: add min/max protocol version options The existing secureProtocol option only allows setting the allowed protocol to a specific version, or setting it to "all supported versions". It also used obscure strings based on OpenSSL C API functions. Directly setting the min or max is easier to use and explain. Backport-PR-URL: https://github.com/nodejs/node/pull/26270 PR-URL: https://github.com/nodejs/node/pull/24405 Reviewed-By: Refael Ackermann Reviewed-By: Rod Vagg --- doc/api/errors.md | 11 ++ doc/api/tls.md | 17 ++- lib/_tls_common.js | 40 ++++-- lib/_tls_wrap.js | 4 + lib/https.js | 8 ++ lib/internal/errors.js | 4 + lib/tls.js | 4 + src/node_constants.cc | 4 + src/node_crypto.cc | 13 +- test/fixtures/tls-connect.js | 69 ++++++---- test/parallel/test-https-agent-getname.js | 4 +- test/parallel/test-tls-min-max-version.js | 149 ++++++++++++++++++++++ 12 files changed, 284 insertions(+), 43 deletions(-) create mode 100644 test/parallel/test-tls-min-max-version.js diff --git a/doc/api/errors.md b/doc/api/errors.md index e050392fc65163..d5ab7569f11a27 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1652,6 +1652,17 @@ recommended to use 2048 bits or larger for stronger security. A TLS/SSL handshake timed out. In this case, the server must also abort the connection. + +### ERR_TLS_INVALID_PROTOCOL_VERSION + +Valid TLS protocol versions are `'TLSv1'`, `'TLSv1.1'`, or `'TLSv1.2'`. + + +### ERR_TLS_PROTOCOL_VERSION_CONFLICT + +Attempting to set a TLS protocol `minVersion` or `maxVersion` conflicts with an +attempt to set the `secureProtocol` explicitly. Use one mechanism or the other. + ### ERR_TLS_RENEGOTIATE diff --git a/doc/api/tls.md b/doc/api/tls.md index e66e7ffe719506..6aeb5817531545 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -1069,6 +1069,14 @@ changes: passphrase: ]}`. The object form can only occur in an array. `object.passphrase` is optional. Encrypted keys will be decrypted with `object.passphrase` if provided, or `options.passphrase` if it is not. + * `maxVersion` {string} Optionally set the maximum TLS version to allow. One + of `TLSv1.2'`, `'TLSv1.1'`, or `'TLSv1'`. Cannot be specified along with the + `secureProtocol` option, use one or the other. **Default:** `'TLSv1.2'`. + * `minVersion` {string} Optionally set the minimum TLS version to allow. One + of `TLSv1.2'`, `'TLSv1.1'`, or `'TLSv1'`. Cannot be specified along with the + `secureProtocol` option, use one or the other. It is not recommended to use + less than TLSv1.2, but it may be required for interoperability. + **Default:** `'TLSv1'`. * `passphrase` {string} Shared passphrase used for a single private key and/or a PFX. * `pfx` {string|string[]|Buffer|Buffer[]|Object[]} PFX or PKCS12 encoded @@ -1084,9 +1092,12 @@ changes: which is not usually necessary. This should be used carefully if at all! Value is a numeric bitmask of the `SSL_OP_*` options from [OpenSSL Options][]. - * `secureProtocol` {string} SSL method to use. The possible values are listed - as [SSL_METHODS][], use the function names as strings. For example, - `'TLSv1_2_method'` to force TLS version 1.2. **Default:** `'TLS_method'`. + * `secureProtocol` {string} The TLS protocol version to use. The possible + values are listed as [SSL_METHODS][], use the function names as strings. For + example, use `'TLSv1_1_method'` to force TLS version 1.1, or `'TLS_method'` + to allow any TLS protocol version. It is not recommended to use TLS versions + less than 1.2, but it may be required for interoperability. **Default:** + none, see `minVersion`. * `sessionIdContext` {string} Opaque identifier used by servers to ensure session state is not shared between applications. Unused by clients. diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 1073f5d520e0b9..ee4e64f873c835 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -26,19 +26,35 @@ const { isArrayBufferView } = require('internal/util/types'); const tls = require('tls'); const { ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED, - ERR_INVALID_ARG_TYPE + ERR_INVALID_ARG_TYPE, + ERR_TLS_INVALID_PROTOCOL_VERSION, + ERR_TLS_PROTOCOL_VERSION_CONFLICT, } = require('internal/errors').codes; -const { SSL_OP_CIPHER_SERVER_PREFERENCE } = process.binding('constants').crypto; +const { + SSL_OP_CIPHER_SERVER_PREFERENCE, + TLS1_VERSION, + TLS1_1_VERSION, + TLS1_2_VERSION, +} = process.binding('constants').crypto; // Lazily loaded var crypto = null; -const { SecureContext: NativeSecureContext } = internalBinding('crypto'); +function toV(which, v, def) { + if (v == null) v = def; + if (v === 'TLSv1') return TLS1_VERSION; + if (v === 'TLSv1.1') return TLS1_1_VERSION; + if (v === 'TLSv1.2') return TLS1_2_VERSION; + throw new ERR_TLS_INVALID_PROTOCOL_VERSION(v, which); +} -function SecureContext(secureProtocol, secureOptions, context) { +const { SecureContext: NativeSecureContext } = internalBinding('crypto'); +function SecureContext(secureProtocol, secureOptions, context, + minVersion, maxVersion) { if (!(this instanceof SecureContext)) { - return new SecureContext(secureProtocol, secureOptions, context); + return new SecureContext(secureProtocol, secureOptions, context, + minVersion, maxVersion); } if (context) { @@ -47,10 +63,15 @@ function SecureContext(secureProtocol, secureOptions, context) { this.context = new NativeSecureContext(); if (secureProtocol) { - this.context.init(secureProtocol); - } else { - this.context.init(); + if (minVersion != null) + throw new ERR_TLS_PROTOCOL_VERSION_CONFLICT(minVersion, secureProtocol); + if (maxVersion != null) + throw new ERR_TLS_PROTOCOL_VERSION_CONFLICT(maxVersion, secureProtocol); } + + this.context.init(secureProtocol, + toV('minimum', minVersion, tls.DEFAULT_MIN_VERSION), + toV('maximum', maxVersion, tls.DEFAULT_MAX_VERSION)); } if (secureOptions) this.context.setOptions(secureOptions); @@ -76,7 +97,8 @@ exports.createSecureContext = function createSecureContext(options, context) { if (options.honorCipherOrder) secureOptions |= SSL_OP_CIPHER_SERVER_PREFERENCE; - const c = new SecureContext(options.secureProtocol, secureOptions, context); + const c = new SecureContext(options.secureProtocol, secureOptions, context, + options.minVersion, options.maxVersion); var i; var val; diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 9bfdd4062fc762..88b7bad40255f1 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -877,6 +877,8 @@ function Server(options, listener) { ciphers: this.ciphers, ecdhCurve: this.ecdhCurve, dhparam: this.dhparam, + minVersion: this.minVersion, + maxVersion: this.maxVersion, secureProtocol: this.secureProtocol, secureOptions: this.secureOptions, honorCipherOrder: this.honorCipherOrder, @@ -948,6 +950,8 @@ Server.prototype.setOptions = function(options) { if (options.clientCertEngine) this.clientCertEngine = options.clientCertEngine; if (options.ca) this.ca = options.ca; + if (options.minVersion) this.minVersion = options.minVersion; + if (options.maxVersion) this.maxVersion = options.maxVersion; if (options.secureProtocol) this.secureProtocol = options.secureProtocol; if (options.crl) this.crl = options.crl; if (options.ciphers) this.ciphers = options.ciphers; diff --git a/lib/https.js b/lib/https.js index c1053da1a13eb6..6f31315a559ef9 100644 --- a/lib/https.js +++ b/lib/https.js @@ -188,6 +188,14 @@ Agent.prototype.getName = function getName(options) { if (options.servername && options.servername !== options.host) name += options.servername; + name += ':'; + if (options.minVersion) + name += options.minVersion; + + name += ':'; + if (options.maxVersion) + name += options.maxVersion; + name += ':'; if (options.secureProtocol) name += options.secureProtocol; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 71b4a87ca0d86f..b2eb16c1f854b3 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -870,6 +870,10 @@ E('ERR_TLS_CERT_ALTNAME_INVALID', 'Hostname/IP does not match certificate\'s altnames: %s', Error); E('ERR_TLS_DH_PARAM_SIZE', 'DH parameter size %s is less than 2048', Error); E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout', Error); +E('ERR_TLS_INVALID_PROTOCOL_VERSION', + '%j is not a valid %s TLS protocol version', TypeError); +E('ERR_TLS_PROTOCOL_VERSION_CONFLICT', + 'TLS protocol version %j conflicts with secureProtocol %j', TypeError); E('ERR_TLS_RENEGOTIATE', 'Attempt to renegotiate TLS session failed', Error); E('ERR_TLS_RENEGOTIATION_DISABLED', 'TLS session renegotiation disabled for this socket', Error); diff --git a/lib/tls.js b/lib/tls.js index b8de6efc8e402d..1e8686c8efa315 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -52,6 +52,10 @@ exports.DEFAULT_CIPHERS = exports.DEFAULT_ECDH_CURVE = 'auto'; +exports.DEFAULT_MAX_VERSION = 'TLSv1.2'; + +exports.DEFAULT_MIN_VERSION = 'TLSv1'; + exports.getCiphers = internalUtil.cachedResult( () => internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true) ); diff --git a/src/node_constants.cc b/src/node_constants.cc index 753b8f8fe16602..9cd50fe4e9e87b 100644 --- a/src/node_constants.cc +++ b/src/node_constants.cc @@ -1237,6 +1237,10 @@ void DefineCryptoConstants(Local target) { NODE_DEFINE_STRING_CONSTANT(target, "defaultCipherList", per_process_opts->tls_cipher_list.c_str()); + + NODE_DEFINE_CONSTANT(target, TLS1_VERSION); + NODE_DEFINE_CONSTANT(target, TLS1_1_VERSION); + NODE_DEFINE_CONSTANT(target, TLS1_2_VERSION); #endif NODE_DEFINE_CONSTANT(target, INT_MAX); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 31dee9947e0d1e..c5db8377c442e7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -395,11 +395,15 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder()); Environment* env = sc->env(); - int min_version = 0; - int max_version = 0; + CHECK_EQ(args.Length(), 3); + CHECK(args[1]->IsInt32()); + CHECK(args[2]->IsInt32()); + + int min_version = args[1].As()->Value(); + int max_version = args[2].As()->Value(); const SSL_METHOD* method = TLS_method(); - if (args.Length() == 1 && args[0]->IsString()) { + if (args[0]->IsString()) { const node::Utf8Value sslmethod(env->isolate(), args[0]); // Note that SSLv2 and SSLv3 are disallowed but SSLv23_method and friends @@ -424,6 +428,9 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { method = TLS_server_method(); } else if (strcmp(*sslmethod, "SSLv23_client_method") == 0) { method = TLS_client_method(); + } else if (strcmp(*sslmethod, "TLS_method") == 0) { + min_version = 0; + max_version = 0; } else if (strcmp(*sslmethod, "TLSv1_method") == 0) { min_version = TLS1_VERSION; max_version = TLS1_VERSION; diff --git a/test/fixtures/tls-connect.js b/test/fixtures/tls-connect.js index 43c3e6f0fd9fa0..303061adc3223e 100644 --- a/test/fixtures/tls-connect.js +++ b/test/fixtures/tls-connect.js @@ -46,28 +46,45 @@ exports.connect = function connect(options, callback) { const client = {}; const pair = { server, client }; - tls.createServer(options.server, function(conn) { - server.conn = conn; - conn.pipe(conn); - maybeCallback() - }).listen(0, function() { - server.server = this; + try { + tls.createServer(options.server, function(conn) { + server.conn = conn; + conn.pipe(conn); + maybeCallback() + }).listen(0, function() { + server.server = this; - const optClient = util._extend({ - port: this.address().port, - }, options.client); + const optClient = util._extend({ + port: this.address().port, + }, options.client); - tls.connect(optClient) - .on('secureConnect', function() { - client.conn = this; - maybeCallback(); - }) - .on('error', function(err) { + try { + tls.connect(optClient) + .on('secureConnect', function() { + client.conn = this; + maybeCallback(); + }) + .on('error', function(err) { + client.err = err; + client.conn = this; + maybeCallback(); + }); + } catch (err) { client.err = err; - client.conn = this; - maybeCallback(); - }); - }); + // The server won't get a connection, we are done. + callback(err, pair, cleanup); + callback = null; + } + }).on('tlsClientError', function(err, sock) { + server.conn = sock; + server.err = err; + maybeCallback(); + }); + } catch (err) { + // Invalid options can throw, report the error. + pair.server.err = err; + callback(err, pair, () => {}); + } function maybeCallback() { if (!callback) @@ -76,13 +93,13 @@ exports.connect = function connect(options, callback) { const err = pair.client.err || pair.server.err; callback(err, pair, cleanup); callback = null; - - function cleanup() { - if (server.server) - server.server.close(); - if (client.conn) - client.conn.end(); - } } } + + function cleanup() { + if (server.server) + server.server.close(); + if (client.conn) + client.conn.end(); + } } diff --git a/test/parallel/test-https-agent-getname.js b/test/parallel/test-https-agent-getname.js index b68850f21d57ca..5c95da549b20b5 100644 --- a/test/parallel/test-https-agent-getname.js +++ b/test/parallel/test-https-agent-getname.js @@ -12,7 +12,7 @@ const agent = new https.Agent(); // empty options assert.strictEqual( agent.getName({}), - 'localhost:::::::::::::::::' + 'localhost:::::::::::::::::::' ); // pass all options arguments @@ -40,5 +40,5 @@ const options = { assert.strictEqual( agent.getName(options), '0.0.0.0:443:192.168.1.1:ca:cert:dynamic:ciphers:key:pfx:false:localhost:' + - 'secureProtocol:c,r,l:false:ecdhCurve:dhparam:0:sessionIdContext' + '::secureProtocol:c,r,l:false:ecdhCurve:dhparam:0:sessionIdContext' ); diff --git a/test/parallel/test-tls-min-max-version.js b/test/parallel/test-tls-min-max-version.js new file mode 100644 index 00000000000000..94468e47e23f75 --- /dev/null +++ b/test/parallel/test-tls-min-max-version.js @@ -0,0 +1,149 @@ +'use strict'; +const common = require('../common'); +const fixtures = require('../common/fixtures'); + +// Check min/max protocol versions. + +const { + assert, connect, keys, tls +} = require(fixtures.path('tls-connect')); +const DEFAULT_MIN_VERSION = tls.DEFAULT_MIN_VERSION; + +// For v11.x, the default is fixed and cannot be changed via CLI. +assert.strictEqual(DEFAULT_MIN_VERSION, 'TLSv1'); + +function test(cmin, cmax, cprot, smin, smax, sprot, expect) { + connect({ + client: { + checkServerIdentity: (servername, cert) => { }, + ca: `${keys.agent1.cert}\n${keys.agent6.ca}`, + minVersion: cmin, + maxVersion: cmax, + secureProtocol: cprot, + }, + server: { + cert: keys.agent6.cert, + key: keys.agent6.key, + minVersion: smin, + maxVersion: smax, + secureProtocol: sprot, + }, + }, common.mustCall((err, pair, cleanup) => { + if (expect && !expect.match(/^TLS/)) { + assert(err.message.match(expect)); + return cleanup(); + } + + if (expect) { + assert.ifError(pair.server.err); + assert.ifError(pair.client.err); + assert(pair.server.conn); + assert(pair.client.conn); + assert.strictEqual(pair.client.conn.getProtocol(), expect); + assert.strictEqual(pair.server.conn.getProtocol(), expect); + return cleanup(); + } + + assert(pair.server.err); + assert(pair.client.err); + return cleanup(); + })); +} + +const U = undefined; + +// Default protocol is TLSv1.2. +test(U, U, U, U, U, U, 'TLSv1.2'); + +// Insecure or invalid protocols cannot be enabled. +test(U, U, U, U, U, 'SSLv2_method', 'SSLv2 methods disabled'); +test(U, U, U, U, U, 'SSLv3_method', 'SSLv3 methods disabled'); +test(U, U, 'SSLv2_method', U, U, U, 'SSLv2 methods disabled'); +test(U, U, 'SSLv3_method', U, U, U, 'SSLv3 methods disabled'); +test(U, U, 'hokey-pokey', U, U, U, 'Unknown method'); +test(U, U, U, U, U, 'hokey-pokey', 'Unknown method'); + +// Cannot use secureProtocol and min/max versions simultaneously. +test(U, U, U, U, 'TLSv1.2', 'TLS1_2_method', 'conflicts with secureProtocol'); +test(U, U, U, 'TLSv1.2', U, 'TLS1_2_method', 'conflicts with secureProtocol'); +test(U, 'TLSv1.2', 'TLS1_2_method', U, U, U, 'conflicts with secureProtocol'); +test('TLSv1.2', U, 'TLS1_2_method', U, U, U, 'conflicts with secureProtocol'); + +// TLS_method means "any supported protocol". +test(U, U, 'TLSv1_2_method', U, U, 'TLS_method', 'TLSv1.2'); +test(U, U, 'TLSv1_1_method', U, U, 'TLS_method', 'TLSv1.1'); +test(U, U, 'TLSv1_method', U, U, 'TLS_method', 'TLSv1'); +test(U, U, 'TLS_method', U, U, 'TLSv1_2_method', 'TLSv1.2'); +test(U, U, 'TLS_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); +test(U, U, 'TLS_method', U, U, 'TLSv1_method', 'TLSv1'); + +// SSLv23 also means "any supported protocol" greater than the default +// minimum (which is configurable via command line). +test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', 'TLSv1.2'); + +if (DEFAULT_MIN_VERSION === 'TLSv1.2') { + test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', null); + test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', null); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', null); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', null); +} + +if (DEFAULT_MIN_VERSION === 'TLSv1.1') { + test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', 'TLSv1.1'); + test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', null); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', null); +} + +if (DEFAULT_MIN_VERSION === 'TLSv1') { + test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', 'TLSv1.1'); + test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', 'TLSv1'); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); + test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', 'TLSv1'); +} + +// TLSv1 thru TLSv1.2 are only supported with explicit configuration with API or +// CLI (--tls-v1.0 and --tls-v1.1). +test(U, U, 'TLSv1_2_method', U, U, 'TLSv1_2_method', 'TLSv1.2'); +test(U, U, 'TLSv1_1_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); +test(U, U, 'TLSv1_method', U, U, 'TLSv1_method', 'TLSv1'); + +// The default default. +if (DEFAULT_MIN_VERSION === 'TLSv1.2') { + test(U, U, 'TLSv1_1_method', U, U, U, null); + test(U, U, 'TLSv1_method', U, U, U, null); + test(U, U, U, U, U, 'TLSv1_1_method', null); + test(U, U, U, U, U, 'TLSv1_method', null); +} + +// The default with --tls-v1.1. +if (DEFAULT_MIN_VERSION === 'TLSv1.1') { + test(U, U, 'TLSv1_1_method', U, U, U, 'TLSv1.1'); + test(U, U, 'TLSv1_method', U, U, U, null); + test(U, U, U, U, U, 'TLSv1_1_method', 'TLSv1.1'); + test(U, U, U, U, U, 'TLSv1_method', null); +} + +// The default with --tls-v1.0. +if (DEFAULT_MIN_VERSION === 'TLSv1') { + test(U, U, 'TLSv1_1_method', U, U, U, 'TLSv1.1'); + test(U, U, 'TLSv1_method', U, U, U, 'TLSv1'); + test(U, U, U, U, U, 'TLSv1_1_method', 'TLSv1.1'); + test(U, U, U, U, U, 'TLSv1_method', 'TLSv1'); +} + +// TLS min/max are respected when set with no secureProtocol. +test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_method', 'TLSv1'); +test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_1_method', 'TLSv1.1'); +test('TLSv1', 'TLSv1.2', U, U, U, 'TLSv1_2_method', 'TLSv1.2'); + +test(U, U, 'TLSv1_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1'); +test(U, U, 'TLSv1_1_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); +test(U, U, 'TLSv1_2_method', 'TLSv1', 'TLSv1.2', U, 'TLSv1.2'); + +test('TLSv1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1.2', U, 'TLSv1', 'TLSv1.1', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1', U, 'TLSv1', 'TLSv1.1', U, 'TLSv1'); +test('TLSv1', 'TLSv1.2', U, 'TLSv1', 'TLSv1', U, 'TLSv1'); +test('TLSv1.1', 'TLSv1.1', U, 'TLSv1', 'TLSv1.2', U, 'TLSv1.1'); +test('TLSv1', 'TLSv1.2', U, 'TLSv1.1', 'TLSv1.1', U, 'TLSv1.1');