From 4e37ac424f4d60a005ed2b29a0a87d2eb8970145 Mon Sep 17 00:00:00 2001 From: sankalpsthakur Date: Thu, 6 Aug 2026 15:10:32 +0530 Subject: [PATCH 1/5] tls: throw on invalid ALPNProtocols instead of aborting Fixes #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 --- src/crypto/crypto_tls.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 8ef74aee2d0e..b5f6bf708bdc 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1702,7 +1702,10 @@ void TLSWrap::SetALPNProtocols(const FunctionCallbackInfo& args) { ArrayBufferViewContents protos(args[0].As()); SSL* ssl = w->ssl_.get(); if (w->is_client()) { - CHECK_EQ(0, SSL_set_alpn_protos(ssl, protos.data(), protos.length())); + if (SSL_set_alpn_protos(ssl, protos.data(), protos.length()) != 0) { + return THROW_ERR_INVALID_ARG_VALUE( + env, "Invalid ALPNProtocols value"); + } } else { w->alpn_protos_ = std::vector( protos.data(), protos.data() + protos.length()); From 9b9eacaefe7a79a47c7808411ecfd3c0d9eb33ad Mon Sep 17 00:00:00 2001 From: sankalpsthakur Date: Thu, 6 Aug 2026 16:17:51 +0530 Subject: [PATCH 2/5] tls: validate ALPNProtocols in JS and cover server case Address review feedback on #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: https://github.com/nodejs/node/pull/65076 --- lib/tls.js | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/tls.js b/lib/tls.js index 296f6189da17..7ee4f98f792b 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -253,6 +253,10 @@ function convertProtocols(protocols) { const lens = new Array(protocols.length); const buff = Buffer.allocUnsafe(protocols.reduce((p, c, i) => { const len = Buffer.byteLength(c); + if (len === 0) { + throw new ERR_INVALID_ARG_VALUE(`protocols[${i}]`, c, + 'must be a non-empty string'); + } if (len > 255) { throw new ERR_OUT_OF_RANGE('The byte length of the protocol at index ' + `${i} exceeds the maximum length.`, '<= 255', len, true); @@ -271,18 +275,44 @@ function convertProtocols(protocols) { return buff; } +function validateALPNBuffer(buffer) { + // Wire format: sequence of where len is 1 byte (1-255) and + // exactly len bytes follow, no trailing bytes, no zero-length entries. + let offset = 0; + if (buffer.length === 0) { + throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, + 'must not be empty'); + } + while (offset < buffer.length) { + const len = buffer[offset]; + if (len === 0) { + throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, + 'must not contain zero-length protocol'); + } + if (offset + 1 + len > buffer.length) { + throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, + 'contains truncated protocol'); + } + offset += 1 + len; + } +} + exports.convertALPNProtocols = function convertALPNProtocols(protocols, out) { // If protocols is Array - translate it into buffer if (ArrayIsArray(protocols)) { out.ALPNProtocols = convertProtocols(protocols); } else if (isUint8Array(protocols)) { // Copy new buffer not to be modified by user. - out.ALPNProtocols = Buffer.from(protocols); + const buf = Buffer.from(protocols); + validateALPNBuffer(buf); + out.ALPNProtocols = buf; } else if (isArrayBufferView(protocols)) { - out.ALPNProtocols = Buffer.from(protocols.buffer.slice( + const buf = Buffer.from(protocols.buffer.slice( protocols.byteOffset, protocols.byteOffset + protocols.byteLength, )); + validateALPNBuffer(buf); + out.ALPNProtocols = buf; } }; From aac57132f6db4fbb52e9e24b156636a52111f44f Mon Sep 17 00:00:00 2001 From: sankalpsthakur Date: Thu, 6 Aug 2026 16:18:04 +0530 Subject: [PATCH 3/5] test: add validation for invalid ALPNProtocols Refs: https://github.com/nodejs/node/issues/65069 PR-URL: https://github.com/nodejs/node/pull/65076 --- .../test-tls-alpn-protocols-validation.js | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 test/parallel/test-tls-alpn-protocols-validation.js diff --git a/test/parallel/test-tls-alpn-protocols-validation.js b/test/parallel/test-tls-alpn-protocols-validation.js new file mode 100644 index 000000000000..2ad500950f51 --- /dev/null +++ b/test/parallel/test-tls-alpn-protocols-validation.js @@ -0,0 +1,65 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); + +// Array with empty string should throw (client and server paths via convertALPNProtocols) +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols([''], out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Array with empty string mixed +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols(['h2', ''], out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Buffer wire format with leading zero length +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols(Buffer.from([0]), out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Buffer truncated (claims 2 bytes but only 1 follows) +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols(Buffer.from([2, 0x61]), out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Buffer with trailing byte (len says 1 but 2 bytes remain -> trailing) +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols(Buffer.from([1, 0x61, 0x62]), out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Empty buffer should throw +assert.throws(() => { + const out = {}; + tls.convertALPNProtocols(Buffer.alloc(0), out); +}, { + code: 'ERR_INVALID_ARG_VALUE', +}); + +// Valid inputs should not throw +assert.doesNotThrow(() => { + const out = {}; + tls.convertALPNProtocols(['h2', 'http/1.1'], out); +}); +assert.doesNotThrow(() => { + const out = {}; + tls.convertALPNProtocols(Buffer.from([2, 0x61, 0x62, 8, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31]), out); +}); From af6b21fc7849be58cb21d558169891a948f44ef2 Mon Sep 17 00:00:00 2001 From: Sankalp Thakur Date: Thu, 6 Aug 2026 18:12:38 +0530 Subject: [PATCH 4/5] fixup: address review: lint and format Signed-off-by: Sankalp Thakur --- lib/tls.js | 8 ++++---- src/crypto/crypto_tls.cc | 3 +-- .../test-tls-alpn-protocols-validation.js | 16 +++++++++------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/tls.js b/lib/tls.js index 7ee4f98f792b..29420d960842 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -255,7 +255,7 @@ function convertProtocols(protocols) { const len = Buffer.byteLength(c); if (len === 0) { throw new ERR_INVALID_ARG_VALUE(`protocols[${i}]`, c, - 'must be a non-empty string'); + 'must be a non-empty string'); } if (len > 255) { throw new ERR_OUT_OF_RANGE('The byte length of the protocol at index ' + @@ -281,17 +281,17 @@ function validateALPNBuffer(buffer) { let offset = 0; if (buffer.length === 0) { throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, - 'must not be empty'); + 'must not be empty'); } while (offset < buffer.length) { const len = buffer[offset]; if (len === 0) { throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, - 'must not contain zero-length protocol'); + 'must not contain zero-length protocol'); } if (offset + 1 + len > buffer.length) { throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, - 'contains truncated protocol'); + 'contains truncated protocol'); } offset += 1 + len; } diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index b5f6bf708bdc..ed2ce7ea3aef 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1703,8 +1703,7 @@ void TLSWrap::SetALPNProtocols(const FunctionCallbackInfo& args) { SSL* ssl = w->ssl_.get(); if (w->is_client()) { if (SSL_set_alpn_protos(ssl, protos.data(), protos.length()) != 0) { - return THROW_ERR_INVALID_ARG_VALUE( - env, "Invalid ALPNProtocols value"); + return THROW_ERR_INVALID_ARG_VALUE(env, "Invalid ALPNProtocols value"); } } else { w->alpn_protos_ = std::vector( diff --git a/test/parallel/test-tls-alpn-protocols-validation.js b/test/parallel/test-tls-alpn-protocols-validation.js index 2ad500950f51..5244e64afc9f 100644 --- a/test/parallel/test-tls-alpn-protocols-validation.js +++ b/test/parallel/test-tls-alpn-protocols-validation.js @@ -38,10 +38,10 @@ assert.throws(() => { code: 'ERR_INVALID_ARG_VALUE', }); -// Buffer with trailing byte (len says 1 but 2 bytes remain -> trailing) +// Buffer with trailing invalid byte assert.throws(() => { const out = {}; - tls.convertALPNProtocols(Buffer.from([1, 0x61, 0x62]), out); + tls.convertALPNProtocols(Buffer.from([1, 0x61, 0x62, 0x62]), out); }, { code: 'ERR_INVALID_ARG_VALUE', }); @@ -55,11 +55,13 @@ assert.throws(() => { }); // Valid inputs should not throw -assert.doesNotThrow(() => { +{ const out = {}; tls.convertALPNProtocols(['h2', 'http/1.1'], out); -}); -assert.doesNotThrow(() => { +} +{ const out = {}; - tls.convertALPNProtocols(Buffer.from([2, 0x61, 0x62, 8, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31]), out); -}); + tls.convertALPNProtocols(Buffer.from([ + 2, 0x61, 0x62, 8, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31, + ]), out); +} From 6e497bbdbcbb3cebe30b1e289cd0d03124b5967c Mon Sep 17 00:00:00 2001 From: Sankalp Thakur Date: Fri, 7 Aug 2026 23:09:13 +0530 Subject: [PATCH 5/5] tls: allow empty ALPNProtocols and keep C++ CHECK_EQ 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: https://github.com/nodejs/node/pull/65076 --- lib/tls.js | 5 +--- src/crypto/crypto_tls.cc | 4 +-- .../test-tls-alpn-protocols-validation.js | 30 +++++++++++++++---- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/tls.js b/lib/tls.js index 29420d960842..940adef7efaf 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -278,11 +278,8 @@ function convertProtocols(protocols) { function validateALPNBuffer(buffer) { // Wire format: sequence of where len is 1 byte (1-255) and // exactly len bytes follow, no trailing bytes, no zero-length entries. + // Empty buffer is allowed and means skip ALPN (same as []). let offset = 0; - if (buffer.length === 0) { - throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer, - 'must not be empty'); - } while (offset < buffer.length) { const len = buffer[offset]; if (len === 0) { diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index ed2ce7ea3aef..8ef74aee2d0e 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1702,9 +1702,7 @@ void TLSWrap::SetALPNProtocols(const FunctionCallbackInfo& args) { ArrayBufferViewContents protos(args[0].As()); SSL* ssl = w->ssl_.get(); if (w->is_client()) { - if (SSL_set_alpn_protos(ssl, protos.data(), protos.length()) != 0) { - return THROW_ERR_INVALID_ARG_VALUE(env, "Invalid ALPNProtocols value"); - } + CHECK_EQ(0, SSL_set_alpn_protos(ssl, protos.data(), protos.length())); } else { w->alpn_protos_ = std::vector( protos.data(), protos.data() + protos.length()); diff --git a/test/parallel/test-tls-alpn-protocols-validation.js b/test/parallel/test-tls-alpn-protocols-validation.js index 5244e64afc9f..2a93cca891ce 100644 --- a/test/parallel/test-tls-alpn-protocols-validation.js +++ b/test/parallel/test-tls-alpn-protocols-validation.js @@ -6,7 +6,7 @@ if (!common.hasCrypto) const assert = require('assert'); const tls = require('tls'); -// Array with empty string should throw (client and server paths via convertALPNProtocols) +// Array with empty string should throw (zero-length protocol entry) assert.throws(() => { const out = {}; tls.convertALPNProtocols([''], out); @@ -46,22 +46,40 @@ assert.throws(() => { code: 'ERR_INVALID_ARG_VALUE', }); -// Empty buffer should throw -assert.throws(() => { +// Empty array means skip ALPN (allowed) +{ + const out = {}; + tls.convertALPNProtocols([], out); + assert.ok(Buffer.isBuffer(out.ALPNProtocols)); + assert.strictEqual(out.ALPNProtocols.length, 0); +} + +// Empty buffer means skip ALPN (allowed; same as []) +{ const out = {}; tls.convertALPNProtocols(Buffer.alloc(0), out); -}, { - code: 'ERR_INVALID_ARG_VALUE', -}); + assert.ok(Buffer.isBuffer(out.ALPNProtocols)); + assert.strictEqual(out.ALPNProtocols.length, 0); +} + +// Empty Uint8Array means skip ALPN +{ + const out = {}; + tls.convertALPNProtocols(new Uint8Array(0), out); + assert.ok(Buffer.isBuffer(out.ALPNProtocols)); + assert.strictEqual(out.ALPNProtocols.length, 0); +} // Valid inputs should not throw { const out = {}; tls.convertALPNProtocols(['h2', 'http/1.1'], out); + assert.ok(out.ALPNProtocols.length > 0); } { const out = {}; tls.convertALPNProtocols(Buffer.from([ 2, 0x61, 0x62, 8, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31, ]), out); + assert.strictEqual(out.ALPNProtocols.length, 12); }