Skip to content

Commit 613a907

Browse files
kumarrishavUlisesGascon
authored andcommitted
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: #36655Fixes: #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]>
1 parent 9857364 commit 613a907

File tree

4 files changed

+68
-22
lines changed

4 files changed

+68
-22
lines changed

‎lib/internal/tls/secure-context.js‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,31 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
148148
ticketKeys,
149149
}=options;
150150

151+
// Set the cipher list and cipher suite before anything else because
152+
// @SECLEVEL=<n> changes the security level and that affects subsequent
153+
// operations.
154+
if(ciphers!==undefined&&ciphers!==null)
155+
validateString(ciphers,`${name}.ciphers`);
156+
157+
// Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
158+
// cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
159+
// cipher suites all have a standard name format beginning with TLS_, so split
160+
// the ciphers and pass them to the appropriate API.
161+
const{
162+
cipherList,
163+
cipherSuites,
164+
}=processCiphers(ciphers,`${name}.ciphers`);
165+
166+
if(cipherSuites!=='')
167+
context.setCipherSuites(cipherSuites);
168+
context.setCiphers(cipherList);
169+
170+
if(cipherList===''&&
171+
context.getMinProto()<TLS1_3_VERSION&&
172+
context.getMaxProto()>TLS1_2_VERSION){
173+
context.setMinProto(TLS1_3_VERSION);
174+
}
175+
151176
// Add CA before the cert to be able to load cert's issuer in C++ code.
152177
// NOTE(@jasnell): ca, cert, and key are permitted to be falsy, so do not
153178
// change the checks to !== undefined checks.
@@ -218,28 +243,6 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
218243
}
219244
}
220245

221-
if(ciphers!==undefined&&ciphers!==null)
222-
validateString(ciphers,`${name}.ciphers`);
223-
224-
// Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
225-
// cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
226-
// cipher suites all have a standard name format beginning with TLS_, so split
227-
// the ciphers and pass them to the appropriate API.
228-
const{
229-
cipherList,
230-
cipherSuites,
231-
}=processCiphers(ciphers,`${name}.ciphers`);
232-
233-
if(cipherSuites!=='')
234-
context.setCipherSuites(cipherSuites);
235-
context.setCiphers(cipherList);
236-
237-
if(cipherList===''&&
238-
context.getMinProto()<TLS1_3_VERSION&&
239-
context.getMaxProto()>TLS1_2_VERSION){
240-
context.setMinProto(TLS1_3_VERSION);
241-
}
242-
243246
validateString(ecdhCurve,`${name}.ecdhCurve`);
244247
context.setECDHCurve(ecdhCurve);
245248

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIBFjCBwaADAgECAgEBMA0GCSqGSIb3DQEBBQUAMBQxEjAQBgNVBAMTCWxvY2Fs
3+
aG9zdDAeFw0yMzEwMTUxNzQ5MTBaFw0yNDEwMTUxNzQ5MTBaMBQxEjAQBgNVBAMT
4+
CWxvY2FsaG9zdDBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQDW9vH7W98zSi1IfoTG
5+
pTjbvXRzmmSG6y5z1S3gvC6+keC5QQkEdIG5vWas1efX5qEPybptRyM34T6aWv+U
6+
uzUJAgMBAAEwDQYJKoZIhvcNAQEFBQADQQAEIwD5mLIALrim6uD39DO/umYDtDIb
7+
TAQmgWdkQrCdCtX0Yp49gJyaq2HtFgsk/cxMoYMYkDtT5a7nwEQu+Xqt
8+
-----END CERTIFICATE-----

‎test/fixtures/keys/agent11-key.pem‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
-----BEGIN RSA PRIVATE KEY-----
2+
MIIBOwIBAAJBANb28ftb3zNKLUh+hMalONu9dHOaZIbrLnPVLeC8Lr6R4LlBCQR0
3+
gbm9ZqzV59fmoQ/Jum1HIzfhPppa/5S7NQkCAwEAAQJAaetb6GKoY/lUvre4bLjU
4+
f1Gmo5+bkO8pAGI2LNoMnlETjLjlnvShkqu0kxY96G5Il6VSX4Yjz0D40f4IrlJW
5+
AQIhAPChOjGBlOFcGA/pPmzMcW8jRCLvVubiO9TpiYVhWz45AiEA5LIKsSR8HT9y
6+
eyVNNNkRbNvTrddbvXMBBjj+KwxQrVECIQDjalzHQQJl4lXTY8rdpHJoaNoSckSd
7+
PJ7zYCvaZOKI8QIhALoGbRYMxHySCJBNFlE/pKH06mnE/RXMf2/NWkov+UwRAiAz
8+
ucgBN8xY5KvG3eI78WHdE2B5X0B4EabFXmUlzIrhTA==
9+
-----END RSA PRIVATE KEY-----
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
constcommon=require('../common');
3+
4+
if(!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
constassert=require('assert');
8+
consttls=require('tls');
9+
constfixtures=require('../common/fixtures');
10+
11+
{
12+
constoptions={
13+
key: fixtures.readKey('agent11-key.pem'),
14+
cert: fixtures.readKey('agent11-cert.pem'),
15+
ciphers: 'DEFAULT'
16+
};
17+
18+
// Should throw error as key is too small because openssl v3 doesn't allow it
19+
assert.throws(()=>tls.createServer(options,common.mustNotCall()),
20+
/keytoosmall/i);
21+
22+
// Reducing SECLEVEL to 0 in ciphers retains compatibility with previous versions of OpenSSL like using a small key.
23+
// As ciphers are getting set before the cert and key get loaded.
24+
options.ciphers='DEFAULT:@SECLEVEL=0';
25+
assert.ok(tls.createServer(options,common.mustNotCall()));
26+
}

0 commit comments

Comments
(0)