From: Amos Jeffries Date: Wed, 20 Jan 2016 02:59:02 +0000 (+1300) Subject: Add support for GnuTLS and other non-OpenSSL logics to LockingPointer X-Git-Tag: SQUID_4_0_13~39^2~21 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=154c26950b46e983951a39cf19b4f131d998e3ec;p=thirdparty%2Fsquid.git Add support for GnuTLS and other non-OpenSSL logics to LockingPointer This brings LockingPointer and TidyPointer back to the same API definition for the reset() method. And ensures that cases where LockingPointer could accidentally have been used with reset() now lock/unlock properly. To do this a static global is defined to store the currently active Lock for each allocated base T objects. For each type T the template Locks() method needs to be instantiated in an appropriate place. If there is no better place src/security/support.cc is used. --- diff --git a/src/base/TidyPointer.h b/src/base/TidyPointer.h index ce2c3bd966..5eb7d43b65 100644 --- a/src/base/TidyPointer.h +++ b/src/base/TidyPointer.h @@ -30,7 +30,7 @@ public: /// Address of the raw pointer, for pointer-setting functions T **addr() { return &raw; } /// Reset raw pointer - delete last one and save new one. - void reset(T *t) { + virtual void reset(T *t) { deletePointer(); raw = t; } @@ -42,7 +42,7 @@ public: return ret; } /// Deallocate raw pointer. - ~TidyPointer() { + virtual ~TidyPointer() { deletePointer(); } private: diff --git a/src/client_side.cc b/src/client_side.cc index b9634c8fa1..ce5cbc671e 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3573,9 +3573,9 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer // fake certificate adaptation requires bump-server-first mode if (!sslServerBump) { assert(port->signingCert.get()); - certProperties.signWithX509.resetAndLock(port->signingCert.get()); + certProperties.signWithX509.reset(port->signingCert.get()); if (port->signPkey.get()) - certProperties.signWithPkey.resetAndLock(port->signPkey.get()); + certProperties.signWithPkey.reset(port->signPkey.get()); certProperties.signAlgorithm = Ssl::algSignTrusted; return; } @@ -3587,7 +3587,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer assert(sslServerBump->entry); if (sslServerBump->entry->isEmpty()) { if (X509 *mimicCert = sslServerBump->serverCert.get()) - certProperties.mimicCert.resetAndLock(mimicCert); + certProperties.mimicCert.reset(mimicCert); ACLFilledChecklist checklist(NULL, sslServerBump->request.getRaw(), clientConnection != NULL ? clientConnection->rfc931 : dash_str); @@ -3640,14 +3640,14 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer if (certProperties.signAlgorithm == Ssl::algSignUntrusted) { assert(port->untrustedSigningCert.get()); - certProperties.signWithX509.resetAndLock(port->untrustedSigningCert.get()); - certProperties.signWithPkey.resetAndLock(port->untrustedSignPkey.get()); + certProperties.signWithX509.reset(port->untrustedSigningCert.get()); + certProperties.signWithPkey.reset(port->untrustedSignPkey.get()); } else { assert(port->signingCert.get()); - certProperties.signWithX509.resetAndLock(port->signingCert.get()); + certProperties.signWithX509.reset(port->signingCert.get()); if (port->signPkey.get()) - certProperties.signWithPkey.resetAndLock(port->signPkey.get()); + certProperties.signWithPkey.reset(port->signPkey.get()); } signAlgorithm = certProperties.signAlgorithm; diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h index 3d3c6e0890..106d7e9027 100644 --- a/src/security/LockingPointer.h +++ b/src/security/LockingPointer.h @@ -12,6 +12,7 @@ #include "base/TidyPointer.h" #if USE_OPENSSL + #if HAVE_OPENSSL_CRYPTO_H #include #endif @@ -24,6 +25,11 @@ sk_object ## _pop_free(a, freefunction); \ } +#else // !USE_OPENSSL + +#include "base/Lock.h" +#include + #endif // Macro to be used to define the C++ equivalent function of an extern "C" @@ -46,44 +52,70 @@ public: typedef TidyPointer Parent; typedef LockingPointer SelfType; - explicit LockingPointer(T *t = nullptr): Parent(t) {} + explicit LockingPointer(T *t = nullptr): Parent() {reset(t);} + + virtual ~LockingPointer() { Parent::reset(nullptr); } explicit LockingPointer(const SelfType &o): Parent() { - resetAndLock(o.get()); + reset(o.get()); } SelfType &operator =(const SelfType & o) { - resetAndLock(o.get()); + reset(o.get()); return *this; } -#if __cplusplus >= 201103L - explicit LockingPointer(LockingPointer &&o): Parent(o.get()) { - *o.addr() = nullptr; + explicit LockingPointer(LockingPointer &&o) : Parent() { + *(this->addr()) = o.get(); + o.release(); } LockingPointer &operator =(LockingPointer &&o) { if (o.get() != this->get()) { - this->reset(o.get()); - *o.addr() = nullptr; + if (this->get()) { + Parent::reset(o.get()); + } else { + *(this->addr()) = o.get(); + o.release(); + } } return *this; } + + virtual void reset(T *t) { + if (t == this->get()) + return; + +#if !USE_OPENSSL + // OpenSSL maintains the reference locks through calls to Deallocator + // our manual locking does not have that luxury + if (this->get()) { + if (SelfType::Locks().at(this->get()).unlock()) + SelfType::Locks().erase(this->get()); + } #endif + Parent::reset(t); - void resetAndLock(T *t) { - if (t != this->get()) { - this->reset(t); + if (t) { #if USE_OPENSSL - if (t) - CRYPTO_add(&t->references, 1, lock); -#elif USE_GNUTLS - // XXX: GnuTLS does not provide locking ? + CRYPTO_add(&t->references, 1, lock); #else - assert(false); + SelfType::Locks()[t].lock(); // find/create and lock #endif } } + +private: +#if !USE_OPENSSL + // since we can never be sure if a raw-* passed to us is already being + // lock counted by another LockingPointer<> and the types pointed to are + // defined by third-party libraries we have to maintain the locks in a + // type-specific static external to both the Pointer and base classes. + static std::unordered_map & Locks() { + static std::unordered_map Instance; + return Instance; + } +#endif }; } // namespace Security diff --git a/src/security/Makefile.am b/src/security/Makefile.am index 17affaff1c..afc0007fe0 100644 --- a/src/security/Makefile.am +++ b/src/security/Makefile.am @@ -23,4 +23,5 @@ libsecurity_la_SOURCES= \ PeerOptions.h \ ServerOptions.cc \ ServerOptions.h \ - Session.h + Session.h \ + support.cc diff --git a/src/security/forward.h b/src/security/forward.h index 54d6b06fb3..382f2c9616 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -43,7 +43,7 @@ typedef Security::LockingPointer CertPoin CtoCpp1(gnutls_x509_crt_deinit, gnutls_x509_crt_t) typedef Security::LockingPointer CertPointer; #else -typedef void * CertPointer; +typedef Security::LockingPointer CertPointer; #endif #if USE_OPENSSL @@ -53,7 +53,7 @@ typedef LockingPointer CrlPoi CtoCpp1(gnutls_x509_crl_deinit, gnutls_x509_crl_t) typedef Security::LockingPointer CrlPointer; #else -typedef void *CrlPointer; +typedef Security::LockingPointer CrlPointer; #endif typedef std::list CertRevokeList; @@ -62,7 +62,7 @@ typedef std::list CertRevokeList; CtoCpp1(DH_free, DH *); typedef Security::LockingPointer DhePointer; #else -typedef void *DhePointer; +typedef Security::LockingPointer DhePointer; #endif class KeyData; diff --git a/src/security/support.cc b/src/security/support.cc new file mode 100644 index 0000000000..0e376d13c2 --- /dev/null +++ b/src/security/support.cc @@ -0,0 +1,30 @@ +/* + * Copyright (C) 1996-2016 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "security/forward.h" + +#if !USE_OPENSSL + +namespace Security +{ + +#if USE_GNUTLS +template std::unordered_map & ContextPointer::Locks(); + +template std::unordered_map & CertPointer::Locks(); + +template std::unordered_map & CrlPointer::Locks(); +#endif + +template std::unordered_map & DhePointer::Locks(); + +} // namespace Security + +#endif /* !USE_OPENSSL */ + diff --git a/src/ssl/ErrorDetail.cc b/src/ssl/ErrorDetail.cc index b6ce27ff92..ec3ff47461 100644 --- a/src/ssl/ErrorDetail.cc +++ b/src/ssl/ErrorDetail.cc @@ -628,12 +628,12 @@ const String &Ssl::ErrorDetail::toString() const Ssl::ErrorDetail::ErrorDetail( Ssl::ssl_error_t err_no, X509 *cert, X509 *broken, const char *aReason): error_no (err_no), lib_error_no(SSL_ERROR_NONE), errReason(aReason) { if (cert) - peer_cert.resetAndLock(cert); + peer_cert.reset(cert); if (broken) - broken_cert.resetAndLock(broken); + broken_cert.reset(broken); else - broken_cert.resetAndLock(cert); + broken_cert.reset(cert); detailEntry.error_no = SSL_ERROR_NONE; } @@ -644,11 +644,11 @@ Ssl::ErrorDetail::ErrorDetail(Ssl::ErrorDetail const &anErrDetail) request = anErrDetail.request; if (anErrDetail.peer_cert.get()) { - peer_cert.resetAndLock(anErrDetail.peer_cert.get()); + peer_cert.reset(anErrDetail.peer_cert.get()); } if (anErrDetail.broken_cert.get()) { - broken_cert.resetAndLock(anErrDetail.broken_cert.get()); + broken_cert.reset(anErrDetail.broken_cert.get()); } detailEntry = anErrDetail.detailEntry; diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 2f0199d928..a5d872d98a 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -760,7 +760,7 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) if (!serverBump->serverCert.get()) { // remember the server certificate from the ErrorDetail object if (error && error->detail && error->detail->peerCert()) - serverBump->serverCert.resetAndLock(error->detail->peerCert()); + serverBump->serverCert.reset(error->detail->peerCert()); else { handleServerCertificate(); } @@ -885,7 +885,7 @@ Ssl::PeekingPeerConnector::serverCertificateVerified() if (ConnStateData *csd = request->clientConnectionManager.valid()) { Security::CertPointer serverCert; if(Ssl::ServerBump *serverBump = csd->serverBump()) - serverCert.resetAndLock(serverBump->serverCert.get()); + serverCert.reset(serverBump->serverCert.get()); else { const int fd = serverConnection()->fd; SSL *ssl = fd_table[fd].ssl; diff --git a/src/ssl/cert_validate_message.cc b/src/ssl/cert_validate_message.cc index 7c2249ed61..409283309f 100644 --- a/src/ssl/cert_validate_message.cc +++ b/src/ssl/cert_validate_message.cc @@ -216,7 +216,7 @@ Ssl::CertValidationResponse::RecvdError & Ssl::CertValidationResponse::RecvdErro void Ssl::CertValidationResponse::RecvdError::setCert(X509 *aCert) { - cert.resetAndLock(aCert); + cert.reset(aCert); } Ssl::CertValidationMsg::CertItem::CertItem(const CertItem &old) @@ -235,7 +235,7 @@ Ssl::CertValidationMsg::CertItem & Ssl::CertValidationMsg::CertItem::operator = void Ssl::CertValidationMsg::CertItem::setCert(X509 *aCert) { - cert.resetAndLock(aCert); + cert.reset(aCert); } const std::string Ssl::CertValidationMsg::code_cert_validate("cert_validate"); diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index fba15426a7..da8370777b 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -424,7 +424,7 @@ static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl: Ssl::EVP_PKEY_Pointer pkey; // Use signing certificates private key as generated certificate private key if (properties.signWithPkey.get()) - pkey.resetAndLock(properties.signWithPkey.get()); + pkey.reset(properties.signWithPkey.get()); else // if not exist generate one pkey.reset(Ssl::createSslPrivateKey()); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index d4fc4c8196..9fecc54efc 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -307,7 +307,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) ACLFilledChecklist *filledCheck = Filled(check); assert(!filledCheck->sslErrors); filledCheck->sslErrors = new Ssl::CertErrors(Ssl::CertError(error_no, broken_cert)); - filledCheck->serverCert.resetAndLock(peer_cert); + filledCheck->serverCert.reset(peer_cert); if (check->fastCheck() == ACCESS_ALLOWED) { debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer); ok = 1; @@ -1288,8 +1288,8 @@ bool Ssl::generateUntrustedCert(Security::CertPointer &untrustedCert, EVP_PKEY_P // O, OU, and other CA subject fields will be mimicked // Expiration date and other common properties will be mimicked certProperties.signAlgorithm = Ssl::algSignSelf; - certProperties.signWithPkey.resetAndLock(pkey.get()); - certProperties.mimicCert.resetAndLock(cert.get()); + certProperties.signWithPkey.reset(pkey.get()); + certProperties.mimicCert.reset(cert.get()); return Ssl::generateSslCertificate(untrustedCert, untrustedPkey, certProperties); } @@ -1342,19 +1342,19 @@ Ssl::CreateServer(Security::ContextPtr sslContext, const int fd, const char *squ Ssl::CertError::CertError(ssl_error_t anErr, X509 *aCert, int aDepth): code(anErr), depth(aDepth) { - cert.resetAndLock(aCert); + cert.reset(aCert); } Ssl::CertError::CertError(CertError const &err): code(err.code), depth(err.depth) { - cert.resetAndLock(err.cert.get()); + cert.reset(err.cert.get()); } Ssl::CertError & Ssl::CertError::operator = (const CertError &old) { code = old.code; - cert.resetAndLock(old.cert.get()); + cert.reset(old.cert.get()); return *this; }