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: nodejs/node#43419

PR-URL: nodejs/node#43427
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: AdamMajer

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, () => {

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

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;

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

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) {

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;

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

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) {

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

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

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

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

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

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

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,