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

tls: wrap SNICallback invocation in try/catch · nodejs/node@4bfda30 · GitHub

/ node Public

Commit 4bfda30

Browse files
mcollina
authored and
RafaelGSS
committed
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: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2026-21637
1 parent 45b55dc commit 4bfda30

2 files changed

Lines changed: 107 additions & 13 deletions

File tree

‎lib/internal/tls/wrap.js‎

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

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

218-
if (err)
219-
return owner.destroy(err);
219+
if (err)
220+
return owner.destroy(err);
220221

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

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

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

232236

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

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

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

0 commit comments

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