FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: drop hand-rolled TLS client hello parser · nodejs/node@8f4ee7e · GitHub

/ node Public

Commit 8f4ee7e

Browse files
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 2dd7d4c commit 8f4ee7e

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
@@ -406,7 +406,6 @@
406406
'src/crypto/crypto_rsa.cc',
407407
'src/crypto/crypto_spkac.cc',
408408
'src/crypto/crypto_util.cc',
409-
'src/crypto/crypto_clienthello.cc',
410409
'src/crypto/crypto_dh.cc',
411410
'src/crypto/crypto_hash.cc',
412411
'src/crypto/crypto_keys.cc',
@@ -416,7 +415,6 @@
416415
'src/crypto/crypto_x509.cc',
417416
'src/crypto/crypto_argon2.h',
418417
'src/crypto/crypto_bio.h',
419-
'src/crypto/crypto_clienthello-inl.h',
420418
'src/crypto/crypto_dh.h',
421419
'src/crypto/crypto_hmac.h',
422420
'src/crypto/crypto_kmac.h',
@@ -432,7 +430,6 @@
432430
'src/crypto/crypto_keygen.h',
433431
'src/crypto/crypto_scrypt.h',
434432
'src/crypto/crypto_tls.h',
435-
'src/crypto/crypto_clienthello.h',
436433
'src/crypto/crypto_context.h',
437434
'src/crypto/crypto_ec.h',
438435
'src/crypto/crypto_pqc.h',
@@ -462,7 +459,6 @@
462459
'src/tracing/trace_event_legacy.h',
463460
],
464461
'node_cctest_openssl_sources': [
465-
'test/cctest/test_crypto_clienthello.cc',
466462
'test/cctest/test_node_crypto.cc',
467463
'test/cctest/test_node_crypto_env.cc',
468464
],
@@ -1297,54 +1293,6 @@
12971293
}],
12981294
],
12991295
}, # fuzz_env
1300-
{ # fuzz_ClientHelloParser.cc
1301-
'target_name': 'fuzz_ClientHelloParser',
1302-
'type': 'executable',
1303-
'dependencies': [
1304-
'<(node_lib_target_name)',
1305-
],
1306-
'includes': [
1307-
'node.gypi'
1308-
],
1309-
'include_dirs': [
1310-
'src',
1311-
'tools/msvs/genfiles',
1312-
'deps/v8/include',
1313-
'deps/cares/include',
1314-
'deps/uv/include',
1315-
'test/cctest',
1316-
],
1317-
'defines': [
1318-
'NODE_ARCH="<(target_arch)"',
1319-
'NODE_PLATFORM="<(OS)"',
1320-
'NODE_WANT_INTERNALS=1',
1321-
],
1322-
'sources': [
1323-
'test/fuzzers/fuzz_ClientHelloParser.cc',
1324-
],
1325-
'conditions': [
1326-
[ 'node_shared_hdr_histogram=="false"', {
1327-
'dependencies': [
1328-
'deps/histogram/histogram.gyp:histogram',
1329-
],
1330-
}],
1331-
[ 'node_shared_uvwasi=="false"', {
1332-
'dependencies': [ 'deps/uvwasi/uvwasi.gyp:uvwasi' ],
1333-
'include_dirs': [ 'deps/uvwasi/include' ],
1334-
}],
1335-
['OS=="linux" or OS=="openharmony"', {
1336-
'ldflags': [ '-fsanitize=fuzzer' ]
1337-
}],
1338-
# Ensure that ossfuzz flag has been set and that we are on Linux
1339-
[ 'OS not in "linux openharmony" or ossfuzz!="true"', {
1340-
'type': 'none',
1341-
}],
1342-
# Avoid excessive LTO
1343-
['enable_lto=="true"', {
1344-
'ldflags': [ '-fno-lto' ],
1345-
}],
1346-
],
1347-
}, # fuzz_ClientHelloParser.cc
13481296
{ # fuzz_strings
13491297
'target_name': 'fuzz_strings',
13501298
'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)
Back | FazBrowse Home | New Git URL