From dee8039c9b18eaa28427d4c535e98e12a1a91410 Mon Sep 17 00:00:00 2001 From: Kumar Rishav Date: Mon, 16 Oct 2023 19:55:36 +0000 Subject: [PATCH] tls: fix order of setting cipher before setting cert and key Set the cipher list and cipher suite before anything else because @SECLEVEL= changes the security level and that affects subsequent operations. Fixes: https://github.com/nodejs/node/issues/36655 Fixes: https://github.com/nodejs/node/issues/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: https://github.com/nodejs/node/pull/50186 Reviewed-By: James M Snell Reviewed-By: Paolo Insogna --- lib/internal/tls/secure-context.js | 47 ++++++++++--------- test/fixtures/keys/agent11-cert.pem | 8 ++++ test/fixtures/keys/agent11-key.pem | 9 ++++ .../test-tls-reduced-SECLEVEL-in-cipher.js | 26 ++++++++++ 4 files changed, 68 insertions(+), 22 deletions(-) create mode 100644 test/fixtures/keys/agent11-cert.pem create mode 100644 test/fixtures/keys/agent11-key.pem create mode 100644 test/parallel/test-tls-reduced-SECLEVEL-in-cipher.js diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js index 0fa3098ffa1020..40d995f9890d53 100644 --- 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= 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 new file mode 100644 index 00000000000000..42b34d537fc535 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..a8bccd007c857c --- /dev/null +++ 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 new file mode 100644 index 00000000000000..9f4458e0a7d671 --- /dev/null +++ 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())); +}