Do not leak Security::CertErrors created in X509_verify_cert() (#1346)

ACLFilledChecklist was using a raw C pointer for handling cbdata-managed
Security::CertErrors. Some sslErrors users knew about hidden cbdata
requirements, some did not, resulting in inconsistent locking/unlocking
and associated memory leaks. Upgrading ACLFilledChecklist::sslErrors to
smart CbcPointer fixes those leaks (and simplifies code).

diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index edd603b7787a1331c5a4b18fb15c358073047c85..285540162a364aa724e88f7add2881e6180f53c4 100644 (file)
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -33,9 +33,6 @@ ACLFilledChecklist::ACLFilledChecklist() :
 #endif
 #if SQUID_SNMP
     snmp_community(nullptr),
-#endif
-#if USE_OPENSSL
-    sslErrors(nullptr),
 #endif
     requestErrorType(ERR_MAX),
     conn_(nullptr),
@@ -61,10 +58,6 @@ ACLFilledChecklist::~ACLFilledChecklist()
 
     cbdataReferenceDone(conn_);
 
-#if USE_OPENSSL
-    cbdataReferenceDone(sslErrors);
-#endif
-
     debugs(28, 4, "ACLFilledChecklist destroyed " << this);
 }
 
@@ -227,9 +220,6 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re
 #endif
 #if SQUID_SNMP
     snmp_community(nullptr),
-#endif
-#if USE_OPENSSL
-    sslErrors(nullptr),
 #endif
     requestErrorType(ERR_MAX),
     conn_(nullptr),

diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index c600ebba48d33a3f04ec328aab632ab748b27ae6..0e71be360a1247c48bfae43a1d2ec99a213fad5e 100644 (file)
--- a/src/acl/FilledChecklist.h
+++ b/src/acl/FilledChecklist.h
@@ -87,11 +87,12 @@ public:
     char *snmp_community;
 #endif
 
+    // TODO: RefCount errors; do not ignore them because their "owner" is gone!
     /// TLS server [certificate validation] errors, in undefined order.
     /// The errors are accumulated as Squid goes through validation steps
     /// and server certificates. They are cleared on connection retries.
     /// For sslproxy_cert_error checks, contains just the current/last error.
-    const Security::CertErrors *sslErrors;
+    CbcPointer<Security::CertErrors> sslErrors;
 
     /// Peer certificate being checked by ssl_verify_cb() and by
     /// Security::PeerConnector class. In other contexts, the peer

diff --git a/src/acl/SslError.cc b/src/acl/SslError.cc
index f06f24df27a173dccdf86d70ad51ef55c636bae8..4bea9f73499d039e8d25c6471e4d54ff35fd7a22 100644 (file)
--- a/src/acl/SslError.cc
+++ b/src/acl/SslError.cc
@@ -14,6 +14,6 @@
 int
 ACLSslErrorStrategy::match (ACLData<MatchType> * &data, ACLFilledChecklist *checklist)
 {
-    return data->match (checklist->sslErrors);
+    return data->match(checklist->sslErrors.get());
 }
 

diff --git a/src/client_side.cc b/src/client_side.cc
index 00d49faab943e542edb32182a472a28a56deb5a4..3d6f0a5727ec66c359e891a8104917594db7c401 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1524,11 +1524,10 @@ bool ConnStateData::serveDelayedError(Http::Stream *context)
             bool allowDomainMismatch = false;
             if (Config.ssl_client.cert_error) {
                 ACLFilledChecklist check(Config.ssl_client.cert_error, nullptr);
-                check.sslErrors = new Security::CertErrors(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert));
+                const auto sslErrors = std::make_unique<Security::CertErrors>(Security::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert));
+                check.sslErrors = sslErrors.get();
                 clientAclChecklistFill(check, http);
                 allowDomainMismatch = check.fastCheck().allowed();
-                delete check.sslErrors;
-                check.sslErrors = nullptr;
             }
 
             if (!allowDomainMismatch) {
@@ -3586,7 +3585,7 @@ ConnStateData::fillConnectionLevelDetails(ACLFilledChecklist &checklist) const
 
 #if USE_OPENSSL
     if (!checklist.sslErrors && sslServerBump)
-        checklist.sslErrors = cbdataReference(sslServerBump->sslErrors());
+        checklist.sslErrors = sslServerBump->sslErrors();
 #endif
 
     if (!checklist.rfc931[0]) // checklist creator may have supplied it already

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index b6db325b1a914e4feb359cfbeb64435e71868e64..f12229403e77b94a224f23ec3d9880af27567769 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -402,9 +402,11 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons
         if (!errDetails) {
             bool allowed = false;
             if (check) {
-                check->sslErrors = new Security::CertErrors(Security::CertError(i->error_no, i->cert, i->error_depth));
+                const auto sslErrors = std::make_unique<Security::CertErrors>(Security::CertError(i->error_no, i->cert, i->error_depth));
+                check->sslErrors = sslErrors.get();
                 if (check->fastCheck().allowed())
                     allowed = true;
+                check->sslErrors.clear();
             }
             // else the Config.ssl_client.cert_error access list is not defined
             // and the first error will cause the error page
@@ -418,10 +420,6 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons
                 const char *aReason = i->error_reason.empty() ? nullptr : i->error_reason.c_str();
                 errDetails = new ErrorDetail(i->error_no, peerCert, brokenCert, aReason);
             }
-            if (check) {
-                delete check->sslErrors;
-                check->sslErrors = nullptr;
-            }
         }
 
         if (!errs)

diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc
index 76715da3f32dbcbcf86ad024365628424a2f48e1..ffa85b7efc5763068dfb1debcb92f159ae03fafb 100644 (file)
--- a/src/ssl/ServerBump.cc
+++ b/src/ssl/ServerBump.cc
@@ -62,13 +62,12 @@ Ssl::ServerBump::attachServerSession(const Security::SessionPointer &s)
     serverSession = s;
 }
 
-const Security::CertErrors *
+Security::CertErrors *
 Ssl::ServerBump::sslErrors() const
 {
     if (!serverSession)
         return nullptr;
 
-    const Security::CertErrors *errs = static_cast<const Security::CertErrors*>(SSL_get_ex_data(serverSession.get(), ssl_ex_index_ssl_errors));
-    return errs;
+    return static_cast<Security::CertErrors*>(SSL_get_ex_data(serverSession.get(), ssl_ex_index_ssl_errors));
 }
 

diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h
index f60983ec95d76429c2adf5f7e1e5d0b002c03c1d..c8af4d0208e3d75b233b6fa2b84d0fa5f4654349 100644 (file)
--- a/src/ssl/ServerBump.h
+++ b/src/ssl/ServerBump.h
@@ -38,7 +38,7 @@ public:
     explicit ServerBump(ClientHttpRequest *http, StoreEntry *e = nullptr, Ssl::BumpMode mode = Ssl::bumpServerFirst);
     ~ServerBump();
     void attachServerSession(const Security::SessionPointer &); ///< Sets the server TLS session object
-    const Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors
+    Security::CertErrors *sslErrors() const; ///< SSL [certificate validation] errors
 
     /// whether there was a successful connection to (and peeking at) the origin server
     bool connectedOk() const {return entry && entry->isEmpty();}

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index ce7896ead6ec52edf0f07aae9377e805ba93264c..28e88136d803cad2e961d0a01ade421d3f9d89f4 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -351,7 +351,8 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
             if (check) {
                 ACLFilledChecklist *filledCheck = Filled(check);
                 const auto savedErrors = filledCheck->sslErrors;
-                filledCheck->sslErrors = new Security::CertErrors(Security::CertError(error_no, broken_cert));
+                const auto sslErrors = std::make_unique<Security::CertErrors>(Security::CertError(error_no, broken_cert));
+                filledCheck->sslErrors = sslErrors.get();
                 filledCheck->serverCert = peer_cert;
                 if (check->fastCheck().allowed()) {
                     debugs(83, 3, "bypassing SSL error " << error_no << " in " << *peer_cert);
@@ -359,7 +360,6 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
                 } else {
                     debugs(83, 5, "confirming SSL error " << error_no);
                 }
-                delete filledCheck->sslErrors;
                 filledCheck->sslErrors = savedErrors;
                 filledCheck->serverCert.reset();
             }