tls: drop hand-rolled TLS client hello parser · nodejs/node@d75957f · GitHub
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

‎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

‎src/crypto/crypto_clienthello-inl.h‎

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

0 commit comments

Comments
 (0)