Skip to content

Commit cc3f294

Browse files
mcollinaaduh95
authored andcommitted
tls: wrap SNICallback invocation in try/catch
Wrap the owner._SNICallback() invocation in loadSNI() with try/catch to route exceptions through owner.destroy() instead of letting them become uncaught exceptions. This completes the fix from CVE-2026-21637 which added try/catch protection to callALPNCallback, onPskServerCallback, and onPskClientCallback but missed loadSNI(). Without this fix, a remote unauthenticated attacker can crash any Node.js TLS server whose SNICallback may throw on unexpected input by sending a single TLS ClientHello with a crafted server_name value. Fixes: https://hackerone.com/reports/3556769 Refs: https://hackerone.com/reports/3473882 CVE-ID: CVE-2026-21637 PR-URL: nodejs-private/node-private#839 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2026-21637
1 parent 0083071 commit cc3f294

File tree

2 files changed

+107
-13
lines changed

2 files changed

+107
-13
lines changed

lib/_tls_wrap.js

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -209,23 +209,27 @@ function loadSNI(info) {
209209
return requestOCSP(owner, info);
210210

211211
let once = false;
212-
owner._SNICallback(servername, (err, context) => {
213-
if (once)
214-
return owner.destroy(new ERR_MULTIPLE_CALLBACK());
215-
once = true;
212+
try {
213+
owner._SNICallback(servername, (err, context) => {
214+
if (once)
215+
return owner.destroy(new ERR_MULTIPLE_CALLBACK());
216+
once = true;
216217

217-
if (err)
218-
return owner.destroy(err);
218+
if (err)
219+
return owner.destroy(err);
219220

220-
if (owner._handle === null)
221-
return owner.destroy(new ERR_SOCKET_CLOSED());
221+
if (owner._handle === null)
222+
return owner.destroy(new ERR_SOCKET_CLOSED());
222223

223-
// TODO(indutny): eventually disallow raw `SecureContext`
224-
if (context)
225-
owner._handle.sni_context = context.context || context;
224+
// TODO(indutny): eventually disallow raw `SecureContext`
225+
if (context)
226+
owner._handle.sni_context = context.context || context;
226227

227-
requestOCSP(owner, info);
228-
});
228+
requestOCSP(owner, info);
229+
});
230+
} catch (err) {
231+
owner.destroy(err);
232+
}
229233
}
230234

231235

test/parallel/test-tls-psk-alpn-callback-exception-handling.js

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,4 +335,94 @@ describe('TLS callback exception handling', () => {
335335

336336
await promise;
337337
});
338+
339+
// Test 7: SNI callback throwing should emit tlsClientError
340+
it('SNICallback throwing emits tlsClientError', async (t) => {
341+
const server = tls.createServer({
342+
key: fixtures.readKey('agent2-key.pem'),
343+
cert: fixtures.readKey('agent2-cert.pem'),
344+
SNICallback: (servername, cb) => {
345+
throw new Error('Intentional SNI callback error');
346+
},
347+
});
348+
349+
t.after(() => server.close());
350+
351+
const { promise, resolve, reject } = createTestPromise();
352+
353+
server.on('tlsClientError', common.mustCall((err, socket) => {
354+
try {
355+
assert.ok(err instanceof Error);
356+
assert.strictEqual(err.message, 'Intentional SNI callback error');
357+
socket.destroy();
358+
resolve();
359+
} catch (e) {
360+
reject(e);
361+
}
362+
}));
363+
364+
server.on('secureConnection', () => {
365+
reject(new Error('secureConnection should not fire'));
366+
});
367+
368+
await new Promise((res) => server.listen(0, res));
369+
370+
const client = tls.connect({
371+
port: server.address().port,
372+
host: '127.0.0.1',
373+
servername: 'evil.attacker.com',
374+
rejectUnauthorized: false,
375+
});
376+
377+
client.on('error', () => {});
378+
379+
await promise;
380+
});
381+
382+
// Test 8: SNI callback with validation error should emit tlsClientError
383+
it('SNICallback validation error emits tlsClientError', async (t) => {
384+
const server = tls.createServer({
385+
key: fixtures.readKey('agent2-key.pem'),
386+
cert: fixtures.readKey('agent2-cert.pem'),
387+
SNICallback: (servername, cb) => {
388+
// Simulate common developer pattern: throw on unknown servername
389+
if (servername !== 'expected.example.com') {
390+
throw new Error(`Unknown servername: ${servername}`);
391+
}
392+
cb(null, null);
393+
},
394+
});
395+
396+
t.after(() => server.close());
397+
398+
const { promise, resolve, reject } = createTestPromise();
399+
400+
server.on('tlsClientError', common.mustCall((err, socket) => {
401+
try {
402+
assert.ok(err instanceof Error);
403+
assert.ok(err.message.includes('Unknown servername'));
404+
socket.destroy();
405+
resolve();
406+
} catch (e) {
407+
reject(e);
408+
}
409+
}));
410+
411+
server.on('secureConnection', () => {
412+
reject(new Error('secureConnection should not fire'));
413+
});
414+
415+
await new Promise((res) => server.listen(0, res));
416+
417+
const client = tls.connect({
418+
port: server.address().port,
419+
host: '127.0.0.1',
420+
servername: 'unexpected.domain.com',
421+
rejectUnauthorized: false,
422+
});
423+
424+
client.on('error', () => {});
425+
426+
await promise;
427+
});
338428
});

0 commit comments

Comments
 (0)