From 9cde7a033e66a8e9e691c708cfb448161fa2c27d Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Mon, 27 Jun 2022 10:47:13 +0200 Subject: [PATCH] crypto: don't disable TLS 1.3 without suites In the manual page, there is a statement that ciphersuites contain explicit default settings - all TLS 1.3 ciphersuites enabled. In node, we assume that an empty setting mean no ciphersuites and we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to disable TLS 1.3 and by not override the default ciphersuits with an empty string. So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit list of ciphers. If none are acceptable, the correct approach is to disable TLS 1.3 instead elsewhere. Fixes: https://github.com/nodejs/node/issues/43419 PR-URL: https://github.com/nodejs/node/pull/43427 Reviewed-By: Matteo Collina Reviewed-By: Paolo Insogna Reviewed-By: James M Snell --- benchmark/tls/secure-pair.js | 2 ++ benchmark/tls/throughput-c2s.js | 3 ++- benchmark/tls/throughput-s2c.js | 3 ++- benchmark/tls/tls-connect.js | 3 ++- lib/internal/tls/secure-context.js | 9 ++------ .../test-tls-client-getephemeralkeyinfo.js | 3 ++- test/parallel/test-tls-client-mindhsize.js | 3 ++- test/parallel/test-tls-dhe.js | 3 ++- test/parallel/test-tls-ecdh-auto.js | 3 ++- test/parallel/test-tls-ecdh-multiple.js | 3 ++- test/parallel/test-tls-ecdh.js | 3 ++- test/parallel/test-tls-getcipher.js | 6 ++++-- test/parallel/test-tls-multi-key.js | 6 ++++-- test/parallel/test-tls-multi-pfx.js | 6 ++++-- test/parallel/test-tls-set-ciphers.js | 21 ++++++++++++++++++- 15 files changed, 54 insertions(+), 23 deletions(-) diff --git a/benchmark/tls/secure-pair.js b/benchmark/tls/secure-pair.js index 76658fc3c42..08be1f7e46f 100644 --- a/benchmark/tls/secure-pair.js +++ b/benchmark/tls/secure-pair.js @@ -25,6 +25,7 @@ function main({ dur, size, securing }) { isServer: true, requestCert: true, rejectUnauthorized: true, + maxVersion: 'TLSv1.2', }; const server = net.createServer(onRedirectConnection); @@ -38,6 +39,7 @@ function main({ dur, size, securing }) { cert: options.cert, isServer: false, rejectUnauthorized: false, + maxVersion: options.maxVersion, }; const network = securing === 'clear' ? net : tls; const conn = network.connect(clientOptions, () => { diff --git a/benchmark/tls/throughput-c2s.js b/benchmark/tls/throughput-c2s.js index f3a96abcbc0..023b42cbeda 100644 --- a/benchmark/tls/throughput-c2s.js +++ b/benchmark/tls/throughput-c2s.js @@ -33,7 +33,8 @@ function main({ dur, type, size }) { key: fixtures.readKey('rsa_private.pem'), cert: fixtures.readKey('rsa_cert.crt'), ca: fixtures.readKey('rsa_ca.crt'), - ciphers: 'AES256-GCM-SHA384' + ciphers: 'AES256-GCM-SHA384', + maxVersion: 'TLSv1.2', }; const server = tls.createServer(options, onConnection); diff --git a/benchmark/tls/throughput-s2c.js b/benchmark/tls/throughput-s2c.js index a505a719d30..d3018cf851d 100644 --- a/benchmark/tls/throughput-s2c.js +++ b/benchmark/tls/throughput-s2c.js @@ -40,7 +40,8 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { key: fixtures.readKey('rsa_private.pem'), cert: fixtures.readKey('rsa_cert.crt'), ca: fixtures.readKey('rsa_ca.crt'), - ciphers: 'AES256-GCM-SHA384' + ciphers: 'AES256-GCM-SHA384', + maxVersion: 'TLSv1.2', }; let socketOpts; diff --git a/benchmark/tls/tls-connect.js b/benchmark/tls/tls-connect.js index 3fc2ecb6149..db50306485a 100644 --- a/benchmark/tls/tls-connect.js +++ b/benchmark/tls/tls-connect.js @@ -21,7 +21,8 @@ function main(conf) { key: fixtures.readKey('rsa_private.pem'), cert: fixtures.readKey('rsa_cert.crt'), ca: fixtures.readKey('rsa_ca.crt'), - ciphers: 'AES256-GCM-SHA384' + ciphers: 'AES256-GCM-SHA384', + maxVersion: 'TLSv1.2', }; const server = tls.createServer(options, onConnection); diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js index 152627b420a..a9bf4a1da71 100644 --- a/lib/internal/tls/secure-context.js +++ b/lib/internal/tls/secure-context.js @@ -225,15 +225,10 @@ function configSecureContext(context, options = kEmptyObject, name = 'options') cipherSuites, } = processCiphers(ciphers, `${name}.ciphers`); - context.setCipherSuites(cipherSuites); + if (cipherSuites !== '') + context.setCipherSuites(cipherSuites); context.setCiphers(cipherList); - if (cipherSuites === '' && - context.getMaxProto() > TLS1_2_VERSION && - context.getMinProto() < TLS1_3_VERSION) { - context.setMaxProto(TLS1_2_VERSION); - } - if (cipherList === '' && context.getMinProto() < TLS1_3_VERSION && context.getMaxProto() > TLS1_2_VERSION) { diff --git a/test/parallel/test-tls-client-getephemeralkeyinfo.js b/test/parallel/test-tls-client-getephemeralkeyinfo.js index 82f41cfe7e0..8abb319b828 100644 --- a/test/parallel/test-tls-client-getephemeralkeyinfo.js +++ b/test/parallel/test-tls-client-getephemeralkeyinfo.js @@ -23,7 +23,8 @@ function test(size, type, name, cipher) { const options = { key: key, cert: cert, - ciphers: cipher + ciphers: cipher, + maxVersion: 'TLSv1.2', }; if (name) options.ecdhCurve = name; diff --git a/test/parallel/test-tls-client-mindhsize.js b/test/parallel/test-tls-client-mindhsize.js index 1ccf49fa929..92ac9959368 100644 --- a/test/parallel/test-tls-client-mindhsize.js +++ b/test/parallel/test-tls-client-mindhsize.js @@ -41,7 +41,8 @@ function test(size, err, next) { const client = tls.connect({ minDHSize: 2048, port: this.address().port, - rejectUnauthorized: false + rejectUnauthorized: false, + maxVersion: 'TLSv1.2', }, function() { nsuccess++; server.close(); diff --git a/test/parallel/test-tls-dhe.js b/test/parallel/test-tls-dhe.js index ef645ce1b6c..0d531a3d6f0 100644 --- a/test/parallel/test-tls-dhe.js +++ b/test/parallel/test-tls-dhe.js @@ -53,7 +53,8 @@ function test(keylen, expectedCipher, cb) { key: key, cert: cert, ciphers: ciphers, - dhparam: loadDHParam(keylen) + dhparam: loadDHParam(keylen), + maxVersion: 'TLSv1.2', }; const server = tls.createServer(options, function(conn) { diff --git a/test/parallel/test-tls-ecdh-auto.js b/test/parallel/test-tls-ecdh-auto.js index 7b535ecd3a1..1ca5c22335c 100644 --- a/test/parallel/test-tls-ecdh-auto.js +++ b/test/parallel/test-tls-ecdh-auto.js @@ -23,7 +23,8 @@ const options = { key: loadPEM('agent2-key'), cert: loadPEM('agent2-cert'), ciphers: '-ALL:ECDHE-RSA-AES128-SHA256', - ecdhCurve: 'auto' + ecdhCurve: 'auto', + maxVersion: 'TLSv1.2', }; const reply = 'I AM THE WALRUS'; // Something recognizable diff --git a/test/parallel/test-tls-ecdh-multiple.js b/test/parallel/test-tls-ecdh-multiple.js index 25e6314a54a..3cf02701f4d 100644 --- a/test/parallel/test-tls-ecdh-multiple.js +++ b/test/parallel/test-tls-ecdh-multiple.js @@ -23,7 +23,8 @@ const options = { key: loadPEM('agent2-key'), cert: loadPEM('agent2-cert'), ciphers: '-ALL:ECDHE-RSA-AES128-SHA256', - ecdhCurve: 'secp256k1:prime256v1:secp521r1' + ecdhCurve: 'secp256k1:prime256v1:secp521r1', + maxVersion: 'TLSv1.2', }; const reply = 'I AM THE WALRUS'; // Something recognizable diff --git a/test/parallel/test-tls-ecdh.js b/test/parallel/test-tls-ecdh.js index c0d2625a9a6..8c879f850c9 100644 --- a/test/parallel/test-tls-ecdh.js +++ b/test/parallel/test-tls-ecdh.js @@ -38,7 +38,8 @@ const options = { key: fixtures.readKey('agent2-key.pem'), cert: fixtures.readKey('agent2-cert.pem'), ciphers: '-ALL:ECDHE-RSA-AES128-SHA256', - ecdhCurve: 'prime256v1' + ecdhCurve: 'prime256v1', + maxVersion: 'TLSv1.2' }; const reply = 'I AM THE WALRUS'; // Something recognizable diff --git a/test/parallel/test-tls-getcipher.js b/test/parallel/test-tls-getcipher.js index 744276aa59b..2a234d59016 100644 --- a/test/parallel/test-tls-getcipher.js +++ b/test/parallel/test-tls-getcipher.js @@ -48,7 +48,8 @@ server.listen(0, '127.0.0.1', common.mustCall(function() { host: '127.0.0.1', port: this.address().port, ciphers: 'AES128-SHA256', - rejectUnauthorized: false + rejectUnauthorized: false, + maxVersion: 'TLSv1.2', }, common.mustCall(function() { const cipher = this.getCipher(); assert.strictEqual(cipher.name, 'AES128-SHA256'); @@ -62,7 +63,8 @@ server.listen(0, '127.0.0.1', common.mustCall(function() { host: '127.0.0.1', port: this.address().port, ciphers: 'ECDHE-RSA-AES128-GCM-SHA256', - rejectUnauthorized: false + rejectUnauthorized: false, + maxVersion: 'TLSv1.2', }, common.mustCall(function() { const cipher = this.getCipher(); assert.strictEqual(cipher.name, 'ECDHE-RSA-AES128-GCM-SHA256'); diff --git a/test/parallel/test-tls-multi-key.js b/test/parallel/test-tls-multi-key.js index b9eaa05d59f..22a80d9d377 100644 --- a/test/parallel/test-tls-multi-key.js +++ b/test/parallel/test-tls-multi-key.js @@ -154,11 +154,12 @@ function test(options) { rejectUnauthorized: true, ca: clientTrustRoots, checkServerIdentity: (_, c) => assert.strictEqual(c.subject.CN, eccCN), + maxVersion: 'TLSv1.2' }, common.mustCall(function() { assert.deepStrictEqual(ecdsa.getCipher(), { name: 'ECDHE-ECDSA-AES256-GCM-SHA384', standardName: 'TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384', - version: 'TLSv1.2' + version: 'TLSv1.2', }); assert.strictEqual(ecdsa.getPeerCertificate().subject.CN, eccCN); assert.strictEqual(ecdsa.getPeerCertificate().asn1Curve, 'prime256v1'); @@ -173,11 +174,12 @@ function test(options) { rejectUnauthorized: true, ca: clientTrustRoots, checkServerIdentity: (_, c) => assert.strictEqual(c.subject.CN, rsaCN), + maxVersion: 'TLSv1.2', }, common.mustCall(function() { assert.deepStrictEqual(rsa.getCipher(), { name: 'ECDHE-RSA-AES256-GCM-SHA384', standardName: 'TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384', - version: 'TLSv1.2' + version: 'TLSv1.2', }); assert.strictEqual(rsa.getPeerCertificate().subject.CN, rsaCN); assert(rsa.getPeerCertificate().exponent, 'cert for an RSA key'); diff --git a/test/parallel/test-tls-multi-pfx.js b/test/parallel/test-tls-multi-pfx.js index f353183ce2f..80bd0d37281 100644 --- a/test/parallel/test-tls-multi-pfx.js +++ b/test/parallel/test-tls-multi-pfx.js @@ -24,12 +24,14 @@ const server = tls.createServer(options, function(conn) { }).listen(0, function() { const ecdsa = tls.connect(this.address().port, { ciphers: 'ECDHE-ECDSA-AES256-GCM-SHA384', - rejectUnauthorized: false + maxVersion: 'TLSv1.2', + rejectUnauthorized: false, }, common.mustCall(function() { ciphers.push(ecdsa.getCipher()); const rsa = tls.connect(server.address().port, { ciphers: 'ECDHE-RSA-AES256-GCM-SHA384', - rejectUnauthorized: false + maxVersion: 'TLSv1.2', + rejectUnauthorized: false, }, common.mustCall(function() { ciphers.push(rsa.getCipher()); ecdsa.end(); diff --git a/test/parallel/test-tls-set-ciphers.js b/test/parallel/test-tls-set-ciphers.js index f08af9b089a..c2d9740201d 100644 --- a/test/parallel/test-tls-set-ciphers.js +++ b/test/parallel/test-tls-set-ciphers.js @@ -13,19 +13,31 @@ const { } = require(fixtures.path('tls-connect')); -function test(cciphers, sciphers, cipher, cerr, serr) { +function test(cciphers, sciphers, cipher, cerr, serr, options) { assert(cipher || cerr || serr, 'test missing any expectations'); const where = inspect(new Error()).split('\n')[2].replace(/[^(]*/, ''); + + const max_tls_ver = (ciphers, options) => { + if (options instanceof Object && Object.hasOwn(options, 'maxVersion')) + return options.maxVersion; + if ((typeof ciphers === 'string' || ciphers instanceof String) && ciphers.length > 0 && !ciphers.includes('TLS_')) + return 'TLSv1.2'; + + return 'TLSv1.3'; + }; + connect({ client: { checkServerIdentity: (servername, cert) => { }, ca: `${keys.agent1.cert}\n${keys.agent6.ca}`, ciphers: cciphers, + maxVersion: max_tls_ver(cciphers, options), }, server: { cert: keys.agent6.cert, key: keys.agent6.key, ciphers: sciphers, + maxVersion: max_tls_ver(sciphers, options), }, }, common.mustCall((err, pair, cleanup) => { function u(_) { return _ === undefined ? 'U' : _; } @@ -87,6 +99,13 @@ test('AES128-SHA:TLS_AES_256_GCM_SHA384', test('AES256-SHA:TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384'); test(U, 'AES256-SHA:TLS_AES_256_GCM_SHA384', 'TLS_AES_256_GCM_SHA384'); +// Cipher order ignored, TLS1.3 before TLS1.2 and +// cipher suites are not disabled if TLS ciphers are set only +// TODO: maybe these tests should be reworked so maxVersion clamping +// is done explicitly and not implicitly in the test() function +test('AES256-SHA', U, 'TLS_AES_256_GCM_SHA384', U, U, { maxVersion: 'TLSv1.3' }); +test(U, 'AES256-SHA', 'TLS_AES_256_GCM_SHA384', U, U, { maxVersion: 'TLSv1.3' }); + // TLS_AES_128_CCM_8_SHA256 & TLS_AES_128_CCM_SHA256 are not enabled by // default, but work. test('TLS_AES_128_CCM_8_SHA256', U,