diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index ec2379f15b55be..ed1aea868d8385 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -48,13 +48,18 @@ static constexpr int kX509NameFlagsMultiline = XN_FLAG_SEP_MULTILINE | XN_FLAG_FN_SN; -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( @@ -386,12 +391,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(); @@ -399,16 +404,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 e0956395e91c18..96ca981aec9e44 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)