From 3574e5577e866223908ad7856c7a3b5255f7397d Mon Sep 17 00:00:00 2001 From: Tony Gies Date: Thu, 27 Aug 2026 13:42:36 -0500 Subject: [PATCH] tls: read the peer certificate chain without consuming it X509Certificate::GetPeerCert() built the certificate objects by deleting entries from the stack returned by SSL_get_peer_cert_chain(), which is owned by the SSL session. The first read emptied it, so any later read by getPeerCertificate() or getPeerX509Certificate(), on either peer, saw a truncated chain or nothing. Copy each issuer with X509_dup instead and leave the session's stack untouched. onServerSocketSecure() only called getPeerX509Certificate() to check whether a peer certificate was present, building the whole chain on every server handshake; that is what first exposed the destructive read. Use a lightweight hasPeerCertificate() binding for the presence check. Signed-off-by: Tony Gies --- lib/internal/tls/wrap.js | 2 +- src/crypto/crypto_tls.cc | 10 +++ src/crypto/crypto_tls.h | 2 + src/crypto/crypto_x509.cc | 61 ++++++++----- src/crypto/crypto_x509.h | 11 ++- ...est-tls-peer-certificate-repeated-reads.js | 87 +++++++++++++++++++ 6 files changed, 148 insertions(+), 25 deletions(-) create mode 100644 test/parallel/test-tls-peer-certificate-repeated-reads.js diff --git a/lib/internal/tls/wrap.js b/lib/internal/tls/wrap.js index 1c6e0577ce3d..37fa7845843f 100644 --- a/lib/internal/tls/wrap.js +++ b/lib/internal/tls/wrap.js @@ -1322,7 +1322,7 @@ function onServerSocketSecure() { if (this._rejectUnauthorized) this.destroy(); - } else if (!this._handle.getPeerX509Certificate()) { + } else if (!this._handle.hasPeerCertificate()) { // Ncrypto reports X509_V_OK for TLS 1.3 resumption without a peer // certificate, as it uses PSKs. Require one to authorize the socket. this.authorizationError = 'UNABLE_TO_GET_ISSUER_CERT'; diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 5a531209e232..a13015530dd4 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1765,6 +1765,13 @@ void TLSWrap::GetPeerX509Certificate(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret); } +void TLSWrap::HasPeerCertificate(const FunctionCallbackInfo& args) { + TLSWrap* w; + ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); + bool has_peer_cert = static_cast(X509Pointer::PeerFrom(w->ssl_)); + args.GetReturnValue().Set(has_peer_cert); +} + void TLSWrap::GetCertificate(const FunctionCallbackInfo& args) { TLSWrap* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); @@ -2288,6 +2295,8 @@ void TLSWrap::Initialize( isolate, t, "getPeerCertificate", GetPeerCertificate); SetProtoMethodNoSideEffect( isolate, t, "getPeerX509Certificate", GetPeerX509Certificate); + SetProtoMethodNoSideEffect( + isolate, t, "hasPeerCertificate", HasPeerCertificate); SetProtoMethodNoSideEffect(isolate, t, "getPeerFinished", GetPeerFinished); SetProtoMethodNoSideEffect(isolate, t, "getProtocol", GetProtocol); SetProtoMethodNoSideEffect(isolate, t, "getSession", GetSession); @@ -2347,6 +2356,7 @@ void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetFinished); registry->Register(GetPeerCertificate); registry->Register(GetPeerX509Certificate); + registry->Register(HasPeerCertificate); registry->Register(GetPeerFinished); registry->Register(GetProtocol); registry->Register(GetSession); diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index 9a5f59ed472e..2d3ccef24fac 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -229,6 +229,8 @@ class TLSWrap : public AsyncWrap, const v8::FunctionCallbackInfo& args); static void GetPeerX509Certificate( const v8::FunctionCallbackInfo& args); + static void HasPeerCertificate( + const v8::FunctionCallbackInfo& args); static void GetPeerFinished(const v8::FunctionCallbackInfo& args); static void GetProtocol(const v8::FunctionCallbackInfo& args); static void GetServername(const v8::FunctionCallbackInfo& args); diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc index 05336f5e70d2..b81141e1a915 100644 --- a/src/crypto/crypto_x509.cc +++ b/src/crypto/crypto_x509.cc @@ -920,14 +920,22 @@ void X509Certificate::IsX509Certificate( MaybeLocal X509Certificate::New(Environment* env, X509Pointer cert, - STACK_OF(X509) * issuer_chain) { + const STACK_OF(X509) * issuer_chain) { std::shared_ptr mcert(new ManagedX509(std::move(cert))); return New(env, std::move(mcert), issuer_chain); } MaybeLocal X509Certificate::New(Environment* env, std::shared_ptr cert, - STACK_OF(X509) * issuer_chain) { + const STACK_OF(X509) * issuer_chain) { + return NewWithIssuers(env, std::move(cert), issuer_chain, 0); +} + +MaybeLocal X509Certificate::NewWithIssuers( + Environment* env, + std::shared_ptr cert, + const STACK_OF(X509) * issuer_chain, + int start) { EscapableHandleScope scope(env->isolate()); Local obj; if (!GetConstructorTemplate(env) @@ -937,20 +945,23 @@ MaybeLocal X509Certificate::New(Environment* env, return MaybeLocal(); } - Local issuer_chain_obj; - if (issuer_chain != nullptr && sk_X509_num(issuer_chain)) { - X509Pointer cert(X509_dup(sk_X509_value(issuer_chain, 0))); - sk_X509_delete(issuer_chain, 0); - auto maybeObj = - sk_X509_num(issuer_chain) - ? X509Certificate::New(env, std::move(cert), issuer_chain) - : X509Certificate::New(env, std::move(cert)); - if (!maybeObj.ToLocal(&issuer_chain_obj)) [[unlikely]] { + Local issuer; + if (issuer_chain != nullptr && start < sk_X509_num(issuer_chain)) { + X509Pointer issuer_cert = + X509View(sk_X509_value(issuer_chain, start)).clone(); + if (!issuer_cert) [[unlikely]] { + return MaybeLocal(); + } + if (!NewWithIssuers(env, + std::make_shared(std::move(issuer_cert)), + issuer_chain, + start + 1) + .ToLocal(&issuer)) [[unlikely]] { return MaybeLocal(); } } - new X509Certificate(env, obj, std::move(cert), issuer_chain_obj); + new X509Certificate(env, obj, std::move(cert), issuer); return scope.Escape(obj); } @@ -967,23 +978,29 @@ MaybeLocal X509Certificate::GetPeerCert(Environment* env, GetPeerCertificateFlag flag) { ClearErrorOnReturn clear_error_on_return; + // The peer chain is owned by the SSL session and must not be modified. Its + // first entry is the peer certificate on the client but not on the server. + const STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(ssl.get()); + int issuers_start = 0; + X509Pointer cert; if ((flag & GetPeerCertificateFlag::SERVER) == GetPeerCertificateFlag::SERVER) { cert = X509Pointer::PeerFrom(ssl); } - - STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(ssl.get()); - if (!cert && (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0)) - return MaybeLocal(); - - if (!cert) [[unlikely]] { - cert.reset(sk_X509_value(ssl_certs, 0)); - sk_X509_delete(ssl_certs, 0); + if (!cert) { + if (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0) + return MaybeLocal(); + cert = X509View(sk_X509_value(ssl_certs, 0)).clone(); + if (!cert) [[unlikely]] + return MaybeLocal(); + issuers_start = 1; } - return sk_X509_num(ssl_certs) ? New(env, std::move(cert), ssl_certs) - : New(env, std::move(cert)); + return NewWithIssuers(env, + std::make_shared(std::move(cert)), + ssl_certs, + issuers_start); } v8::MaybeLocal X509Certificate::toObject(Environment* env) { diff --git a/src/crypto/crypto_x509.h b/src/crypto/crypto_x509.h index dda2ed05ce65..a6f64d4f2d4f 100644 --- a/src/crypto/crypto_x509.h +++ b/src/crypto/crypto_x509.h @@ -60,12 +60,12 @@ class X509Certificate final : public BaseObject { static v8::MaybeLocal New( Environment* env, ncrypto::X509Pointer cert, - STACK_OF(X509) * issuer_chain = nullptr); + const STACK_OF(X509) * issuer_chain = nullptr); static v8::MaybeLocal New( Environment* env, std::shared_ptr cert, - STACK_OF(X509)* issuer_chain = nullptr); + const STACK_OF(X509) * issuer_chain = nullptr); static v8::MaybeLocal GetCert(Environment* env, const ncrypto::SSLPointer& ssl); @@ -121,6 +121,13 @@ class X509Certificate final : public BaseObject { std::shared_ptr cert, v8::Local issuer_chain = v8::Local()); + // Like New(), but reads the issuer chain from issuer_chain[start] upward. + static v8::MaybeLocal NewWithIssuers( + Environment* env, + std::shared_ptr cert, + const STACK_OF(X509) * issuer_chain, + int start); + std::shared_ptr cert_; BaseObjectPtr issuer_cert_; }; diff --git a/test/parallel/test-tls-peer-certificate-repeated-reads.js b/test/parallel/test-tls-peer-certificate-repeated-reads.js new file mode 100644 index 000000000000..a50d69687b9c --- /dev/null +++ b/test/parallel/test-tls-peer-certificate-repeated-reads.js @@ -0,0 +1,87 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +// Reading the peer certificate must not consume the chain held by the SSL +// session. getPeerX509Certificate() and getPeerCertificate() have to keep +// returning the full chain however often, and in whichever order, they are +// called on either end of the connection (see the #65579 regression, where an +// internal getPeerX509Certificate() call left getPeerCertificate(true) with +// only the leaf). On the server this also covers the peer certificate check +// that runs before 'secureConnection' is emitted. + +const assert = require('assert'); +const { X509Certificate } = require('crypto'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +// Each peer presents a distinct leaf -> intermediate -> root chain, so the +// certificate read back has two issuers above the leaf. +const serverChain = [ + 'leaf-from-intermediate-cert.pem', + 'intermediate-ca.pem', + 'fake-startcom-root-cert.pem', +].map((name) => fixtures.readKey(name)); +const clientChain = [ + 'agent10-cert.pem', + 'ca4-cert.pem', + 'ca2-cert.pem', +].map((name) => fixtures.readKey(name)); + +function fingerprints(chain) { + return chain.map((pem) => new X509Certificate(pem).fingerprint256); +} + +function checkPeerCertificate(socket, chain, side) { + assert.strictEqual(socket.authorized, true, side); + const [leaf, intermediate, root] = fingerprints(chain); + + // Two rounds, alternating the read methods, so a chain consumed by one read + // would be observed by the next. + for (let round = 0; round < 2; round++) { + const x509 = socket.getPeerX509Certificate(); + assert.strictEqual(x509.fingerprint256, leaf, side); + assert.strictEqual(x509.issuerCertificate.fingerprint256, + intermediate, side); + + const detailed = socket.getPeerCertificate(true); + assert.strictEqual(detailed.fingerprint256, leaf, side); + assert.strictEqual(detailed.issuerCertificate.fingerprint256, + intermediate, side); + assert.strictEqual( + detailed.issuerCertificate.issuerCertificate.fingerprint256, root, side); + + assert.strictEqual(socket.getPeerCertificate().fingerprint256, leaf, side); + } +} + +const server = tls.createServer({ + key: fixtures.readKey('leaf-from-intermediate-key.pem'), + cert: Buffer.concat(serverChain), + ca: clientChain[2], + requestCert: true, +}, common.mustCall((socket) => { + checkPeerCertificate(socket, clientChain, 'server'); + socket.end(); +})); + +server.listen(0, common.mustCall(() => { + const socket = tls.connect({ + port: server.address().port, + key: fixtures.readKey('agent10-key.pem'), + cert: Buffer.concat(clientChain), + ca: serverChain[2], + }, common.mustCall(() => { + checkPeerCertificate(socket, serverChain, 'client'); + + // The client receives the server chain verbatim, so its X509 certificate + // links all the way to the root, exercising the recursive issuer build + // more than one level deep. + const [, , root] = fingerprints(serverChain); + const x509 = socket.getPeerX509Certificate(); + assert.strictEqual(x509.issuerCertificate.issuerCertificate.fingerprint256, + root); + })); + socket.on('close', common.mustCall(() => server.close())); +}));