Skip to content

Commit df8fbfb

Browse files
mcollinajuanarbol
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#819 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>
1 parent d6b6051 commit df8fbfb

File tree

2 files changed

+107
-13
lines changed

2 files changed

+107
-13
lines changed

lib/internal/tls/wrap.js

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

213213
let once = false;
214-
owner._SNICallback(servername, (err, context) => {
215-
if (once)
216-
return owner.destroy(new ERR_MULTIPLE_CALLBACK());
217-
once = true;
214+
try {
215+
owner._SNICallback(servername, (err, context) => {
216+
if (once)
217+
return owner.destroy(new ERR_MULTIPLE_CALLBACK());
218+
once = true;
218219

219-
if (err)
220-
return owner.destroy(err);
220+
if (err)
221+
return owner.destroy(err);
221222

222-
if (owner._handle === null)
223-
return owner.destroy(new ERR_SOCKET_CLOSED());
223+
if (owner._handle === null)
224+
return owner.destroy(new ERR_SOCKET_CLOSED());
224225

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

229-
requestOCSP(owner, info);
230-
});
230+
requestOCSP(owner, info);
231+
});
232+
} catch (err) {
233+
owner.destroy(err);
234+
}
231235
}
232236

233237

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

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

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

0 commit comments

Comments
 (0)