Bug 5188: 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 02aee7022279357ab9207ee53160418d945afe16..e5217ab9fa471b4a53d3552dfef771953151f33c 100644 (file)
--- a/compat/openssl.h
+++ b/compat/openssl.h
@@ -23,6 +23,8 @@
 #error compat/openssl.h depends on USE_OPENSSL
 #endif
 
+#include <algorithm>
+
 #if HAVE_OPENSSL_ASN1_H
 #include <openssl/asn1.h>
 #endif

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index 5668c6e5d7002b6459eeef064dbf4ceba3d6f141..0755c9548565bad13668fef52d0263cfbcb657e7 100644 (file)
--- 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 611d60465d80ef52b905196d5583310bf31fa83d..259c0fe744d23e30589406a864719b8d0ca44d37 100644 (file)
--- a/src/security/cert_generators/file/certificate_db.cc
+++ b/src/security/cert_generators/file/certificate_db.cc
@@ -648,12 +648,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 596f258ea76c6c1e771a6d0fa42921d9e27de5a6..1d8e4a968def75a131198fc5bced40a2dfd280a3 100644 (file)
--- 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 c486727f070add359b4d83861c02ed4cf532087a..209fba930b95c71f9dd7995db9b6a4c97ede2b29 100644 (file)
--- 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 <string>
+
 #if HAVE_OPENSSL_ASN1_H
 #include <openssl/asn1.h>
 #endif
@@ -24,8 +27,6 @@
 #if HAVE_OPENSSL_X509V3_H
 #include <openssl/x509v3.h>
 #endif
-#endif
-#include <string>
 
 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 3ad135d8cf242949433476498dc69bed1d613e07..70656253c9141f5e20227e8aaa35c9165698a0c9 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -1109,20 +1109,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, X509 *>(SBuf(buffer), aCert));
+        X509_NAME_oneline(X509_get_subject_name(aCert.get()), buffer, sizeof(buffer));
+        list.insert(std::pair<SBuf, X509 *>(SBuf(buffer), aCert.release()));
     }
     debugs(83, 4, "Loaded " << list.size() << " certificates from file: '" << certsFile << "'");
-    BIO_free(in);
     return true;
 }