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

tls: throw on invalid ALPNProtocols instead of aborting · nodejs/node@2a0b1ec · GitHub

/ node Public

Commit 2a0b1ec

Browse files
sankalpsthakur
authored and
aduh95
committed
tls: throw on invalid ALPNProtocols instead of aborting
tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent ac3c412 commit 2a0b1ec

3 files changed

Lines changed: 122 additions & 8 deletions

File tree

‎lib/tls.js‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,10 @@ function convertProtocols(protocols) {
251251
const lens = new Array(protocols.length);
252252
const buff = Buffer.allocUnsafe(protocols.reduce((p, c, i) => {
253253
const len = Buffer.byteLength(c);
254+
if (len === 0) {
255+
throw new ERR_INVALID_ARG_VALUE(`protocols[${i}]`, c,
256+
'must be a non-empty string');
257+
}
254258
if (len > 255) {
255259
throw new ERR_OUT_OF_RANGE('The byte length of the protocol at index ' +
256260
`${i} exceeds the maximum length.`, '<= 255', len, true);
@@ -269,18 +273,41 @@ function convertProtocols(protocols) {
269273
return buff;
270274
}
271275

276+
function validateALPNBuffer(buffer) {
277+
// Wire format: sequence of <len><proto> where len is 1 byte (1-255) and
278+
// exactly len bytes follow, no trailing bytes, no zero-length entries.
279+
// Empty buffer is allowed and means skip ALPN (same as []).
280+
let offset = 0;
281+
while (offset < buffer.length) {
282+
const len = buffer[offset];
283+
if (len === 0) {
284+
throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer,
285+
'must not contain zero-length protocol');
286+
}
287+
if (offset + 1 + len > buffer.length) {
288+
throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer,
289+
'contains truncated protocol');
290+
}
291+
offset += 1 + len;
292+
}
293+
}
294+
272295
exports.convertALPNProtocols = function convertALPNProtocols(protocols, out) {
273296
// If protocols is Array - translate it into buffer
274297
if (ArrayIsArray(protocols)) {
275298
out.ALPNProtocols = convertProtocols(protocols);
276299
} else if (isUint8Array(protocols)) {
277300
// Copy new buffer not to be modified by user.
278-
out.ALPNProtocols = Buffer.from(protocols);
301+
const buf = Buffer.from(protocols);
302+
validateALPNBuffer(buf);
303+
out.ALPNProtocols = buf;
279304
} else if (isArrayBufferView(protocols)) {
280-
out.ALPNProtocols = Buffer.from(protocols.buffer.slice(
305+
const buf = Buffer.from(protocols.buffer.slice(
281306
protocols.byteOffset,
282307
protocols.byteOffset + protocols.byteLength,
283308
));
309+
validateALPNBuffer(buf);
310+
out.ALPNProtocols = buf;
284311
}
285312
};
286313

‎test/parallel/test-tls-alpn-protocols-validation.js‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
6+
const assert = require('assert');
7+
const tls = require('tls');
8+
9+
// Array with empty string should throw (zero-length protocol entry)
10+
assert.throws(() => {
11+
const out = {};
12+
tls.convertALPNProtocols([''], out);
13+
}, {
14+
code: 'ERR_INVALID_ARG_VALUE',
15+
});
16+
17+
// Array with empty string mixed
18+
assert.throws(() => {
19+
const out = {};
20+
tls.convertALPNProtocols(['h2', ''], out);
21+
}, {
22+
code: 'ERR_INVALID_ARG_VALUE',
23+
});
24+
25+
// Buffer wire format with leading zero length
26+
assert.throws(() => {
27+
const out = {};
28+
tls.convertALPNProtocols(Buffer.from([0]), out);
29+
}, {
30+
code: 'ERR_INVALID_ARG_VALUE',
31+
});
32+
33+
// Buffer truncated (claims 2 bytes but only 1 follows)
34+
assert.throws(() => {
35+
const out = {};
36+
tls.convertALPNProtocols(Buffer.from([2, 0x61]), out);
37+
}, {
38+
code: 'ERR_INVALID_ARG_VALUE',
39+
});
40+
41+
// Buffer with trailing invalid byte
42+
assert.throws(() => {
43+
const out = {};
44+
tls.convertALPNProtocols(Buffer.from([1, 0x61, 0x62, 0x62]), out);
45+
}, {
46+
code: 'ERR_INVALID_ARG_VALUE',
47+
});
48+
49+
// Empty array means skip ALPN (allowed)
50+
{
51+
const out = {};
52+
tls.convertALPNProtocols([], out);
53+
assert.ok(Buffer.isBuffer(out.ALPNProtocols));
54+
assert.strictEqual(out.ALPNProtocols.length, 0);
55+
}
56+
57+
// Empty buffer means skip ALPN (allowed; same as [])
58+
{
59+
const out = {};
60+
tls.convertALPNProtocols(Buffer.alloc(0), out);
61+
assert.ok(Buffer.isBuffer(out.ALPNProtocols));
62+
assert.strictEqual(out.ALPNProtocols.length, 0);
63+
}
64+
65+
// Empty Uint8Array means skip ALPN
66+
{
67+
const out = {};
68+
tls.convertALPNProtocols(new Uint8Array(0), out);
69+
assert.ok(Buffer.isBuffer(out.ALPNProtocols));
70+
assert.strictEqual(out.ALPNProtocols.length, 0);
71+
}
72+
73+
// Valid inputs should not throw
74+
{
75+
const out = {};
76+
tls.convertALPNProtocols(['h2', 'http/1.1'], out);
77+
assert.ok(out.ALPNProtocols.length > 0);
78+
}
79+
{
80+
const out = {};
81+
tls.convertALPNProtocols(Buffer.from([
82+
2, 0x61, 0x62, 8, 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31,
83+
]), out);
84+
assert.strictEqual(out.ALPNProtocols.length, 12);
85+
}

‎test/parallel/test-tls-basic-validations.js‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,17 +81,19 @@ assert.throws(() => tls.createServer({ ticketKeys: Buffer.alloc(0) }), {
8181
});
8282

8383
{
84-
const buffer = Buffer.from('abcd');
84+
const buffer = Buffer.from([3, 0x61, 0x62, 0x63]);
8585
const out = {};
8686
tls.convertALPNProtocols(buffer, out);
87-
out.ALPNProtocols.write('efgh');
88-
assert(buffer.equals(Buffer.from('abcd')));
89-
assert(out.ALPNProtocols.equals(Buffer.from('efgh')));
87+
out.ALPNProtocols.write('def', 1);
88+
assert(buffer.equals(Buffer.from([3, 0x61, 0x62, 0x63])));
89+
assert(out.ALPNProtocols.equals(Buffer.from([3, 0x64, 0x65, 0x66])));
9090
}
9191

9292
{
93-
const arrayBufferViewStr = 'abcd';
94-
const inputBuffer = Buffer.from(arrayBufferViewStr.repeat(8), 'utf8');
93+
const inputBuffer = Buffer.concat([
94+
Buffer.from([31]),
95+
Buffer.alloc(31, 0x61),
96+
]);
9597
for (const expectView of common.getArrayBufferViews(inputBuffer)) {
9698
const out = {};
9799
const expected = Buffer.from(expectView.buffer.slice(),

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL