Skip to content

Commit

Permalink
tls: fix order of setting cipher before setting cert and key
Browse files Browse the repository at this point in the history
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 <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
  • Loading branch information
kumarrishav authored and UlisesGascon committed Dec 19, 2023
1 parent 9857364 commit 613a907
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 22 deletions.
47 changes: 25 additions & 22 deletions lib/internal/tls/secure-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);

Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/keys/agent11-cert.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-----BEGIN CERTIFICATE-----
MIIBFjCBwaADAgECAgEBMA0GCSqGSIb3DQEBBQUAMBQxEjAQBgNVBAMTCWxvY2Fs
aG9zdDAeFw0yMzEwMTUxNzQ5MTBaFw0yNDEwMTUxNzQ5MTBaMBQxEjAQBgNVBAMT
CWxvY2FsaG9zdDBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQDW9vH7W98zSi1IfoTG
pTjbvXRzmmSG6y5z1S3gvC6+keC5QQkEdIG5vWas1efX5qEPybptRyM34T6aWv+U
uzUJAgMBAAEwDQYJKoZIhvcNAQEFBQADQQAEIwD5mLIALrim6uD39DO/umYDtDIb
TAQmgWdkQrCdCtX0Yp49gJyaq2HtFgsk/cxMoYMYkDtT5a7nwEQu+Xqt
-----END CERTIFICATE-----
9 changes: 9 additions & 0 deletions test/fixtures/keys/agent11-key.pem
Original file line number Diff line number Diff line change
@@ -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-----
26 changes: 26 additions & 0 deletions test/parallel/test-tls-reduced-SECLEVEL-in-cipher.js
Original file line number Diff line number Diff line change
@@ -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()));
}

0 comments on commit 613a907

Please sign in to comment.