This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.2.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 83bb02235ca79c804b934de441634af4b61ad93d 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. (cherry picked from commit b85ebd8709ac16ca8cdae5372bab38fc7b90e148) --- 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 0c500f9006..342e0dea49 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -1240,6 +1240,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; } @@ -1424,6 +1426,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); @@ -1432,7 +1439,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 b14462a4da..e4150f0915 100644 --- a/src/iocore/net/SSLNetVConnection.cc +++ b/src/iocore/net/SSLNetVConnection.cc @@ -1743,6 +1743,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 7c37a2367d..ef1d0080dd 100644 --- a/src/iocore/net/SSLUtils.cc +++ b/src/iocore/net/SSLUtils.cc @@ -1261,8 +1261,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 @@ -1688,13 +1691,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); @@ -1739,24 +1744,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); } @@ -2516,10 +2521,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; @@ -2531,9 +2536,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; } @@ -2587,16 +2591,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; } @@ -2647,6 +2650,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) { @@ -2663,6 +2667,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; }
