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 <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: James M Snell <jasnell@gmail.com>

15 files changed, 54 insertions, 23 deletions

diff --git a/benchmark/tls/secure-pair.js b/benchmark/tls/secure-pair.js
index 76658fc3c4..08be1f7e46 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 f3a96abcbc..023b42cbed 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 a505a719d3..d3018cf851 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 3fc2ecb614..db50306485 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 152627b420..a9bf4a1da7 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 82f41cfe7e..8abb319b82 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 1ccf49fa92..92ac995936 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 ef645ce1b6..0d531a3d6f 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 7b535ecd3a..1ca5c22335 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 25e6314a54..3cf02701f4 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 c0d2625a9a..8c879f850c 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 744276aa59..2a234d5901 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 b9eaa05d59..22a80d9d37 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 f353183ce2..80bd0d3728 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 f08af9b089..c2d9740201 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,