tls: fix order of setting cipher before setting cert and key

Set the cipher list and cipher suite before anything else because @SECLEVEL=<n> changes the security level and that affects subsequent operations. Fixes: #36655 Fixes: #49549 Refs: https://github.com/orgs/nodejs/discussions/49634 Refs: https://github.com/orgs/nodejs/discussions/46545 Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html PR-URL: #50186 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
nodejs · Nov 23, 2023 · dee8039 · dee8039
1 parent f7245d7
commit dee8039
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 22 deletions.
diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
@@ -148,6 +148,31 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     ticketKeys,
   } = options;
 
+  // Set the cipher list and cipher suite before anything else because
+  // @SECLEVEL=<n> changes the security level and that affects subsequent
+  // operations.
+  if (ciphers !== undefined && ciphers !== null)
+    validateString(ciphers, `${name}.ciphers`);
+
+  // Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
+  // cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
+  // cipher suites all have a standard name format beginning with TLS_, so split
+  // the ciphers and pass them to the appropriate API.
+  const {
+    cipherList,
+    cipherSuites,
+  } = processCiphers(ciphers, `${name}.ciphers`);
+
+  if (cipherSuites !== '')
+    context.setCipherSuites(cipherSuites);
+  context.setCiphers(cipherList);
+
+  if (cipherList === '' &&
+      context.getMinProto() < TLS1_3_VERSION &&
+      context.getMaxProto() > TLS1_2_VERSION) {
+    context.setMinProto(TLS1_3_VERSION);
+  }
+
   // Add CA before the cert to be able to load cert's issuer in C++ code.
   // NOTE(@jasnell): ca, cert, and key are permitted to be falsy, so do not
   // change the checks to !== undefined checks.
@@ -218,28 +243,6 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     }
   }
 
-  if (ciphers !== undefined && ciphers !== null)
-    validateString(ciphers, `${name}.ciphers`);
-
-  // Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
-  // cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
-  // cipher suites all have a standard name format beginning with TLS_, so split
-  // the ciphers and pass them to the appropriate API.
-  const {
-    cipherList,
-    cipherSuites,
-  } = processCiphers(ciphers, `${name}.ciphers`);
-
-  if (cipherSuites !== '')
-    context.setCipherSuites(cipherSuites);
-  context.setCiphers(cipherList);
-
-  if (cipherList === '' &&
-      context.getMinProto() < TLS1_3_VERSION &&
-      context.getMaxProto() > TLS1_2_VERSION) {
-    context.setMinProto(TLS1_3_VERSION);
-  }
-
   validateString(ecdhCurve, `${name}.ecdhCurve`);
   context.setECDHCurve(ecdhCurve);
 

diff --git a/test/fixtures/keys/agent11-cert.pem b/test/fixtures/keys/agent11-cert.pem
@@ -0,0 +1,8 @@
+-----BEGIN CERTIFICATE-----
+MIIBFjCBwaADAgECAgEBMA0GCSqGSIb3DQEBBQUAMBQxEjAQBgNVBAMTCWxvY2Fs
+aG9zdDAeFw0yMzEwMTUxNzQ5MTBaFw0yNDEwMTUxNzQ5MTBaMBQxEjAQBgNVBAMT
+CWxvY2FsaG9zdDBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQDW9vH7W98zSi1IfoTG
+pTjbvXRzmmSG6y5z1S3gvC6+keC5QQkEdIG5vWas1efX5qEPybptRyM34T6aWv+U
+uzUJAgMBAAEwDQYJKoZIhvcNAQEFBQADQQAEIwD5mLIALrim6uD39DO/umYDtDIb
+TAQmgWdkQrCdCtX0Yp49gJyaq2HtFgsk/cxMoYMYkDtT5a7nwEQu+Xqt
+-----END CERTIFICATE-----
diff --git a/test/fixtures/keys/agent11-key.pem b/test/fixtures/keys/agent11-key.pem
@@ -0,0 +1,9 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIBOwIBAAJBANb28ftb3zNKLUh+hMalONu9dHOaZIbrLnPVLeC8Lr6R4LlBCQR0
+gbm9ZqzV59fmoQ/Jum1HIzfhPppa/5S7NQkCAwEAAQJAaetb6GKoY/lUvre4bLjU
+f1Gmo5+bkO8pAGI2LNoMnlETjLjlnvShkqu0kxY96G5Il6VSX4Yjz0D40f4IrlJW
+AQIhAPChOjGBlOFcGA/pPmzMcW8jRCLvVubiO9TpiYVhWz45AiEA5LIKsSR8HT9y
+eyVNNNkRbNvTrddbvXMBBjj+KwxQrVECIQDjalzHQQJl4lXTY8rdpHJoaNoSckSd
+PJ7zYCvaZOKI8QIhALoGbRYMxHySCJBNFlE/pKH06mnE/RXMf2/NWkov+UwRAiAz
+ucgBN8xY5KvG3eI78WHdE2B5X0B4EabFXmUlzIrhTA==
+-----END RSA PRIVATE KEY-----
diff --git a/test/parallel/test-tls-reduced-SECLEVEL-in-cipher.js b/test/parallel/test-tls-reduced-SECLEVEL-in-cipher.js
@@ -0,0 +1,26 @@
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+{
+  const options = {
+    key: fixtures.readKey('agent11-key.pem'),
+    cert: fixtures.readKey('agent11-cert.pem'),
+    ciphers: 'DEFAULT'
+  };
+
+  // Should throw error as key is too small because openssl v3 doesn't allow it
+  assert.throws(() => tls.createServer(options, common.mustNotCall()),
+                /key too small/i);
+
+  // Reducing SECLEVEL to 0 in ciphers retains compatibility with previous versions of OpenSSL like using a small key.
+  // As ciphers are getting set before the cert and key get loaded.
+  options.ciphers = 'DEFAULT:@SECLEVEL=0';
+  assert.ok(tls.createServer(options, common.mustNotCall()));
+}