tls: read the peer certificate chain without consuming it · nodejs/node@6a3d8a6 · GitHub
Skip to content

Commit 6a3d8a6

Browse files
tgiesaduh95
tgies
authored and
aduh95
committed
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 <tgies@tgies.net> PR-URL: #65602 Fixes: #65579 Refs: #64677 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent ebe2ac2 commit 6a3d8a6

6 files changed

Lines changed: 148 additions & 25 deletions

File tree

‎lib/internal/tls/wrap.js‎

Lines changed: 1 addition & 1 deletion

‎src/crypto/crypto_tls.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,6 +1765,13 @@ void TLSWrap::GetPeerX509Certificate(const FunctionCallbackInfo<Value>& args) {
17651765
args.GetReturnValue().Set(ret);
17661766
}
17671767

1768+
void TLSWrap::HasPeerCertificate(const FunctionCallbackInfo<Value>& args) {
1769+
TLSWrap* w;
1770+
ASSIGN_OR_RETURN_UNWRAP(&w, args.This());
1771+
bool has_peer_cert = static_cast<bool>(X509Pointer::PeerFrom(w->ssl_));
1772+
args.GetReturnValue().Set(has_peer_cert);
1773+
}
1774+
17681775
void TLSWrap::GetCertificate(const FunctionCallbackInfo<Value>& args) {
17691776
TLSWrap* w;
17701777
ASSIGN_OR_RETURN_UNWRAP(&w, args.This());
@@ -2288,6 +2295,8 @@ void TLSWrap::Initialize(
22882295
isolate, t, "getPeerCertificate", GetPeerCertificate);
22892296
SetProtoMethodNoSideEffect(
22902297
isolate, t, "getPeerX509Certificate", GetPeerX509Certificate);
2298+
SetProtoMethodNoSideEffect(
2299+
isolate, t, "hasPeerCertificate", HasPeerCertificate);
22912300
SetProtoMethodNoSideEffect(isolate, t, "getPeerFinished", GetPeerFinished);
22922301
SetProtoMethodNoSideEffect(isolate, t, "getProtocol", GetProtocol);
22932302
SetProtoMethodNoSideEffect(isolate, t, "getSession", GetSession);
@@ -2347,6 +2356,7 @@ void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
23472356
registry->Register(GetFinished);
23482357
registry->Register(GetPeerCertificate);
23492358
registry->Register(GetPeerX509Certificate);
2359+
registry->Register(HasPeerCertificate);
23502360
registry->Register(GetPeerFinished);
23512361
registry->Register(GetProtocol);
23522362
registry->Register(GetSession);

‎src/crypto/crypto_tls.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,8 @@ class TLSWrap : public AsyncWrap,
229229
const v8::FunctionCallbackInfo<v8::Value>& args);
230230
static void GetPeerX509Certificate(
231231
const v8::FunctionCallbackInfo<v8::Value>& args);
232+
static void HasPeerCertificate(
233+
const v8::FunctionCallbackInfo<v8::Value>& args);
232234
static void GetPeerFinished(const v8::FunctionCallbackInfo<v8::Value>& args);
233235
static void GetProtocol(const v8::FunctionCallbackInfo<v8::Value>& args);
234236
static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);

‎src/crypto/crypto_x509.cc‎

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -920,14 +920,22 @@ void X509Certificate::IsX509Certificate(
920920

921921
MaybeLocal<Object> X509Certificate::New(Environment* env,
922922
X509Pointer cert,
923-
STACK_OF(X509) * issuer_chain) {
923+
const STACK_OF(X509) * issuer_chain) {
924924
std::shared_ptr<ManagedX509> mcert(new ManagedX509(std::move(cert)));
925925
return New(env, std::move(mcert), issuer_chain);
926926
}
927927

928928
MaybeLocal<Object> X509Certificate::New(Environment* env,
929929
std::shared_ptr<ManagedX509> cert,
930-
STACK_OF(X509) * issuer_chain) {
930+
const STACK_OF(X509) * issuer_chain) {
931+
return NewWithIssuers(env, std::move(cert), issuer_chain, 0);
932+
}
933+
934+
MaybeLocal<Object> X509Certificate::NewWithIssuers(
935+
Environment* env,
936+
std::shared_ptr<ManagedX509> cert,
937+
const STACK_OF(X509) * issuer_chain,
938+
int start) {
931939
EscapableHandleScope scope(env->isolate());
932940
Local<Object> obj;
933941
if (!GetConstructorTemplate(env)
@@ -937,20 +945,23 @@ MaybeLocal<Object> X509Certificate::New(Environment* env,
937945
return MaybeLocal<Object>();
938946
}
939947

940-
Local<Object> issuer_chain_obj;
941-
if (issuer_chain != nullptr && sk_X509_num(issuer_chain)) {
942-
X509Pointer cert(X509_dup(sk_X509_value(issuer_chain, 0)));
943-
sk_X509_delete(issuer_chain, 0);
944-
auto maybeObj =
945-
sk_X509_num(issuer_chain)
946-
? X509Certificate::New(env, std::move(cert), issuer_chain)
947-
: X509Certificate::New(env, std::move(cert));
948-
if (!maybeObj.ToLocal(&issuer_chain_obj)) [[unlikely]] {
948+
Local<Object> issuer;
949+
if (issuer_chain != nullptr && start < sk_X509_num(issuer_chain)) {
950+
X509Pointer issuer_cert =
951+
X509View(sk_X509_value(issuer_chain, start)).clone();
952+
if (!issuer_cert) [[unlikely]] {
953+
return MaybeLocal<Object>();
954+
}
955+
if (!NewWithIssuers(env,
956+
std::make_shared<ManagedX509>(std::move(issuer_cert)),
957+
issuer_chain,
958+
start + 1)
959+
.ToLocal(&issuer)) [[unlikely]] {
949960
return MaybeLocal<Object>();
950961
}
951962
}
952963

953-
new X509Certificate(env, obj, std::move(cert), issuer_chain_obj);
964+
new X509Certificate(env, obj, std::move(cert), issuer);
954965
return scope.Escape(obj);
955966
}
956967

@@ -967,23 +978,29 @@ MaybeLocal<Object> X509Certificate::GetPeerCert(Environment* env,
967978
GetPeerCertificateFlag flag) {
968979
ClearErrorOnReturn clear_error_on_return;
969980

981+
// The peer chain is owned by the SSL session and must not be modified. Its
982+
// first entry is the peer certificate on the client but not on the server.
983+
const STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(ssl.get());
984+
int issuers_start = 0;
985+
970986
X509Pointer cert;
971987
if ((flag & GetPeerCertificateFlag::SERVER) ==
972988
GetPeerCertificateFlag::SERVER) {
973989
cert = X509Pointer::PeerFrom(ssl);
974990
}
975-
976-
STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(ssl.get());
977-
if (!cert && (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0))
978-
return MaybeLocal<Object>();
979-
980-
if (!cert) [[unlikely]] {
981-
cert.reset(sk_X509_value(ssl_certs, 0));
982-
sk_X509_delete(ssl_certs, 0);
991+
if (!cert) {
992+
if (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0)
993+
return MaybeLocal<Object>();
994+
cert = X509View(sk_X509_value(ssl_certs, 0)).clone();
995+
if (!cert) [[unlikely]]
996+
return MaybeLocal<Object>();
997+
issuers_start = 1;
983998
}
984999

985-
return sk_X509_num(ssl_certs) ? New(env, std::move(cert), ssl_certs)
986-
: New(env, std::move(cert));
1000+
return NewWithIssuers(env,
1001+
std::make_shared<ManagedX509>(std::move(cert)),
1002+
ssl_certs,
1003+
issuers_start);
9871004
}
9881005

9891006
v8::MaybeLocal<v8::Value> X509Certificate::toObject(Environment* env) {

‎src/crypto/crypto_x509.h‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,12 @@ class X509Certificate final : public BaseObject {
6060
static v8::MaybeLocal<v8::Object> New(
6161
Environment* env,
6262
ncrypto::X509Pointer cert,
63-
STACK_OF(X509) * issuer_chain = nullptr);
63+
const STACK_OF(X509) * issuer_chain = nullptr);
6464

6565
static v8::MaybeLocal<v8::Object> New(
6666
Environment* env,
6767
std::shared_ptr<ManagedX509> cert,
68-
STACK_OF(X509)* issuer_chain = nullptr);
68+
const STACK_OF(X509) * issuer_chain = nullptr);
6969

7070
static v8::MaybeLocal<v8::Object> GetCert(Environment* env,
7171
const ncrypto::SSLPointer& ssl);
@@ -121,6 +121,13 @@ class X509Certificate final : public BaseObject {
121121
std::shared_ptr<ManagedX509> cert,
122122
v8::Local<v8::Object> issuer_chain = v8::Local<v8::Object>());
123123

124+
// Like New(), but reads the issuer chain from issuer_chain[start] upward.
125+
static v8::MaybeLocal<v8::Object> NewWithIssuers(
126+
Environment* env,
127+
std::shared_ptr<ManagedX509> cert,
128+
const STACK_OF(X509) * issuer_chain,
129+
int start);
130+
124131
std::shared_ptr<ManagedX509> cert_;
125132
BaseObjectPtr<X509Certificate> issuer_cert_;
126133
};

‎test/parallel/test-tls-peer-certificate-repeated-reads.js‎

Lines changed: 87 additions & 0 deletions

0 commit comments

Comments
 (0)