Skip to content

Commit

Permalink
tls: add min/max protocol version options
Browse files Browse the repository at this point in the history 
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: #24676
PR-URL: #24405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
  • Loading branch information
@sam-github @BridgeAR
sam-github authored and BridgeAR committed Dec 7, 2018
1 parent bfec6a4 commit acb7351
Show file tree
Hide file tree
Showing 12 changed files with 294 additions and 43 deletions.
11 changes: 11 additions & 0 deletions doc/api/errors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1655,6 +1655,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.

<a id="ERR_TLS_INVALID_PROTOCOL_VERSION"></a>
### ERR_TLS_INVALID_PROTOCOL_VERSION

Valid TLS protocol versions are `'TLSv1'`, `'TLSv1.1'`, or `'TLSv1.2'`.

<a id="ERR_TLS_PROTOCOL_VERSION_CONFLICT"></a>
### 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.

<a id="ERR_TLS_RENEGOTIATE"></a>
### ERR_TLS_RENEGOTIATE

Expand Down
17 changes: 14 additions & 3 deletions doc/api/tls.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,14 @@ changes:
passphrase: <string>]}`. 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
Expand All @@ -1149,9 +1157,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.

Expand Down
40 changes: 31 additions & 9 deletions lib/_tls_common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,34 @@ 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 } = internalBinding('constants').crypto;
const {
SSL_OP_CIPHER_SERVER_PREFERENCE,
TLS1_VERSION,
TLS1_1_VERSION,
TLS1_2_VERSION,
} = internalBinding('constants').crypto;

// Lazily loaded from internal/crypto/util.
let toBuf = null;

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);
}

const { SecureContext: NativeSecureContext } = internalBinding('crypto');
function SecureContext(secureProtocol, secureOptions, context) {
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) {
Expand All @@ -46,10 +62,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);
Expand All @@ -75,7 +96,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;

Expand Down
14 changes: 14 additions & 0 deletions lib/_tls_wrap.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,16 @@ Server.prototype.setSecureContext = function(options) {
else
this.ca = undefined;

if (options.minVersion)
this.minVersion = options.minVersion;
else
this.minVersion = undefined;

if (options.maxVersion)
this.maxVersion = options.maxVersion;
else
this.maxVersion = undefined;

if (options.secureProtocol)
this.secureProtocol = options.secureProtocol;
else
Expand Down Expand Up @@ -969,6 +979,8 @@ Server.prototype.setSecureContext = function(options) {
ciphers: this.ciphers,
ecdhCurve: this.ecdhCurve,
dhparam: this.dhparam,
minVersion: this.minVersion,
maxVersion: this.maxVersion,
secureProtocol: this.secureProtocol,
secureOptions: this.secureOptions,
honorCipherOrder: this.honorCipherOrder,
Expand Down Expand Up @@ -1021,6 +1033,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;
Expand Down
8 changes: 8 additions & 0 deletions lib/https.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,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;
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,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);
Expand Down
4 changes: 4 additions & 0 deletions lib/tls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,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)
);
Expand Down
4 changes: 4 additions & 0 deletions src/node_constants.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,10 @@ void DefineCryptoConstants(Local<Object> 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);
}
Expand Down
13 changes: 10 additions & 3 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,15 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& 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<Int32>()->Value();
int max_version = args[2].As<Int32>()->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
Expand All @@ -434,6 +438,9 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& 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;
Expand Down
69 changes: 43 additions & 26 deletions test/fixtures/tls-connect.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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();
}
}
4 changes: 2 additions & 2 deletions test/parallel/test-https-agent-getname.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const agent = new https.Agent();
// empty options
assert.strictEqual(
agent.getName({}),
'localhost:::::::::::::::::'
'localhost:::::::::::::::::::'
);

// pass all options arguments
Expand Down Expand Up @@ -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'
);
Loading

0 comments on commit acb7351

Please sign in to comment.