Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()));
}