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#43419
AdamMajer · Jun 20, 2022 · 7f25b5b · 7f25b5b
1 parent 3d0a0b6
commit 7f25b5b
Show file tree

Hide file tree

Showing 12 changed files with 49 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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-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
@@ -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
@@ -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
@@ -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 && options.hasOwnProperty('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,
@@ -108,3 +127,4 @@ test('', 'TLS_AES_256_GCM_SHA384', 'TLS_AES_256_GCM_SHA384');
 // Using null should be treated the same as undefined.
 test(null, 'AES256-SHA', 'AES256-SHA');
 test('AES256-SHA', null, 'AES256-SHA');
+