From a3a845b8ba2f0ea4c2ca9e734520f496a13cf6de 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 76658fc3c42ad7..08be1f7e46f8ba 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 f3a96abcbc0174..023b42cbeda685 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 a505a719d30884..d3018cf851db75 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 3fc2ecb614978b..db50306485aec3 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 152627b420a612..a9bf4a1da71eca 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 73ac215102ddbb..95f232de637c47 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 a6fbc67bd88361..2b243575071e1a 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 ef645ce1b6c624..0d531a3d6f0179 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 7b535ecd3a18f0..1ca5c22335c850 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 25e6314a54a58d..3cf02701f4dc19 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 c0d2625a9a6168..8c879f850c9b8d 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 744276aa59bf37..2a234d59016c1c 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 b9eaa05d59feb6..22a80d9d377727 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 f353183ce2fa0c..80bd0d37281f13 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 f08af9b089adb9..c2d9740201d74a 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,