Skip to content

Commit ed11b57

Browse files
mcollinaaduh95
mcollina
authored and
aduh95
committed
tls: apply the CA of SNI contexts without a certificate
`CertCbDone()` only installed an SNI context's CA store and client CA list when `setSniContext()` succeeded, which requires the context to have a certificate. A context added with `server.addContext(host, { ca })` was therefore ignored for client certificate verification, and clients for that host were verified against the default context's CA instead. Apply the SNI context's CA whenever it configured one through `addCACert()`, even without a certificate. A context with neither a certificate nor its own CA still keeps the default trust store, rather than switching to the bundled root certificates. Also report a failure of `setSniContext()` instead of silently continuing with the default credentials. Refs: https://hackerone.com/reports/4103553 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#1020 Reviewed-By: Tim Perry <pimterry@gmail.com> CVE-ID: CVE-2026-123867
1 parent 6c5b08e commit ed11b57

4 files changed

Lines changed: 107 additions & 5 deletions

File tree

‎src/crypto/crypto_context.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1675,6 +1675,7 @@ inline void SecureContext::Reset() {
16751675
ctx_.reset();
16761676
cert_.reset();
16771677
issuer_.reset();
1678+
has_own_ca_ = false;
16781679
}
16791680

16801681
SecureContext::~SecureContext() {
@@ -1964,6 +1965,7 @@ void SecureContext::SetAllowPartialTrustChain(
19641965
void SecureContext::SetCACert(const BIOPointer& bio) {
19651966
ClearErrorOnReturn clear_error_on_return;
19661967
AddCACertificates(env(), ctx_.get(), bio, &own_cert_store_cache_);
1968+
has_own_ca_ = true;
19671969
}
19681970

19691971
void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {

‎src/crypto/crypto_context.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ class SecureContext final : public BaseObject {
5252
static SecureContext* Create(Environment* env);
5353

5454
const ncrypto::SSLCtxPointer& ctx() const { return ctx_; }
55+
// True if CA certificates were added with addCACert(), as opposed to the
56+
// context relying on the default root store.
57+
bool has_own_ca() const { return has_own_ca_; }
5558

5659
// Non-const ctx() that allows for non-default initialization of
5760
// the SecureContext.
@@ -188,6 +191,7 @@ class SecureContext final : public BaseObject {
188191
ncrypto::X509Pointer issuer_;
189192
// Non-owning cache for SSL_CTX_get_cert_store(ctx_.get())
190193
X509_STORE* own_cert_store_cache_ = nullptr;
194+
bool has_own_ca_ = false;
191195

192196
unsigned char ticket_key_name_[16];
193197
unsigned char ticket_key_aes_[16];

‎src/crypto/crypto_tls.cc‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1776,11 +1776,19 @@ void TLSWrap::CertCbDone(const FunctionCallbackInfo<Value>& args) {
17761776
// Store the SNI context for later use.
17771777
w->sni_context_ = BaseObjectPtr<SecureContext>(sc);
17781778

1779-
if (w->ssl_.setSniContext(w->sni_context_->ctx()) && !w->SetCACerts(sc)) {
1780-
// Not clear why sometimes we throw error, and sometimes we call
1781-
// onerror(). Both cause .destroy(), but onerror does a bit more.
1782-
unsigned long err = ERR_get_error(); // NOLINT(runtime/int)
1783-
return ThrowCryptoError(env, err, "CertCbDone");
1779+
// An SNI context without a certificate keeps the default context's
1780+
// certificate, but a `ca` it configured must still replace the default
1781+
// trust store for client certificates. A context with neither keeps the
1782+
// default context's trust store.
1783+
const bool has_cert = SSL_CTX_get0_certificate(sc->ctx().get()) != nullptr;
1784+
if (has_cert || sc->has_own_ca()) {
1785+
if ((has_cert && !w->ssl_.setSniContext(w->sni_context_->ctx())) ||
1786+
!w->SetCACerts(sc)) {
1787+
// Not clear why sometimes we throw error, and sometimes we call
1788+
// onerror(). Both cause .destroy(), but onerror does a bit more.
1789+
unsigned long err = ERR_get_error(); // NOLINT(runtime/int)
1790+
return ThrowCryptoError(env, err, "CertCbDone");
1791+
}
17841792
}
17851793
// setSniContext copies the cert via SSL_use_certificate which does not
17861794
// carry over pre-compressed certificate data (comp_cert). If the new

‎test/parallel/test-tls-sni-context-ca-only.js‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
// An SNI context that sets `ca` but no certificate must still replace the
8+
// default trust store for client certificates on that host. A context that
9+
// sets neither keeps the default trust store.
10+
11+
const assert = require('assert');
12+
const tls = require('tls');
13+
const fixtures = require('../common/fixtures');
14+
15+
const serverCreds = {
16+
key: fixtures.readKey('agent2-key.pem'),
17+
cert: fixtures.readKey('agent2-cert.pem'),
18+
};
19+
const ca1 = fixtures.readKey('ca1-cert.pem');
20+
const ca2 = fixtures.readKey('ca2-cert.pem');
21+
const clients = {
22+
agent1: { // Issued by CA1.
23+
key: fixtures.readKey('agent1-key.pem'),
24+
cert: fixtures.readKey('agent1-cert.pem'),
25+
},
26+
agent3: { // Issued by CA2.
27+
key: fixtures.readKey('agent3-key.pem'),
28+
cert: fixtures.readKey('agent3-cert.pem'),
29+
},
30+
};
31+
32+
const cases = [
33+
{ name: 'ca only', context: { ca: ca2 }, accepted: 'agent3' },
34+
{ name: 'cert, key and ca', context: { ...serverCreds, ca: ca2 },
35+
accepted: 'agent3' },
36+
{ name: 'no ca or cert', context: { ciphers: 'DEFAULT' },
37+
accepted: 'agent1' },
38+
];
39+
40+
function connect(port, client, version) {
41+
return new Promise((resolve) => {
42+
const socket = tls.connect({
43+
port,
44+
servername: 'protected.example',
45+
...clients[client],
46+
rejectUnauthorized: false,
47+
minVersion: version,
48+
maxVersion: version,
49+
});
50+
let data = '';
51+
socket.setEncoding('utf8');
52+
socket.on('data', (chunk) => data += chunk);
53+
socket.on('error', () => {});
54+
socket.on('close', () => resolve(data));
55+
});
56+
}
57+
58+
async function test(version, { name, context, accepted }) {
59+
const server = tls.createServer({
60+
...serverCreds,
61+
ca: ca1,
62+
requestCert: true,
63+
rejectUnauthorized: true,
64+
minVersion: version,
65+
maxVersion: version,
66+
}, common.mustCall((socket) => {
67+
assert.strictEqual(socket.servername, 'protected.example');
68+
socket.end(socket.getPeerCertificate().subject.CN);
69+
}));
70+
server.on('tlsClientError', () => {});
71+
server.addContext('protected.example', context);
72+
await new Promise((resolve) => server.listen(0, resolve));
73+
74+
const { port } = server.address();
75+
for (const client of Object.keys(clients)) {
76+
const expected = client === accepted ? client : '';
77+
assert.strictEqual(await connect(port, client, version), expected,
78+
`${version}, ${name}: ${client}`);
79+
}
80+
server.close();
81+
}
82+
83+
(async () => {
84+
for (const version of ['TLSv1.2', 'TLSv1.3']) {
85+
for (const c of cases)
86+
await test(version, c);
87+
}
88+
})().then(common.mustCall());

0 commit comments

Comments
 (0)