From aeccc8227e7e0fe91731be8f89fc262a15165e1d Mon Sep 17 00:00:00 2001 From: Adam Majer Date: Tue, 14 Jun 2022 16:43:52 +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 --- 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 76658fc3c42a..08be1f7e46f8 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 f3a96abcbc01..023b42cbeda6 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 a505a719d308..d3018cf851db 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 3fc2ecb61497..db50306485ae 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 152627b420a6..a9bf4a1da71e 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 82f41cfe7e06..8abb319b8288 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 1ccf49fa9292..92ac99593682 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 ef645ce1b6c6..0d531a3d6f01 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 7b535ecd3a18..1ca5c22335c8 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 25e6314a54a5..3cf02701f4dc 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 c0d2625a9a61..8c879f850c9b 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 744276aa59bf..2a234d59016c 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 b9eaa05d59fe..22a80d9d3777 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 f353183ce2fa..80bd0d37281f 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 f08af9b089ad..c2d9740201d7 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,