TLS: refactor Security::ContextPointer to a std::shared_ptr

These pointers now use the same construction pattern tested out with
Security::SessionPointer.

It also fixes a reference counting bug in GnuTLS code paths where the
PeerConnector::initialize() method would be passed a temporary Pointer
and thus free the context/credentials before it was used by the session
verify logics.

diff --git a/src/client_side.cc b/src/client_side.cc
index 6881e041ee416a9a6fcf4981d50a6a07ba09dbcc..39aad1e3abeddfe3ca5cb8843be7add7aaaf8197 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2852,8 +2852,7 @@ ConnStateData::sslCrtdHandleReply(const Helper::Reply &reply)
                     if (!ret)
                         debugs(33, 5, "Failed to set certificates to ssl object for PeekAndSplice mode");
 
-                    Security::ContextPointer ctx;
-                    ctx.resetAndLock(SSL_get_SSL_CTX(ssl));
+                    Security::ContextPointer ctx(Security::GetFrom(fd_table[clientConnection->fd].ssl));
                     Ssl::configureUnconfiguredSslContext(ctx, signAlgorithm, *port);
                 } else {
                     Security::ContextPointer ctx(Ssl::generateSslContextUsingPkeyAndCertFromMemory(reply_message.getBody().c_str(), *port));
@@ -3008,8 +3007,7 @@ ConnStateData::getSslContextStart()
             if (!Ssl::configureSSL(ssl, certProperties, *port))
                 debugs(33, 5, "Failed to set certificates to ssl object for PeekAndSplice mode");
 
-            Security::ContextPointer ctx;
-            ctx.resetAndLock(SSL_get_SSL_CTX(ssl));
+            Security::ContextPointer ctx(Security::GetFrom(fd_table[clientConnection->fd].ssl));
             Ssl::configureUnconfiguredSslContext(ctx, certProperties.signAlgorithm, *port);
         } else {
             Security::ContextPointer dynCtx(Ssl::generateSslContext(certProperties, *port));

diff --git a/src/security/Context.h b/src/security/Context.h
index 337c3b2c6e2d2edbf98f8a675c513255463f79ed..99e7c6561cf2209613dc3740f80ef36b9a02b2ae 100644 (file)
--- a/src/security/Context.h
+++ b/src/security/Context.h
@@ -9,9 +9,6 @@
 #ifndef SQUID_SRC_SECURITY_CONTEXT_H
 #define SQUID_SRC_SECURITY_CONTEXT_H
 
-#include "security/forward.h"
-#include "security/LockingPointer.h"
-
 #if USE_OPENSSL
 #if HAVE_OPENSSL_SSL_H
 #include <openssl/ssl.h>
@@ -26,19 +23,14 @@
 namespace Security {
 
 #if USE_OPENSSL
-CtoCpp1(SSL_CTX_free, SSL_CTX *);
-#if defined(CRYPTO_LOCK_SSL_CTX) // OpenSSL 1.0
-inline int SSL_CTX_up_ref(SSL_CTX *t) {if (t) CRYPTO_add(&t->references, 1, CRYPTO_LOCK_SSL_CTX); return 0;}
-#endif
-typedef Security::LockingPointer<SSL_CTX, SSL_CTX_free_cpp, HardFun<int, SSL_CTX *, SSL_CTX_up_ref> > ContextPointer;
+typedef std::shared_ptr<SSL_CTX> ContextPointer;
 
 #elif USE_GNUTLS
-CtoCpp1(gnutls_certificate_free_credentials, gnutls_certificate_credentials_t);
-typedef Security::LockingPointer<struct gnutls_certificate_credentials_st, gnutls_certificate_free_credentials_cpp> ContextPointer;
+typedef std::shared_ptr<struct gnutls_certificate_credentials_st> ContextPointer;
 
 #else
 // use void* so we can check against nullptr
-typedef Security::LockingPointer<void, nullptr> ContextPointer;
+typedef std::shared_ptr<void> ContextPointer;
 
 #endif
 

diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc
index 070665f012404ebcaf271977a044d16791630789..04ed6248874fd7031209a8d6bd35858b7d730030 100644 (file)
--- a/src/security/PeerOptions.cc
+++ b/src/security/PeerOptions.cc
@@ -257,7 +257,9 @@ Security::PeerOptions::createBlankContext() const
         const auto x = ERR_get_error();
         fatalf("Failed to allocate TLS client context: %s\n", Security::ErrorString(x));
     }
-    ctx.resetWithoutLocking(t);
+    ctx = Security::ContextPointer(t, [](SSL_CTX *p) {
+        SSL_CTX_free(p);
+    });
 
 #elif USE_GNUTLS
     // Initialize for X.509 certificate exchange
@@ -265,7 +267,9 @@ Security::PeerOptions::createBlankContext() const
     if (const int x = gnutls_certificate_allocate_credentials(&t)) {
         fatalf("Failed to allocate TLS client context: %s\n", Security::ErrorString(x));
     }
-    ctx.resetWithoutLocking(t);
+    ctx = Security::ContextPointer(t, [](gnutls_certificate_credentials_t p) {
+        gnutls_certificate_free_credentials(p);
+    });
 
 #else
     debugs(83, 1, "WARNING: Failed to allocate TLS client context: No TLS library");

diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc
index d8c16e887ace5c6a9b0137d7f3eb589dd82f73e1..1661ad2d14f284ad3049a0885839737dde8bf3ed 100644 (file)
--- a/src/security/ServerOptions.cc
+++ b/src/security/ServerOptions.cc
@@ -101,7 +101,9 @@ Security::ServerOptions::createBlankContext() const
         const auto x = ERR_get_error();
         debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: " << Security::ErrorString(x));
     }
-    ctx.resetWithoutLocking(t);
+    ctx = Security::ContextPointer(t, [](SSL_CTX *p) {
+        SSL_CTX_free(p);
+    });
 
 #elif USE_GNUTLS
     // Initialize for X.509 certificate exchange
@@ -109,7 +111,9 @@ Security::ServerOptions::createBlankContext() const
     if (const int x = gnutls_certificate_allocate_credentials(&t)) {
         debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: " << Security::ErrorString(x));
     }
-    ctx.resetWithoutLocking(t);
+    ctx = Security::ContextPointer(t, [](gnutls_certificate_credentials_t p) {
+        gnutls_certificate_free_credentials(p);
+    });
 
 #else
     debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: No TLS library");

diff --git a/src/security/Session.h b/src/security/Session.h
index 8c40efdb488ef101203b3aa3f7715b1e9c6ec923..6551bed601953333f5690d01c649a9c9e0228c00 100644 (file)
--- a/src/security/Session.h
+++ b/src/security/Session.h
@@ -78,6 +78,14 @@ void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::Sessi
 void SetSessionResumeData(const Security::SessionPointer &, const Security::SessionStatePointer &);
 
 #if USE_OPENSSL
+/// Helper function to retrieve a (non-locked) ContextPointer from a SessionPointer
+inline Security::ContextPointer
+GetFrom(Security::SessionPointer &s)
+{
+    auto *ctx = SSL_get_SSL_CTX(s.get());
+    return Security::ContextPointer(ctx, [](SSL_CTX *) {/* nothing to unlock/free */});
+}
+
 /// \deprecated use the PeerOptions/ServerOptions API methods instead.
 /// Wraps SessionPointer value creation to reduce risk of
 /// a nasty hack in ssl/support.cc.