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: #43419 PR-URL: #43427 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Jul 20, 2022 · 03fefb2 · 03fefb2
1 parent 8d2389a
commit 03fefb2
Show file tree

Hide file tree

Showing 15 changed files with 54 additions and 23 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-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
@@ -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
@@ -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
@@ -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 && 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,