Skip to content

Commit

Permalink
src: fix static analysis warning and use smart ptr
Browse files Browse the repository at this point in the history
Coverity issues a warning about `value_before_reset == ca`, where
value_before_reset is a pointer that may not be valid. While comparing
the pointer itself should still work, we should really be using smart
pointers here so that this particular check can be simplified without
running into a memory leak.

Refactor SSL_CTX_get_issuer to return a smart pointer and update the
call sites accordingly. Note that we might have to change that in the
future once we improve error handling throughout crypto/tls.

Refs: nodejs#37990
  • Loading branch information
tniessen committed May 16, 2022
1 parent aa14ff9 commit 3bd3bba
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 22 deletions.
33 changes: 18 additions & 15 deletions src/crypto/crypto_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,18 @@ static constexpr int kX509NameFlagsRFC2253WithinUtf8JSON =
~ASN1_STRFLGS_ESC_MSB &
~ASN1_STRFLGS_ESC_CTRL;

bool SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) {
X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert) {
X509_STORE* store = SSL_CTX_get_cert_store(ctx);
DeleteFnPtr<X509_STORE_CTX, X509_STORE_CTX_free> store_ctx(
X509_STORE_CTX_new());
return store_ctx.get() != nullptr &&
X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 &&
X509_STORE_CTX_get1_issuer(issuer, store_ctx.get(), cert) == 1;
X509Pointer result;
X509* issuer;
if (store_ctx.get() != nullptr &&
X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 &&
X509_STORE_CTX_get1_issuer(&issuer, store_ctx.get(), cert) == 1) {
result.reset(issuer);
}
return result;
}

void LogSecret(
Expand Down Expand Up @@ -390,29 +395,27 @@ MaybeLocal<Object> GetLastIssuedCert(
Environment* const env) {
Local<Context> context = env->isolate()->GetCurrentContext();
while (X509_check_issued(cert->get(), cert->get()) != X509_V_OK) {
X509* ca;
if (SSL_CTX_get_issuer(SSL_get_SSL_CTX(ssl.get()), cert->get(), &ca) <= 0)
X509Pointer ca;
if (!(ca = SSL_CTX_get_issuer(SSL_get_SSL_CTX(ssl.get()), cert->get())))
break;

Local<Object> ca_info;
MaybeLocal<Object> maybe_ca_info = X509ToObject(env, ca);
MaybeLocal<Object> maybe_ca_info = X509ToObject(env, ca.get());
if (!maybe_ca_info.ToLocal(&ca_info))
return MaybeLocal<Object>();

if (!Set<Object>(context, issuer_chain, env->issuercert_string(), ca_info))
return MaybeLocal<Object>();
issuer_chain = ca_info;

// Take the value of cert->get() before the call to cert->reset()
// in order to compare it to ca after and provide a way to exit this loop
// in case it gets stuck.
X509* value_before_reset = cert->get();
// For self-signed certificates whose keyUsage field does not include
// keyCertSign, X509_check_issued() will return false. Avoid going into an
// infinite loop by checking if SSL_CTX_get_issuer() returned the same
// certificate.
if (cert->get() == ca.get()) break;

// Delete previous cert and continue aggregating issuers.
cert->reset(ca);

if (value_before_reset == ca)
break;
*cert = std::move(ca);
}
return MaybeLocal<Object>(issuer_chain);
}
Expand Down
2 changes: 1 addition & 1 deletion src/crypto/crypto_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ struct StackOfXASN1Deleter {
};
using StackOfASN1 = std::unique_ptr<STACK_OF(ASN1_OBJECT), StackOfXASN1Deleter>;

bool SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer);
X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert);

void LogSecret(
const SSLPointer& ssl,
Expand Down
12 changes: 6 additions & 6 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,21 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
// Try getting issuer from a cert store
if (ret) {
if (issuer == nullptr) {
ret = SSL_CTX_get_issuer(ctx, x.get(), &issuer);
ret = ret < 0 ? 0 : 1;
// TODO(tniessen): SSL_CTX_get_issuer does not allow the caller to
// distinguish between a failed operation and an empty result. Fix that
// and then handle the potential error properly here (set ret to 0).
*issuer_ = SSL_CTX_get_issuer(ctx, x.get());
// NOTE: get_cert_store doesn't increment reference count,
// no need to free `store`
} else {
// Increment issuer reference count
issuer = X509_dup(issuer);
if (issuer == nullptr) {
issuer_->reset(X509_dup(issuer));
if (!*issuer_) {
ret = 0;
}
}
}

issuer_->reset(issuer);

if (ret && x != nullptr) {
cert->reset(X509_dup(x.get()));
if (!*cert)
Expand Down

0 comments on commit 3bd3bba

Please sign in to comment.