This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new b85ebd8709 TLS: Fix memory leaks in cert load and OCSP stapling
(#13318)
b85ebd8709 is described below
commit b85ebd8709ac16ca8cdae5372bab38fc7b90e148
Author: Mo Chen <[email protected]>
AuthorDate: Wed Jun 24 23:13:31 2026 -0500
TLS: Fix memory leaks in cert load and OCSP stapling (#13318)
Several error and early-return paths in the TLS server certificate loading
and OCSP stapling code omit cleanup and leak native objects. Most are gated
on misconfiguration (bad or unreadable cert files) or allocation failure, so
they accumulate slowly rather than on every handshake.
1. setClientCertCACerts leaked the X509_STORE when X509_STORE_load_locations
failed; the store is only adopted by SSL_set0_verify_cert_store on
success.
This fires per handshake on the sni.yaml verify_client path when the CA
file or directory is bad. It also now skips the load when X509_STORE_new
returns null, which would otherwise crash on allocation failure.
2. SSLNetVConnection::populate leaked the migrated SSL object when the base
populate failed: the source VC has already relinquished ownership, so the
SSL must be freed when this VC does not adopt it.
3. load_certs leaked the leaf certificate on the mid-chain failure returns
(bad intermediate chain, private key, or CA file). The leaf is now held
in a scoped_X509 so every path releases it.
4. _prep_ssl_ctx and SSLCreateServerContext leaked already-parsed leaves
when
load_certs_and_cross_reference_names failed partway, because the
X509_free
drain was skipped on the early return.
5. set_session_id_context leaked the client CA name stack on the digest
error
paths that run before ownership is handed to the SSL_CTX.
6. ssl_callback_ocsp_stapling ignored the OCSP-response set result and
leaked
the buffer on failure. On BoringSSL it now uses SSL_set_ocsp_response,
which copies the cached buffer and needs no per-handshake allocation; on
OpenSSL it checks SSL_set_tlsext_status_ocsp_resp and frees on failure.
7. stapling_refresh_response leaked the duplicated OCSP CERTID when
TS_OCSP_request_add0_id failed.
---
src/iocore/net/OCSPStapling.cc | 17 +++++++++-
src/iocore/net/SSLNetVConnection.cc | 4 +++
src/iocore/net/SSLUtils.cc | 63 ++++++++++++++++++++-----------------
3 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc
index bce21e159f..948bb4f834 100644
--- a/src/iocore/net/OCSPStapling.cc
+++ b/src/iocore/net/OCSPStapling.cc
@@ -1241,6 +1241,8 @@ stapling_refresh_response(certinfo *cinf,
TS_OCSP_RESPONSE **prsp)
goto err;
}
if (!TS_OCSP_request_add0_id(req, id)) {
+ // add0 only takes ownership of id on success.
+ TS_OCSP_CERTID_free(id);
goto err;
}
@@ -1427,6 +1429,11 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *)
Error("ssl_callback_ocsp_stapling: failed to get certificate status for
%s", cinf->certname);
return SSL_TLSEXT_ERR_NOACK;
} else {
+#ifdef OPENSSL_IS_BORINGSSL
+ // SSL_set_ocsp_response copies the response, so hand it the cached buffer
directly.
+ int set_ok = SSL_set_ocsp_response(ssl, cinf->resp_der, cinf->resp_derlen);
+ ink_mutex_release(&cinf->stapling_mutex);
+#else
unsigned char *p = static_cast<unsigned char
*>(OPENSSL_malloc(cinf->resp_derlen));
if (p == nullptr) {
ink_mutex_release(&cinf->stapling_mutex);
@@ -1435,7 +1442,15 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *)
}
memcpy(p, cinf->resp_der, cinf->resp_derlen);
ink_mutex_release(&cinf->stapling_mutex);
- SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
+ // Takes ownership of p and frees it on success; on failure it does not.
+ int set_ok = SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen);
+ if (set_ok == 0) {
+ OPENSSL_free(p);
+ }
+#endif
+ if (set_ok == 0) {
+ return SSL_TLSEXT_ERR_NOACK;
+ }
Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got
certificate status for %s", cinf->certname);
Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched,
cinf->uri);
return SSL_TLSEXT_ERR_OK;
diff --git a/src/iocore/net/SSLNetVConnection.cc
b/src/iocore/net/SSLNetVConnection.cc
index ed7bb03cd3..5ad9f97b56 100644
--- a/src/iocore/net/SSLNetVConnection.cc
+++ b/src/iocore/net/SSLNetVConnection.cc
@@ -1745,6 +1745,10 @@ SSLNetVConnection::populate(Connection &con,
Continuation *c, void *arg)
{
int retval = super::populate(con, c, arg);
if (retval != EVENT_DONE) {
+ // arg is the migrated SSL this VC would adopt below; release it since we
won't.
+ if (arg != nullptr) {
+ SSL_free(static_cast<SSL *>(arg));
+ }
return retval;
}
// Add in the SSL data
diff --git a/src/iocore/net/SSLUtils.cc b/src/iocore/net/SSLUtils.cc
index 5928042d41..7c7637affa 100644
--- a/src/iocore/net/SSLUtils.cc
+++ b/src/iocore/net/SSLUtils.cc
@@ -1165,8 +1165,11 @@ setClientCertCACerts(SSL *ssl, const char *file, const
char *dir)
#if TS_HAS_VERIFY_CERT_STORE
// The set0 version will take ownership of the X509_STORE object
X509_STORE *ctx = X509_STORE_new();
- if (X509_STORE_load_locations(ctx, file && file[0] != '\0' ? file :
nullptr, dir && dir[0] != '\0' ? dir : nullptr)) {
+ if (ctx != nullptr &&
+ X509_STORE_load_locations(ctx, file && file[0] != '\0' ? file :
nullptr, dir && dir[0] != '\0' ? dir : nullptr)) {
SSL_set0_verify_cert_store(ssl, ctx);
+ } else {
+ X509_STORE_free(ctx);
}
// SSL_set_client_CA_list takes ownership of the STACK_OF(X509) structure
@@ -1552,13 +1555,15 @@ SSLCreateServerContext(const SSLConfigParams *params,
const SSLMultiCertConfigPa
std::unordered_map<int, std::set<std::string>> unique_names;
SSLMultiCertConfigLoader::CertLoadData data;
SSLCertContextType cert_type;
- if (!loader.load_certs_and_cross_reference_names(cert_list, data, params,
sslMultiCertSettings, common_names, unique_names,
- &cert_type)) {
- return nullptr;
- }
+ bool loaded = loader.load_certs_and_cross_reference_names(cert_list, data,
params, sslMultiCertSettings, common_names,
+ unique_names,
&cert_type);
+ // cert_list may hold parsed leaves even when loading failed partway.
for (auto &i : cert_list) {
X509_free(i);
}
+ if (!loaded) {
+ return nullptr;
+ }
std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> ctx(nullptr,
&SSL_CTX_free);
@@ -1603,24 +1608,24 @@ SSLMultiCertConfigLoader::_prep_ssl_ctx(const
shared_SSLMultiCertConfigParams &
const SSLConfigParams *params = this->_params;
SSLCertContextType cert_type;
- if (!this->load_certs_and_cross_reference_names(cert_list, data, params,
sslMultCertSettings.get(), common_names, unique_names,
- &cert_type)) {
- return false;
- }
-
- int i = 0;
- bool good_certs = true;
- for (auto const &cert : cert_list) {
- const char *current_cert_name = data.cert_names_list[i].c_str();
- if (0 > SSLMultiCertConfigLoader::check_server_cert_now(cert,
current_cert_name)) {
- /* At this point, we know cert is bad, and we've already printed a
- descriptive reason as to why cert is bad to the log file */
- Dbg(this->_dbg_ctl(), "Marking certificate as NOT VALID: %s",
current_cert_name);
- good_certs = false;
+ bool good_certs = this->load_certs_and_cross_reference_names(cert_list,
data, params, sslMultCertSettings.get(), common_names,
+ unique_names,
&cert_type);
+
+ if (good_certs) {
+ int i = 0;
+ for (auto const &cert : cert_list) {
+ const char *current_cert_name = data.cert_names_list[i].c_str();
+ if (0 > SSLMultiCertConfigLoader::check_server_cert_now(cert,
current_cert_name)) {
+ /* At this point, we know cert is bad, and we've already printed a
+ descriptive reason as to why cert is bad to the log file */
+ Dbg(this->_dbg_ctl(), "Marking certificate as NOT VALID: %s",
current_cert_name);
+ good_certs = false;
+ }
+ i++;
}
- i++;
}
+ // cert_list may hold parsed leaves even when loading failed partway.
for (auto &cert : cert_list) {
X509_free(cert);
}
@@ -2304,10 +2309,10 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx,
const std::vector<std::string
keyPath.empty() ? "[empty key path]" : keyPath.c_str());
return false;
}
- scoped_BIO bio(BIO_new_mem_buf(secret_data.data(), secret_data.size()));
- X509 *cert = nullptr;
+ scoped_BIO bio(BIO_new_mem_buf(secret_data.data(), secret_data.size()));
+ scoped_X509 cert;
if (bio) {
- cert = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr);
+ cert.reset(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr));
} else {
SSLError("failed to create bio for certificate secret %s of length %ld",
data.cert_names_list[i].c_str(), secret_data.size());
return false;
@@ -2319,9 +2324,8 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx, const
std::vector<std::string
}
Dbg(dbg_ctl_ssl_load, "for ctx=%p, using certificate %s", ctx,
cert_names_list[i].c_str());
- if (!SSL_CTX_use_certificate(ctx, cert)) {
+ if (!SSL_CTX_use_certificate(ctx, cert.get())) {
SSLError("Failed to assign cert from %s to SSL_CTX",
cert_names_list[i].c_str());
- X509_free(cert);
return false;
}
@@ -2375,16 +2379,15 @@ SSLMultiCertConfigLoader::load_certs(SSL_CTX *ctx,
const std::vector<std::string
if (sslMultCertSettings->ocsp_response) {
const char *ocsp_response_name = data.ocsp_list[i].c_str();
std::string
completeOCSPResponsePath(Layout::relative_to(params->ssl_ocsp_response_path_only,
ocsp_response_name));
- if (!ssl_stapling_init_cert(ctx, cert, cert_names_list[i].c_str(),
completeOCSPResponsePath.c_str())) {
+ if (!ssl_stapling_init_cert(ctx, cert.get(),
cert_names_list[i].c_str(), completeOCSPResponsePath.c_str())) {
Warning("failed to configure SSL_CTX for OCSP Stapling info for
certificate at %s", cert_names_list[i].c_str());
}
} else {
- if (!ssl_stapling_init_cert(ctx, cert, cert_names_list[i].c_str(),
nullptr)) {
+ if (!ssl_stapling_init_cert(ctx, cert.get(),
cert_names_list[i].c_str(), nullptr)) {
Warning("failed to configure SSL_CTX for OCSP Stapling info for
certificate at %s", cert_names_list[i].c_str());
}
}
}
- X509_free(cert);
}
return true;
}
@@ -2435,6 +2438,7 @@ SSLMultiCertConfigLoader::set_session_id_context(SSL_CTX
*ctx, const SSLConfigPa
// Set the list of CA's to send to client if we ask for a client
certificate
SSL_CTX_set_client_CA_list(ctx, ca_list);
+ ca_list = nullptr; // ownership transferred to ctx
}
if (EVP_DigestFinal_ex(digest, hash_buf, &hash_len) == 0) {
@@ -2451,6 +2455,9 @@ SSLMultiCertConfigLoader::set_session_id_context(SSL_CTX
*ctx, const SSLConfigPa
fail:
EVP_MD_CTX_free(digest);
+ if (ca_list != nullptr) {
+ sk_X509_NAME_pop_free(ca_list, X509_NAME_free);
+ }
return result;
}
