From: Amos Jeffries Date: Fri, 10 Feb 2017 13:35:05 +0000 (+1300) Subject: TLS: refactor Security::ContextPointer to a std::shared_ptr X-Git-Tag: M-staged-PR71~257^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1c1fae0f30020cde7d3c39854a3870fb6d62f801;p=thirdparty%2Fsquid.git 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 6881e041ee..39aad1e3ab 100644 --- 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 337c3b2c6e..99e7c6561c 100644 --- 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 @@ -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 > ContextPointer; +typedef std::shared_ptr ContextPointer; #elif USE_GNUTLS -CtoCpp1(gnutls_certificate_free_credentials, gnutls_certificate_credentials_t); -typedef Security::LockingPointer ContextPointer; +typedef std::shared_ptr ContextPointer; #else // use void* so we can check against nullptr -typedef Security::LockingPointer ContextPointer; +typedef std::shared_ptr ContextPointer; #endif diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc index 070665f012..04ed624887 100644 --- 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 d8c16e887a..1661ad2d14 100644 --- 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 8c40efdb48..6551bed601 100644 --- 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.