From 2f17c526746ef1f56e0f3c3367a8ac79a181f5df Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 2 Mar 2018 16:55:32 +0100 Subject: [PATCH] src: use std::unique_ptr for STACK_OF(X509) Convert manual memory management of STACK_OF(X509) instances to std::unique_ptr with a custom deleter. Note the tasteful application of std::move() to indicate a function that consumes its argument. PR-URL: https://github.com/nodejs/node/pull/19087 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Daniel Bevenius --- src/node_crypto.cc | 98 +++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 54 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 589c8bae161416..595097e92a4424 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -43,12 +43,14 @@ // StartComAndWoSignData.inc #include "StartComAndWoSignData.inc" -#include #include #include // INT_MAX #include #include #include + +#include +#include #include #define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val, prefix) \ @@ -115,6 +117,12 @@ using v8::String; using v8::Value; +struct StackOfX509Deleter { + void operator()(STACK_OF(X509)* p) const { sk_X509_pop_free(p, X509_free); } +}; + +using StackOfX509 = std::unique_ptr; + #if OPENSSL_VERSION_NUMBER < 0x10100000L static void RSA_get0_key(const RSA* r, const BIGNUM** n, const BIGNUM** e, const BIGNUM** d) { @@ -839,17 +847,15 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, int ret = 0; unsigned long err = 0; // NOLINT(runtime/int) - // Read extra certs - STACK_OF(X509)* extra_certs = sk_X509_new_null(); - if (extra_certs == nullptr) { + StackOfX509 extra_certs(sk_X509_new_null()); + if (!extra_certs) goto done; - } while ((extra = PEM_read_bio_X509(in, nullptr, NoPasswordCallback, nullptr))) { - if (sk_X509_push(extra_certs, extra)) + if (sk_X509_push(extra_certs.get(), extra)) continue; // Failure, free all certs @@ -867,13 +873,11 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, goto done; } - ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs, cert, issuer); + ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs.get(), cert, issuer); if (!ret) goto done; done: - if (extra_certs != nullptr) - sk_X509_pop_free(extra_certs, X509_free); if (extra != nullptr) X509_free(extra); if (x != nullptr) @@ -2004,14 +2008,14 @@ static Local X509ToObject(Environment* env, X509* cert) { static Local AddIssuerChainToObject(X509** cert, Local object, - STACK_OF(X509)* const peer_certs, + StackOfX509 peer_certs, Environment* const env) { Local context = env->isolate()->GetCurrentContext(); - *cert = sk_X509_delete(peer_certs, 0); + *cert = sk_X509_delete(peer_certs.get(), 0); for (;;) { int i; - for (i = 0; i < sk_X509_num(peer_certs); i++) { - X509* ca = sk_X509_value(peer_certs, i); + for (i = 0; i < sk_X509_num(peer_certs.get()); i++) { + X509* ca = sk_X509_value(peer_certs.get(), i); if (X509_check_issued(ca, *cert) != X509_V_OK) continue; @@ -2023,41 +2027,31 @@ static Local AddIssuerChainToObject(X509** cert, X509_free(*cert); // Delete cert and continue aggregating issuers. - *cert = sk_X509_delete(peer_certs, i); + *cert = sk_X509_delete(peer_certs.get(), i); break; } // Issuer not found, break out of the loop. - if (i == sk_X509_num(peer_certs)) + if (i == sk_X509_num(peer_certs.get())) break; } - sk_X509_pop_free(peer_certs, X509_free); return object; } -static bool CloneSSLCerts(X509** cert, - const STACK_OF(X509)* const ssl_certs, - STACK_OF(X509)** peer_certs) { - *peer_certs = sk_X509_new(nullptr); - bool result = true; +static StackOfX509 CloneSSLCerts(X509** cert, + const STACK_OF(X509)* const ssl_certs) { + StackOfX509 peer_certs(sk_X509_new(nullptr)); if (*cert != nullptr) - sk_X509_push(*peer_certs, *cert); + sk_X509_push(peer_certs.get(), *cert); for (int i = 0; i < sk_X509_num(ssl_certs); i++) { *cert = X509_dup(sk_X509_value(ssl_certs, i)); - if (*cert == nullptr) { - result = false; - break; - } - if (!sk_X509_push(*peer_certs, *cert)) { - result = false; - break; - } - } - if (!result) { - sk_X509_pop_free(*peer_certs, X509_free); + if (*cert == nullptr) + return StackOfX509(); + if (!sk_X509_push(peer_certs.get(), *cert)) + return StackOfX509(); } - return result; + return peer_certs; } @@ -2091,7 +2085,6 @@ void SSLWrap::GetPeerCertificate( Base* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder()); Environment* env = w->ssl_env(); - Local context = env->context(); ClearErrorOnReturn clear_error_on_return; @@ -2103,23 +2096,25 @@ void SSLWrap::GetPeerCertificate( // contains the `peer_certificate`, but on server it doesn't. X509* cert = w->is_server() ? SSL_get_peer_certificate(w->ssl_) : nullptr; STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(w->ssl_); - STACK_OF(X509)* peer_certs = nullptr; if (cert == nullptr && (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0)) goto done; // Short result requested. if (args.Length() < 1 || !args[0]->IsTrue()) { - result = X509ToObject(env, - cert == nullptr ? sk_X509_value(ssl_certs, 0) : cert); + X509* target_cert = cert; + if (target_cert == nullptr) + target_cert = sk_X509_value(ssl_certs, 0); + result = X509ToObject(env, target_cert); goto done; } - if (CloneSSLCerts(&cert, ssl_certs, &peer_certs)) { + if (auto peer_certs = CloneSSLCerts(&cert, ssl_certs)) { // First and main certificate. - cert = sk_X509_value(peer_certs, 0); + cert = sk_X509_value(peer_certs.get(), 0); result = X509ToObject(env, cert); - issuer_chain = AddIssuerChainToObject(&cert, result, peer_certs, env); + issuer_chain = + AddIssuerChainToObject(&cert, result, std::move(peer_certs), env); issuer_chain = GetLastIssuedCert(&cert, w->ssl_, issuer_chain, env); // Last certificate should be self-signed. if (X509_check_issued(cert, cert) == X509_V_OK) @@ -3184,25 +3179,23 @@ inline CheckResult CheckWhitelistedServerCert(X509_STORE_CTX* ctx) { unsigned char hash[CNNIC_WHITELIST_HASH_LEN]; unsigned int hashlen = CNNIC_WHITELIST_HASH_LEN; - STACK_OF(X509)* chain = X509_STORE_CTX_get1_chain(ctx); - CHECK_NE(chain, nullptr); - CHECK_GT(sk_X509_num(chain), 0); + StackOfX509 chain(X509_STORE_CTX_get1_chain(ctx)); + CHECK(chain); + CHECK_GT(sk_X509_num(chain.get()), 0); // Take the last cert as root at the first time. - X509* root_cert = sk_X509_value(chain, sk_X509_num(chain)-1); + X509* root_cert = sk_X509_value(chain.get(), sk_X509_num(chain.get())-1); X509_NAME* root_name = X509_get_subject_name(root_cert); if (!IsSelfSigned(root_cert)) { - root_cert = FindRoot(chain); + root_cert = FindRoot(chain.get()); CHECK_NE(root_cert, nullptr); root_name = X509_get_subject_name(root_cert); } - X509* leaf_cert = sk_X509_value(chain, 0); - if (!CheckStartComOrWoSign(root_name, leaf_cert)) { - sk_X509_pop_free(chain, X509_free); + X509* leaf_cert = sk_X509_value(chain.get(), 0); + if (!CheckStartComOrWoSign(root_name, leaf_cert)) return CHECK_CERT_REVOKED; - } // When the cert is issued from either CNNNIC ROOT CA or CNNNIC EV // ROOT CA, check a hash of its leaf cert if it is in the whitelist. @@ -3215,13 +3208,10 @@ inline CheckResult CheckWhitelistedServerCert(X509_STORE_CTX* ctx) { void* result = bsearch(hash, WhitelistedCNNICHashes, arraysize(WhitelistedCNNICHashes), CNNIC_WHITELIST_HASH_LEN, compar); - if (result == nullptr) { - sk_X509_pop_free(chain, X509_free); + if (result == nullptr) return CHECK_CERT_REVOKED; - } } - sk_X509_pop_free(chain, X509_free); return CHECK_OK; }