Skip to content

Commit 5b940dd

Browse files
pimterrynodejs-github-bot
pimterry
authored and
nodejs-github-bot
committed
tls: drop hand-rolled TLS client hello parser
This existed for 'resumeSession', which needed to do an async lookup though SSL_CTX_sess_set_get_cb is sync-only. Nowadays both OpenSSL & BoringSSL have an early ClientHello callback for suspend/resume to handle this properly, so it was redundant, in addition to being complicated and generally a bit fragile & scary. This PR switches to use the modern OpenSSL/BoringSSL mechanisms for this and drops the client hello parser & related infrastructure completely. In addition, there's a new test here, covering a fixed bug: the hello parser silently dropped fragmented hellos, which we now do handle correctly. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64827 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
1 parent 72768c7 commit 5b940dd

14 files changed

Lines changed: 283 additions & 772 deletions

‎lib/internal/tls/wrap.js‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -262,8 +262,8 @@ function loadSession(hello) {
262262
return owner.destroy(new ERR_SOCKET_CLOSED());
263263

264264
owner._handle.loadSession(session);
265-
// Session is loaded. End the parser to allow handshaking to continue.
266-
owner._handle.endParser();
265+
// Session is loaded. Let the handshake continue.
266+
owner._handle.clientHelloDone();
267267
}
268268

269269
if (hello.sessionId.length <= 0 ||
@@ -281,8 +281,8 @@ function loadSession(hello) {
281281
// Sessions with tickets can be resumed directly from the ticket, no server
282282
// session storage is necessary.
283283
// Without a call to a resumeSession listener, a session will never be
284-
// loaded, so end the parser to allow handshaking to continue.
285-
owner._handle.endParser();
284+
// loaded, so let the handshake continue.
285+
owner._handle.clientHelloDone();
286286
}
287287
}
288288

@@ -970,7 +970,6 @@ TLSSocket.prototype._init = function(socket, wrap) {
970970
if (this.server) {
971971
if (this.server.listenerCount('resumeSession') > 0 ||
972972
this.server.listenerCount('newSession') > 0) {
973-
// Also starts the client hello parser as a side effect.
974973
ssl.enableSessionCallbacks();
975974
}
976975
if (this.server.listenerCount('OCSPRequest') > 0)

‎node.gyp‎

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,6 @@
417417
'src/crypto/crypto_rsa.cc',
418418
'src/crypto/crypto_spkac.cc',
419419
'src/crypto/crypto_util.cc',
420-
'src/crypto/crypto_clienthello.cc',
421420
'src/crypto/crypto_dh.cc',
422421
'src/crypto/crypto_hash.cc',
423422
'src/crypto/crypto_keys.cc',
@@ -427,7 +426,6 @@
427426
'src/crypto/crypto_x509.cc',
428427
'src/crypto/crypto_argon2.h',
429428
'src/crypto/crypto_bio.h',
430-
'src/crypto/crypto_clienthello-inl.h',
431429
'src/crypto/crypto_dh.h',
432430
'src/crypto/crypto_hmac.h',
433431
'src/crypto/crypto_kmac.h',
@@ -443,7 +441,6 @@
443441
'src/crypto/crypto_keygen.h',
444442
'src/crypto/crypto_scrypt.h',
445443
'src/crypto/crypto_tls.h',
446-
'src/crypto/crypto_clienthello.h',
447444
'src/crypto/crypto_context.h',
448445
'src/crypto/crypto_ec.h',
449446
'src/crypto/crypto_pqc.h',
@@ -473,7 +470,6 @@
473470
'src/tracing/trace_event_legacy.h',
474471
],
475472
'node_cctest_openssl_sources': [
476-
'test/cctest/test_crypto_clienthello.cc',
477473
'test/cctest/test_node_crypto.cc',
478474
'test/cctest/test_node_crypto_env.cc',
479475
],
@@ -1316,54 +1312,6 @@
13161312
}],
13171313
],
13181314
}, # fuzz_env
1319-
{ # fuzz_ClientHelloParser.cc
1320-
'target_name': 'fuzz_ClientHelloParser',
1321-
'type': 'executable',
1322-
'dependencies': [
1323-
'<(node_lib_target_name)',
1324-
],
1325-
'includes': [
1326-
'node.gypi'
1327-
],
1328-
'include_dirs': [
1329-
'src',
1330-
'tools/msvs/genfiles',
1331-
'deps/v8/include',
1332-
'deps/cares/include',
1333-
'deps/uv/include',
1334-
'test/cctest',
1335-
],
1336-
'defines': [
1337-
'NODE_ARCH="<(target_arch)"',
1338-
'NODE_PLATFORM="<(OS)"',
1339-
'NODE_WANT_INTERNALS=1',
1340-
],
1341-
'sources': [
1342-
'test/fuzzers/fuzz_ClientHelloParser.cc',
1343-
],
1344-
'conditions': [
1345-
[ 'node_shared_hdr_histogram=="false"', {
1346-
'dependencies': [
1347-
'deps/histogram/histogram.gyp:histogram',
1348-
],
1349-
}],
1350-
[ 'node_shared_uvwasi=="false"', {
1351-
'dependencies': [ 'deps/uvwasi/uvwasi.gyp:uvwasi' ],
1352-
'include_dirs': [ 'deps/uvwasi/include' ],
1353-
}],
1354-
['OS=="linux" or OS=="openharmony"', {
1355-
'ldflags': [ '-fsanitize=fuzzer' ]
1356-
}],
1357-
# Ensure that ossfuzz flag has been set and that we are on Linux
1358-
[ 'OS not in "linux openharmony" or ossfuzz!="true"', {
1359-
'type': 'none',
1360-
}],
1361-
# Avoid excessive LTO
1362-
['enable_lto=="true"', {
1363-
'ldflags': [ '-fno-lto' ],
1364-
}],
1365-
],
1366-
}, # fuzz_ClientHelloParser.cc
13671315
{ # fuzz_strings
13681316
'target_name': 'fuzz_strings',
13691317
'type': 'executable',

‎src/crypto/README.md‎

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -30,27 +30,26 @@ throughout the rest of the code.
3030
The rest of the files are structured by their function, as detailed in the
3131
following table:
3232

33-
| File (\*.h/\*.cc) | Description |
34-
| -------------------- | -------------------------------------------------------------------------- |
35-
| `crypto_aes` | AES Cipher support. |
36-
| `crypto_argon2` | Argon2 key / bit generation implementation. |
37-
| `crypto_cipher` | General Encryption/Decryption utilities. |
38-
| `crypto_clienthello` | TLS/SSL client hello parser implementation. Used during SSL/TLS handshake. |
39-
| `crypto_context` | Implementation of the `SecureContext` object. |
40-
| `crypto_dh` | Diffie-Hellman Key Agreement implementation. |
41-
| `crypto_dsa` | DSA (Digital Signature) Key Generation functions. |
42-
| `crypto_ec` | Elliptic-curve cryptography implementation. |
43-
| `crypto_hash` | Basic hash (e.g. SHA-256) functions. |
44-
| `crypto_hkdf` | HKDF (Key derivation) implementation. |
45-
| `crypto_hmac` | HMAC implementations. |
46-
| `crypto_keys` | Utilities for using and generating secret, private, and public keys. |
47-
| `crypto_pbkdf2` | PBKDF2 key / bit generation implementation. |
48-
| `crypto_rsa` | RSA Key Generation functions. |
49-
| `crypto_scrypt` | Scrypt key / bit generation implementation. |
50-
| `crypto_sig` | General digital signature and verification utilities. |
51-
| `crypto_spkac` | Netscape SPKAC certificate utilities. |
52-
| `crypto_ssl` | Implementation of the `SSLWrap` object. |
53-
| `crypto_timing` | Implementation of the TimingSafeEqual. |
33+
| File (\*.h/\*.cc) | Description |
34+
| ----------------- | -------------------------------------------------------------------- |
35+
| `crypto_aes` | AES Cipher support. |
36+
| `crypto_argon2` | Argon2 key / bit generation implementation. |
37+
| `crypto_cipher` | General Encryption/Decryption utilities. |
38+
| `crypto_context` | Implementation of the `SecureContext` object. |
39+
| `crypto_dh` | Diffie-Hellman Key Agreement implementation. |
40+
| `crypto_dsa` | DSA (Digital Signature) Key Generation functions. |
41+
| `crypto_ec` | Elliptic-curve cryptography implementation. |
42+
| `crypto_hash` | Basic hash (e.g. SHA-256) functions. |
43+
| `crypto_hkdf` | HKDF (Key derivation) implementation. |
44+
| `crypto_hmac` | HMAC implementations. |
45+
| `crypto_keys` | Utilities for using and generating secret, private, and public keys. |
46+
| `crypto_pbkdf2` | PBKDF2 key / bit generation implementation. |
47+
| `crypto_rsa` | RSA Key Generation functions. |
48+
| `crypto_scrypt` | Scrypt key / bit generation implementation. |
49+
| `crypto_sig` | General digital signature and verification utilities. |
50+
| `crypto_spkac` | Netscape SPKAC certificate utilities. |
51+
| `crypto_ssl` | Implementation of the `SSLWrap` object. |
52+
| `crypto_timing` | Implementation of the TimingSafeEqual. |
5453

5554
When new crypto protocols are added, they will be added into their own
5655
`crypto_` `*.h` and `*.cc` files.

‎src/crypto/crypto_clienthello-inl.h‎

Lines changed: 0 additions & 90 deletions
This file was deleted.

0 commit comments

Comments
 (0)