Skip to content

Commit d75957f

Browse files
pimterryaduh95
pimterry
authored and
aduh95
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 d4eb6c6 commit d75957f

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
@@ -263,8 +263,8 @@ function loadSession(hello) {
263263
return owner.destroy(new ERR_SOCKET_CLOSED());
264264

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

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

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

‎node.gyp‎

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,6 @@
402402
'src/crypto/crypto_rsa.cc',
403403
'src/crypto/crypto_spkac.cc',
404404
'src/crypto/crypto_util.cc',
405-
'src/crypto/crypto_clienthello.cc',
406405
'src/crypto/crypto_dh.cc',
407406
'src/crypto/crypto_hash.cc',
408407
'src/crypto/crypto_keys.cc',
@@ -412,7 +411,6 @@
412411
'src/crypto/crypto_x509.cc',
413412
'src/crypto/crypto_argon2.h',
414413
'src/crypto/crypto_bio.h',
415-
'src/crypto/crypto_clienthello-inl.h',
416414
'src/crypto/crypto_dh.h',
417415
'src/crypto/crypto_hmac.h',
418416
'src/crypto/crypto_kmac.h',
@@ -428,7 +426,6 @@
428426
'src/crypto/crypto_keygen.h',
429427
'src/crypto/crypto_scrypt.h',
430428
'src/crypto/crypto_tls.h',
431-
'src/crypto/crypto_clienthello.h',
432429
'src/crypto/crypto_context.h',
433430
'src/crypto/crypto_ec.h',
434431
'src/crypto/crypto_pqc.h',
@@ -442,7 +439,6 @@
442439
'src/node_crypto.h',
443440
],
444441
'node_cctest_openssl_sources': [
445-
'test/cctest/test_crypto_clienthello.cc',
446442
'test/cctest/test_node_crypto.cc',
447443
'test/cctest/test_node_crypto_env.cc',
448444
],
@@ -1213,54 +1209,6 @@
12131209
}],
12141210
],
12151211
}, # fuzz_env
1216-
{ # fuzz_ClientHelloParser.cc
1217-
'target_name': 'fuzz_ClientHelloParser',
1218-
'type': 'executable',
1219-
'dependencies': [
1220-
'<(node_lib_target_name)',
1221-
],
1222-
'includes': [
1223-
'node.gypi'
1224-
],
1225-
'include_dirs': [
1226-
'src',
1227-
'tools/msvs/genfiles',
1228-
'deps/v8/include',
1229-
'deps/cares/include',
1230-
'deps/uv/include',
1231-
'test/cctest',
1232-
],
1233-
'defines': [
1234-
'NODE_ARCH="<(target_arch)"',
1235-
'NODE_PLATFORM="<(OS)"',
1236-
'NODE_WANT_INTERNALS=1',
1237-
],
1238-
'sources': [
1239-
'test/fuzzers/fuzz_ClientHelloParser.cc',
1240-
],
1241-
'conditions': [
1242-
[ 'node_shared_hdr_histogram=="false"', {
1243-
'dependencies': [
1244-
'deps/histogram/histogram.gyp:histogram',
1245-
],
1246-
}],
1247-
[ 'node_shared_uvwasi=="false"', {
1248-
'dependencies': [ 'deps/uvwasi/uvwasi.gyp:uvwasi' ],
1249-
'include_dirs': [ 'deps/uvwasi/include' ],
1250-
}],
1251-
['OS=="linux" or OS=="openharmony"', {
1252-
'ldflags': [ '-fsanitize=fuzzer' ]
1253-
}],
1254-
# Ensure that ossfuzz flag has been set and that we are on Linux
1255-
[ 'OS not in "linux openharmony" or ossfuzz!="true"', {
1256-
'type': 'none',
1257-
}],
1258-
# Avoid excessive LTO
1259-
['enable_lto=="true"', {
1260-
'ldflags': [ '-fno-lto' ],
1261-
}],
1262-
],
1263-
}, # fuzz_ClientHelloParser.cc
12641212
{ # fuzz_strings
12651213
'target_name': 'fuzz_strings',
12661214
'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)