FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix static analysis warning and use smart ptr · nodejs/node@4823c5e · GitHub

/ node Public

Commit 4823c5e

Browse files
tniessen
authored and
juanarbol
committed
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: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
1 parent b8f102c commit 4823c5e

3 files changed

Lines changed: 30 additions & 22 deletions

File tree

‎src/crypto/crypto_common.cc‎

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,23 @@ static constexpr int kX509NameFlagsMultiline =
4848
XN_FLAG_SEP_MULTILINE |
4949
XN_FLAG_FN_SN;
5050

51-
bool SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) {
51+
static constexpr int kX509NameFlagsRFC2253WithinUtf8JSON =
52+
XN_FLAG_RFC2253 &
53+
~ASN1_STRFLGS_ESC_MSB &
54+
~ASN1_STRFLGS_ESC_CTRL;
55+
56+
X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert) {
5257
X509_STORE* store = SSL_CTX_get_cert_store(ctx);
5358
DeleteFnPtr<X509_STORE_CTX, X509_STORE_CTX_free> store_ctx(
5459
X509_STORE_CTX_new());
55-
return store_ctx.get() != nullptr &&
56-
X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 &&
57-
X509_STORE_CTX_get1_issuer(issuer, store_ctx.get(), cert) == 1;
60+
X509Pointer result;
61+
X509* issuer;
62+
if (store_ctx.get() != nullptr &&
63+
X509_STORE_CTX_init(store_ctx.get(), store, nullptr, nullptr) == 1 &&
64+
X509_STORE_CTX_get1_issuer(&issuer, store_ctx.get(), cert) == 1) {
65+
result.reset(issuer);
66+
}
67+
return result;
5868
}
5969

6070
void LogSecret(
@@ -386,29 +396,27 @@ MaybeLocal<Object> GetLastIssuedCert(
386396
Environment* const env) {
387397
Local<Context> context = env->isolate()->GetCurrentContext();
388398
while (X509_check_issued(cert->get(), cert->get()) != X509_V_OK) {
389-
X509* ca;
390-
if (SSL_CTX_get_issuer(SSL_get_SSL_CTX(ssl.get()), cert->get(), &ca) <= 0)
399+
X509Pointer ca;
400+
if (!(ca = SSL_CTX_get_issuer(SSL_get_SSL_CTX(ssl.get()), cert->get())))
391401
break;
392402

393403
Local<Object> ca_info;
394-
MaybeLocal<Object> maybe_ca_info = X509ToObject(env, ca);
404+
MaybeLocal<Object> maybe_ca_info = X509ToObject(env, ca.get());
395405
if (!maybe_ca_info.ToLocal(&ca_info))
396406
return MaybeLocal<Object>();
397407

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

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

407418
// Delete previous cert and continue aggregating issuers.
408-
cert->reset(ca);
409-
410-
if (value_before_reset == ca)
411-
break;
419+
*cert = std::move(ca);
412420
}
413421
return MaybeLocal<Object>(issuer_chain);
414422
}

‎src/crypto/crypto_common.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ struct StackOfXASN1Deleter {
2525
};
2626
using StackOfASN1 = std::unique_ptr<STACK_OF(ASN1_OBJECT), StackOfXASN1Deleter>;
2727

28-
bool SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer);
28+
X509Pointer SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert);
2929

3030
void LogSecret(
3131
const SSLPointer& ssl,

‎src/crypto/crypto_context.cc‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,21 +110,21 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
110110
// Try getting issuer from a cert store
111111
if (ret) {
112112
if (issuer == nullptr) {
113-
ret = SSL_CTX_get_issuer(ctx, x.get(), &issuer);
114-
ret = ret < 0 ? 0 : 1;
113+
// TODO(tniessen): SSL_CTX_get_issuer does not allow the caller to
114+
// distinguish between a failed operation and an empty result. Fix that
115+
// and then handle the potential error properly here (set ret to 0).
116+
*issuer_ = SSL_CTX_get_issuer(ctx, x.get());
115117
// NOTE: get_cert_store doesn't increment reference count,
116118
// no need to free `store`
117119
} else {
118120
// Increment issuer reference count
119-
issuer = X509_dup(issuer);
120-
if (issuer == nullptr) {
121+
issuer_->reset(X509_dup(issuer));
122+
if (!*issuer_) {
121123
ret = 0;
122124
}
123125
}
124126
}
125127

126-
issuer_->reset(issuer);
127-
128128
if (ret && x != nullptr) {
129129
cert->reset(X509_dup(x.get()));
130130
if (!*cert)

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL