tls: throw on invalid ALPNProtocols instead of aborting - #65076
tls: throw on invalid ALPNProtocols instead of aborting#65076sankalpsthakur wants to merge 5 commits into
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Thanks for looking at this @sankalpsthakur! I agree it'd be good to fix, but I think there's a different approach that would cover the server case too and be a bit cleaner.
We do also need a test covering this, either as a new test or extending any existing validation tests we have here.
Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076
|
Updated per review: validation now in lib/tls.js convertALPNProtocols (covers client and server, fails early), C++ defense kept, added test covering [''] and malformed buffers (Buffer.from([0]), [2,0x61], [1,0x61,0x62]). Ready for re-review. |
Fixes nodejs#65069 tls.connect() with malformed ALPNProtocols (empty string, invalid wire format buffers) hits CHECK_EQ(0, SSL_set_alpn_protos(...)) and aborts the process with SIGABRT. Replace the hard abort with THROW_ERR_INVALID_ARG_VALUE so invalid input throws a recoverable JavaScript exception. Assisted-by: Codex Signed-off-by: sankalpsthakur <sankalp@example.com>
Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076
Signed-off-by: Sankalp Thakur <sankalp@example.com>
dab0602 to
af6b21f
Compare
|
Thanks for the feedback. I moved validation into convertALPNProtocols so both client and server fail early before reaching OpenSSL, and kept the C++ check as a defense in depth. Empty string protocols now throw via convertProtocols and wire format buffers are validated for empty, zero length and truncated cases. Added test coverage for the repro cases from #65069. I fixed the lint and formatting issues as well and rebased onto latest main. |
Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076
3 participants
Fixes #65069
tls.connect() with malformed ALPNProtocols (e.g. [''] or invalid wire format buffers like Buffer.from([0])) aborts the process with SIGABRT. The failure is CHECK_EQ(0, SSL_set_alpn_protos(...)) in TLSWrap::SetALPNProtocols when OpenSSL rejects the malformed list.
This change replaces the hard abort with THROW_ERR_INVALID_ARG_VALUE so invalid input throws a recoverable JavaScript exception. Valid inputs are unchanged; e.g. ['h2','http/1.1'] and wire buffers produced by convertProtocols continue to work.
Repro: node -e "require('node:tls').connect({ host: '127.0.0.1', port: 9999, ALPNProtocols: [''] })"
Before: SIGABRT exit 134
After: throws ERR_INVALID_ARG_VALUE
Assisted-by: Codex