Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit d9ea6c1

Browse filesBrowse files
kumarrishavrichardlau
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: #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>
1 parent 194ff6a commit d9ea6c1
Copy full SHA for d9ea6c1

File tree

Expand file treeCollapse file tree

4 files changed

+68
-22
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+68
-22
lines changed
Open diff view settings
Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/tls/secure-context.js
+25-22Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,31 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
144144
ticketKeys,
145145
} = options;
146146

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

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

Collapse file
+8Lines 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-----
Collapse file

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

Copy file name to clipboard
+9Lines 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-----
Collapse file
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
const common = require('../common');
3+
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const tls = require('tls');
9+
const fixtures = require('../common/fixtures');
10+
11+
{
12+
const options = {
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+
/key too small/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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.