From 06ba40a8535f84e9fe34b597b7778162f1f67fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 18 May 2022 14:03:31 +0200 Subject: [PATCH] src: fix static analysis warning and use smart ptr 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: https://github.com/nodejs/node/pull/37990 PR-URL: https://github.com/nodejs/node/pull/43117 Reviewed-By: Darshan Sen Reviewed-By: Minwoo Jung --- src/crypto/crypto_common.cc | 33 ++++++++++++++++++--------------- src/crypto/crypto_common.h | 2 +- src/crypto/crypto_context.cc | 12 ++++++------ 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 5db9b1aa56941b..09b93d40c277ed 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -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 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( @@ -390,12 +395,12 @@ MaybeLocal GetLastIssuedCert( Environment* const env) { Local 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 ca_info; - MaybeLocal maybe_ca_info = X509ToObject(env, ca); + MaybeLocal maybe_ca_info = X509ToObject(env, ca.get()); if (!maybe_ca_info.ToLocal(&ca_info)) return MaybeLocal(); @@ -403,16 +408,14 @@ MaybeLocal GetLastIssuedCert( return MaybeLocal(); 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(issuer_chain); } diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h index 5a6869c6ca8845..7843f8acd1c5d5 100644 --- a/src/crypto/crypto_common.h +++ b/src/crypto/crypto_common.h @@ -25,7 +25,7 @@ struct StackOfXASN1Deleter { }; using StackOfASN1 = std::unique_ptr; -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, diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index f8125ab706b733..e2291f72b6622f 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -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)