From: Alex Rousskov Date: Sat, 23 Oct 2021 01:45:42 +0000 (+0000) Subject: Fix reconfiguration leaking tls-cert=... memory (#911) X-Git-Tag: SQUID_6_0_1~280 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b05c195415169b684b6037f306feead45ee9de4e;p=thirdparty%2Fsquid.git Fix reconfiguration leaking tls-cert=... memory (#911) Refactored ReadX509Certificate() API for safe use in more contexts, including in leaking Security::KeyData::loadX509ChainFromFile(). Abstract direct calls to the dangerous PEM_read_bio_X509() API. Leaking (under certain conditions) since master/v5 commit 540e296. --- diff --git a/compat/openssl.h b/compat/openssl.h index ccfcf78df6..615919f539 100644 --- a/compat/openssl.h +++ b/compat/openssl.h @@ -23,6 +23,8 @@ #error compat/openssl.h depends on USE_OPENSSL #endif +#include + #if HAVE_OPENSSL_ASN1_H #include #endif diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc index 5668c6e5d7..0755c95485 100644 --- a/src/security/KeyData.cc +++ b/src/security/KeyData.cc @@ -12,6 +12,7 @@ #include "security/KeyData.h" #include "SquidConfig.h" #include "ssl/bio.h" +#include "ssl/gadgets.h" /** * Read certificate from file. @@ -32,9 +33,7 @@ Security::KeyData::loadX509CertFromFile() return false; } - if (X509 *certificate = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) { - cert.resetWithoutLocking(certificate); - } + cert = Ssl::ReadX509Certificate(bio); // error detected/reported below #elif USE_GNUTLS const char *certFilename = certFile.c_str(); @@ -106,20 +105,20 @@ Security::KeyData::loadX509ChainFromFile() // and add to the chain any other certificate exist in the file CertPointer latestCert = cert; - while (auto ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) { + while (const auto ca = Ssl::ReadX509Certificate(bio)) { // get Issuer name of the cert for debug display - char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca), nullptr, 0); + char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca.get()), nullptr, 0); #if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain // self-signed certificates are not valid in a sent chain - if (X509_check_issued(ca, ca) == X509_V_OK) { + if (X509_check_issued(ca.get(), ca.get()) == X509_V_OK) { debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr); OPENSSL_free(nameStr); continue; } #endif // checks that the chained certs are actually part of a chain for validating cert - const auto checkCode = X509_check_issued(ca, latestCert.get()); + const auto checkCode = X509_check_issued(ca.get(), latestCert.get()); if (checkCode == X509_V_OK) { debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << nameStr); // OpenSSL API requires that we order certificates such that the diff --git a/src/security/cert_generators/file/certificate_db.cc b/src/security/cert_generators/file/certificate_db.cc index 455a7b6c2b..e631413052 100644 --- a/src/security/cert_generators/file/certificate_db.cc +++ b/src/security/cert_generators/file/certificate_db.cc @@ -646,12 +646,11 @@ Ssl::CertificateDb::ReadEntry(std::string filename, Security::CertPointer &cert, Ssl::BIO_Pointer bio; if (!Ssl::OpenCertsFileForReading(bio, filename.c_str())) return false; - if (!Ssl::ReadX509Certificate(bio, cert)) + if (!(cert = Ssl::ReadX509Certificate(bio))) return false; if (!Ssl::ReadPrivateKey(bio, pkey, NULL)) return false; - // The orig certificate is not mandatory - (void)Ssl::ReadX509Certificate(bio, orig); + orig = Ssl::ReadX509Certificate(bio); // optional; may be nil return true; } diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 36262e29ba..6d89e6e44e 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -118,9 +118,7 @@ bool Ssl::readCertAndPrivateKeyFromMemory(Security::CertPointer & cert, Security Ssl::BIO_Pointer bio(BIO_new(BIO_s_mem())); BIO_puts(bio.get(), bufferToRead); - X509 * certPtr = NULL; - cert.resetWithoutLocking(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); - if (!cert) + if (!(cert = Ssl::ReadX509Certificate(bio))) return false; EVP_PKEY * pkeyPtr = NULL; @@ -136,9 +134,7 @@ bool Ssl::readCertFromMemory(Security::CertPointer & cert, char const * bufferTo Ssl::BIO_Pointer bio(BIO_new(BIO_s_mem())); BIO_puts(bio.get(), bufferToRead); - X509 * certPtr = NULL; - cert.resetWithoutLocking(PEM_read_bio_X509(bio.get(), &certPtr, 0, 0)); - if (!cert) + if (!(cert = Ssl::ReadX509Certificate(bio))) return false; return true; @@ -695,15 +691,11 @@ Ssl::OpenCertsFileForReading(Ssl::BIO_Pointer &bio, const char *filename) return true; } -bool -Ssl::ReadX509Certificate(Ssl::BIO_Pointer &bio, Security::CertPointer & cert) +Security::CertPointer +Ssl::ReadX509Certificate(const BIO_Pointer &bio) { assert(bio); - if (X509 *certificate = PEM_read_bio_X509(bio.get(), NULL, NULL, NULL)) { - cert.resetWithoutLocking(certificate); - return true; - } - return false; + return Security::CertPointer(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)); } bool diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 0a2535e41e..062c1b0aee 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -9,12 +9,15 @@ #ifndef SQUID_SSL_GADGETS_H #define SQUID_SSL_GADGETS_H +#if USE_OPENSSL + #include "base/HardFun.h" +#include "compat/openssl.h" #include "security/forward.h" #include "ssl/crtd_message.h" -#if USE_OPENSSL -#include "compat/openssl.h" +#include + #if HAVE_OPENSSL_ASN1_H #include #endif @@ -24,8 +27,6 @@ #if HAVE_OPENSSL_X509V3_H #include #endif -#endif -#include namespace Ssl { @@ -113,11 +114,9 @@ void ReadPrivateKeyFromFile(char const * keyFilename, Security::PrivateKeyPointe */ bool OpenCertsFileForReading(BIO_Pointer &bio, const char *filename); -/** - \ingroup SslCrtdSslAPI - * Read a certificate from bio - */ -bool ReadX509Certificate(BIO_Pointer &bio, Security::CertPointer & cert); +/// reads and returns a certificate using the given OpenSSL BIO +/// \returns a nil pointer on errors (TODO: throw instead) +Security::CertPointer ReadX509Certificate(const BIO_Pointer &); /** \ingroup SslCrtdSslAPI @@ -279,5 +278,7 @@ bool CertificatesCmp(const Security::CertPointer &cert1, const Security::CertPoi const ASN1_BIT_STRING *X509_get_signature(const Security::CertPointer &); } // namespace Ssl + +#endif // USE_OPENSSL #endif // SQUID_SSL_GADGETS_H diff --git a/src/ssl/support.cc b/src/ssl/support.cc index e33fad6adf..1ed635c5ab 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -1111,20 +1111,18 @@ Ssl::findIssuerUri(X509 *cert) bool Ssl::loadCerts(const char *certsFile, Ssl::CertsIndexedList &list) { - BIO *in = BIO_new_file(certsFile, "r"); + const BIO_Pointer in(BIO_new_file(certsFile, "r")); if (!in) { debugs(83, DBG_IMPORTANT, "Failed to open '" << certsFile << "' to load certificates"); return false; } - X509 *aCert; - while((aCert = PEM_read_bio_X509(in, NULL, NULL, NULL))) { + while (auto aCert = ReadX509Certificate(in)) { static char buffer[2048]; - X509_NAME_oneline(X509_get_subject_name(aCert), buffer, sizeof(buffer)); - list.insert(std::pair(SBuf(buffer), aCert)); + X509_NAME_oneline(X509_get_subject_name(aCert.get()), buffer, sizeof(buffer)); + list.insert(std::pair(SBuf(buffer), aCert.release())); } debugs(83, 4, "Loaded " << list.size() << " certificates from file: '" << certsFile << "'"); - BIO_free(in); return true; }