From 33cc06292c1c4c90eacaefeb88290fa92511b057 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Wed, 27 Jan 2016 10:02:00 +1300 Subject: [PATCH] Cleanup: update TLS session pointer types * rename SSL* pointer to Security::SessionPtr and SSL_Pointer to SessionPointer as the smart pointer variant. Matching the model designed for TLS context storage. * update fd_table .ssl member to a SessionPointer for safer session pointer deallocation. * migrate most uses of SSL* to Securit::SessionPtr or auto --- src/acl/Certificate.cc | 2 +- src/adaptation/icap/Xaction.cc | 8 ++--- src/client_side.cc | 26 ++++++++-------- src/client_side_request.cc | 4 +-- src/comm.cc | 12 +++---- src/fd.cc | 2 +- src/fde.h | 3 +- src/format/Format.cc | 16 +++++----- src/security/NegotiationHistory.cc | 6 ++-- src/security/NegotiationHistory.h | 10 ++---- src/security/Session.h | 24 ++++++++++++-- src/ssl/PeerConnector.cc | 50 +++++++++++++++--------------- src/ssl/PeerConnector.h | 6 ++-- src/ssl/bio.cc | 2 +- src/ssl/gadgets.h | 3 -- src/ssl/support.cc | 19 +++++------- src/stat.cc | 4 +-- src/tests/stub_libsecurity.cc | 5 +-- src/tunnel.cc | 2 +- 19 files changed, 99 insertions(+), 105 deletions(-) diff --git a/src/acl/Certificate.cc b/src/acl/Certificate.cc index 00c03df3b8..44144a53ae 100644 --- a/src/acl/Certificate.cc +++ b/src/acl/Certificate.cc @@ -28,7 +28,7 @@ ACLCertificateStrategy::match (ACLData * &data, ACLFilledChecklist *c { const int fd = checklist->fd(); const bool goodDescriptor = 0 <= fd && fd <= Biggest_FD; - auto ssl = goodDescriptor ? fd_table[fd].ssl : nullptr; + auto ssl = goodDescriptor ? fd_table[fd].ssl.get() : nullptr; X509 *cert = SSL_get_peer_certificate(ssl); const bool res = data->match (cert); X509_free(cert); diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index ca4a31ea25..de13235f71 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -60,7 +60,7 @@ public: PeerConnector(aServerConn, aCallback, timeout), icapService(service) {} /* PeerConnector API */ - virtual Security::SessionPointer initializeSsl(); + virtual Security::SessionPtr initializeSsl(); virtual void noteNegotiationDone(ErrorState *error); virtual Security::ContextPtr getSslContext() {return icapService->sslContext;} @@ -296,7 +296,7 @@ void Adaptation::Icap::Xaction::noteCommConnected(const CommConnectCbParams &io) #if USE_OPENSSL // If it is a reused connection and the SSL object is build // we should not negotiate new SSL session - auto ssl = fd_table[io.conn->fd].ssl; + const auto &ssl = fd_table[io.conn->fd].ssl; if (!ssl && service().cfg().secure.encryptTransport) { CbcPointer me(this); securer = asyncCall(93, 4, "Adaptation::Icap::Xaction::handleSecuredPeer", @@ -701,7 +701,7 @@ bool Adaptation::Icap::Xaction::fillVirginHttpHeader(MemBuf &) const } #if USE_OPENSSL -Security::SessionPointer +Security::SessionPtr Ssl::IcapPeerConnector::initializeSsl() { auto ssl = Ssl::PeerConnector::initializeSsl(); @@ -729,7 +729,7 @@ Ssl::IcapPeerConnector::noteNegotiationDone(ErrorState *error) return; const int fd = serverConnection()->fd; - auto ssl = fd_table[fd].ssl; + auto ssl = fd_table[fd].ssl.get(); assert(ssl); if (!SSL_session_reused(ssl)) { if (icapService->sslSession) diff --git a/src/client_side.cc b/src/client_side.cc index b9634c8fa1..8c87acc9e6 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3072,7 +3072,7 @@ ConnStateData::requestTimeout(const CommTimeoutCbParams &io) receivedFirstByte(); return; } - } else if (fd_table[io.conn->fd].ssl == NULL) + } else if (!fd_table[io.conn->fd].ssl) #endif { const HttpRequestMethod method; @@ -3266,7 +3266,7 @@ httpAccept(const CommAcceptCbParams ¶ms) #if USE_OPENSSL /** Create SSL connection structure and update fd_table */ -static Security::SessionPointer +static Security::SessionPtr httpsCreate(const Comm::ConnectionPointer &conn, Security::ContextPtr sslContext) { if (auto ssl = Ssl::CreateServer(sslContext, conn->fd, "client https start")) { @@ -3288,7 +3288,7 @@ static int Squid_SSL_accept(ConnStateData *conn, PF *callback) { int fd = conn->clientConnection->fd; - auto ssl = fd_table[fd].ssl; + auto ssl = fd_table[fd].ssl.get(); int ret; errno = 0; @@ -3337,7 +3337,7 @@ clientNegotiateSSL(int fd, void *data) { ConnStateData *conn = (ConnStateData *)data; X509 *client_cert; - auto ssl = fd_table[fd].ssl; + auto ssl = fd_table[fd].ssl.get(); int ret; if ((ret = Squid_SSL_accept(conn, clientNegotiateSSL)) <= 0) { @@ -3421,7 +3421,7 @@ clientNegotiateSSL(int fd, void *data) static void httpsEstablish(ConnStateData *connState, Security::ContextPtr sslContext) { - Security::SessionPointer ssl = nullptr; + Security::SessionPtr ssl = nullptr; assert(connState); const Comm::ConnectionPointer &details = connState->clientConnection; @@ -3551,7 +3551,7 @@ ConnStateData::sslCrtdHandleReply(const Helper::Reply &reply) debugs(33, 5, HERE << "Certificate for " << sslConnectHostOrIp << " was successfully recieved from ssl_crtd"); if (sslServerBump && (sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare)) { doPeekAndSpliceStep(); - auto ssl = fd_table[clientConnection->fd].ssl; + auto ssl = fd_table[clientConnection->fd].ssl.get(); bool ret = Ssl::configureSSLUsingPkeyAndCertFromMemory(ssl, reply_message.getBody().c_str(), *port); if (!ret) debugs(33, 5, "Failed to set certificates to ssl object for PeekAndSplice mode"); @@ -3711,7 +3711,7 @@ ConnStateData::getSslContextStart() debugs(33, 5, HERE << "Generating SSL certificate for " << certProperties.commonName); if (sslServerBump && (sslServerBump->act.step1 == Ssl::bumpPeek || sslServerBump->act.step1 == Ssl::bumpStare)) { doPeekAndSpliceStep(); - auto ssl = fd_table[clientConnection->fd].ssl; + auto ssl = fd_table[clientConnection->fd].ssl.get(); if (!Ssl::configureSSL(ssl, certProperties, *port)) debugs(33, 5, "Failed to set certificates to ssl object for PeekAndSplice mode"); } else { @@ -3843,7 +3843,7 @@ static void clientPeekAndSpliceSSL(int fd, void *data) { ConnStateData *conn = (ConnStateData *)data; - auto ssl = fd_table[fd].ssl; + auto ssl = fd_table[fd].ssl.get(); debugs(83, 5, "Start peek and splice on FD " << fd); @@ -3905,7 +3905,7 @@ void ConnStateData::startPeekAndSplice() Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, clientPeekAndSpliceSSL, this, 0); switchedToHttps_ = true; - auto ssl = fd_table[clientConnection->fd].ssl; + auto ssl = fd_table[clientConnection->fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); Ssl::ClientBio *bio = static_cast(b->ptr); bio->hold(true); @@ -3941,8 +3941,8 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data) void ConnStateData::splice() { - //Normally we can splice here, because we just got client hello message - auto ssl = fd_table[clientConnection->fd].ssl; + // normally we can splice here, because we just got client hello message + auto ssl = fd_table[clientConnection->fd].ssl.get(); //retrieve received TLS client information clientConnection->tlsNegotiations()->fillWith(ssl); @@ -4001,7 +4001,7 @@ ConnStateData::startPeekAndSpliceDone() void ConnStateData::doPeekAndSpliceStep() { - auto ssl = fd_table[clientConnection->fd].ssl; + auto ssl = fd_table[clientConnection->fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); assert(b); Ssl::ClientBio *bio = static_cast(b->ptr); @@ -4559,7 +4559,7 @@ ConnStateData::handleIdleClientPinnedTlsRead() // renegotiations. We should close the connection except for the last case. Must(pinning.serverConnection != nullptr); - SSL *ssl = fd_table[pinning.serverConnection->fd].ssl; + auto ssl = fd_table[pinning.serverConnection->fd].ssl.get(); if (!ssl) return false; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 290f74d087..2a7f17c547 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -174,7 +174,7 @@ ClientHttpRequest::ClientHttpRequest(ConnStateData * aConn) : #if USE_OPENSSL if (aConn->clientConnection != NULL && aConn->clientConnection->isOpen()) { - if (auto ssl = fd_table[aConn->clientConnection->fd].ssl) + if (auto ssl = fd_table[aConn->clientConnection->fd].ssl.get()) al->cache.sslClientCert.reset(SSL_get_peer_certificate(ssl)); } #endif @@ -851,7 +851,7 @@ ClientHttpRequest::noteAdaptationAclCheckDone(Adaptation::ServiceGroupPointer g) ih->rfc931 = getConn()->clientConnection->rfc931; #if USE_OPENSSL if (getConn()->clientConnection->isOpen()) { - ih->ssluser = sslGetUserEmail(fd_table[getConn()->clientConnection->fd].ssl); + ih->ssluser = sslGetUserEmail(fd_table[getConn()->clientConnection->fd].ssl.get()); } #endif } diff --git a/src/comm.cc b/src/comm.cc index 25bdb55b31..212d6f64a7 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -822,22 +822,18 @@ old_comm_reset_close(int fd) void commStartSslClose(const FdeCbParams ¶ms) { - assert(fd_table[params.fd].ssl != NULL); - ssl_shutdown_method(fd_table[params.fd].ssl); + assert(fd_table[params.fd].ssl); + ssl_shutdown_method(fd_table[params.fd].ssl.get()); } #endif void comm_close_complete(const FdeCbParams ¶ms) { -#if USE_OPENSSL fde *F = &fd_table[params.fd]; + F->ssl.reset(nullptr); - if (F->ssl) { - SSL_free(F->ssl); - F->ssl = NULL; - } - +#if USE_OPENSSL if (F->dynamicSslContext) { SSL_CTX_free(F->dynamicSslContext); F->dynamicSslContext = NULL; diff --git a/src/fd.cc b/src/fd.cc index 8425ac755b..e3e27ac9a8 100644 --- a/src/fd.cc +++ b/src/fd.cc @@ -97,7 +97,7 @@ fd_close(int fd) F->flags.open = false; fdUpdateBiggest(fd, 0); --Number_FD; - *F = fde(); + F->clear(); } #if _SQUID_WINDOWS_ diff --git a/src/fde.h b/src/fde.h index fdf682e41a..35f5387d1f 100644 --- a/src/fde.h +++ b/src/fde.h @@ -137,7 +137,6 @@ public: connection, whereas nfmarkToServer is the value to set on packets *leaving* Squid. */ -private: /** Clear the fde class back to NULL equivalent. */ inline void clear() { type = 0; @@ -168,7 +167,7 @@ private: halfClosedReader = NULL; read_method = NULL; write_method = NULL; - ssl = NULL; + ssl.reset(nullptr); dynamicSslContext = NULL; #if _SQUID_WINDOWS_ win32.handle = (long)NULL; diff --git a/src/format/Format.cc b/src/format/Format.cc index 0b656121a0..674b45af0b 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -1174,8 +1174,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_EXT_ACL_USER_CERT_RAW: if (al->request) { ConnStateData *conn = al->request->clientConnectionManager.get(); - if (conn != NULL && Comm::IsConnOpen(conn->clientConnection)) { - if (SSL *ssl = fd_table[conn->clientConnection->fd].ssl) + if (conn && Comm::IsConnOpen(conn->clientConnection)) { + if (auto ssl = fd_table[conn->clientConnection->fd].ssl.get()) out = sslGetUserCertificatePEM(ssl); } } @@ -1184,8 +1184,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_EXT_ACL_USER_CERTCHAIN_RAW: if (al->request) { ConnStateData *conn = al->request->clientConnectionManager.get(); - if (conn != NULL && Comm::IsConnOpen(conn->clientConnection)) { - if (SSL *ssl = fd_table[conn->clientConnection->fd].ssl) + if (conn && Comm::IsConnOpen(conn->clientConnection)) { + if (auto ssl = fd_table[conn->clientConnection->fd].ssl.get()) out = sslGetUserCertificatePEM(ssl); } } @@ -1194,8 +1194,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_EXT_ACL_USER_CERT: if (al->request) { ConnStateData *conn = al->request->clientConnectionManager.get(); - if (conn != NULL && Comm::IsConnOpen(conn->clientConnection)) { - if (SSL *ssl = fd_table[conn->clientConnection->fd].ssl) + if (conn && Comm::IsConnOpen(conn->clientConnection)) { + if (auto ssl = fd_table[conn->clientConnection->fd].ssl.get()) out = sslGetUserAttribute(ssl, format->data.header.header); } } @@ -1204,8 +1204,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_EXT_ACL_USER_CA_CERT: if (al->request) { ConnStateData *conn = al->request->clientConnectionManager.get(); - if (conn != NULL && Comm::IsConnOpen(conn->clientConnection)) { - if (SSL *ssl = fd_table[conn->clientConnection->fd].ssl) + if (conn && Comm::IsConnOpen(conn->clientConnection)) { + if (auto ssl = fd_table[conn->clientConnection->fd].ssl.get()) out = sslGetCAAttribute(ssl, format->data.header.header); } } diff --git a/src/security/NegotiationHistory.cc b/src/security/NegotiationHistory.cc index 3ee071bf4a..5bad38247d 100644 --- a/src/security/NegotiationHistory.cc +++ b/src/security/NegotiationHistory.cc @@ -50,10 +50,10 @@ Security::NegotiationHistory::printTlsVersion(int v) const #endif } -#if USE_OPENSSL void -Security::NegotiationHistory::fillWith(SSL *ssl) +Security::NegotiationHistory::fillWith(Security::SessionPtr ssl) { +#if USE_OPENSSL if ((cipher = SSL_get_current_cipher(ssl)) != NULL) { // Set the negotiated version only if the cipher negotiated // else probably the negotiation is not completed and version @@ -76,8 +76,8 @@ Security::NegotiationHistory::fillWith(SSL *ssl) debugs(83, 5, "SSL connection info on FD " << bio->fd() << " SSL version " << version_ << " negotiated cipher " << cipherName()); -} #endif +} const char * Security::NegotiationHistory::cipherName() const diff --git a/src/security/NegotiationHistory.h b/src/security/NegotiationHistory.h index 9a7dfd78e1..18f4d313f0 100644 --- a/src/security/NegotiationHistory.h +++ b/src/security/NegotiationHistory.h @@ -9,11 +9,7 @@ #ifndef SQUID_SRC_SECURITY_NEGOTIATIONHISTORY_H #define SQUID_SRC_SECURITY_NEGOTIATIONHISTORY_H -#if USE_OPENSSL -#if HAVE_OPENSSL_SSL_H -#include -#endif -#endif +#include "security/Session.h" namespace Security { @@ -21,9 +17,7 @@ class NegotiationHistory { public: NegotiationHistory(); -#if USE_OPENSSL - void fillWith(SSL *); ///< Extract negotiation information from TLS object -#endif + void fillWith(Security::SessionPtr); ///< Extract negotiation information from TLS object const char *cipherName() const; ///< The name of negotiated cipher /// String representation of TLS negotiated version const char *negotiatedVersion() const {return printTlsVersion(version_);} diff --git a/src/security/Session.h b/src/security/Session.h index 3be6f05e5b..f095fbac67 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -9,6 +9,9 @@ #ifndef SQUID_SRC_SECURITY_SESSION_H #define SQUID_SRC_SECURITY_SESSION_H +// LockingPointer.h instead of TidyPointer.h for CtoCpp1() +#include "security/LockingPointer.h" + #if USE_OPENSSL #if HAVE_OPENSSL_SSL_H #include @@ -21,17 +24,32 @@ #endif #endif +/* + * NOTE: we use TidyPointer for sessions. OpenSSL provides explicit reference + * locking mechanisms, but GnuTLS only provides init/deinit. To ensure matching + * behaviour we cannot use LockingPointer (yet) and must ensure that there is + * no possibility of double-free being used on the raw pointers. That is + * currently done by using a TidyPointer in the global fde table so its + * lifetime matched the connection. + */ + namespace Security { #if USE_OPENSSL -typedef SSL* SessionPointer; +typedef SSL* SessionPtr; +CtoCpp1(SSL_free, SSL *); +typedef TidyPointer SessionPointer; #elif USE_GNUTLS -typedef gnutls_session_t SessionPointer; +typedef gnutls_session_t SessionPtr; +CtoCpp1(gnutls_deinit, gnutls_session_t); +typedef TidyPointer SessionPointer; #else // use void* so we can check against NULL -typedef void* SessionPointer; +typedef void* SessionPtr; +typedef TidyPointer SessionPointer; + #endif } // namespace Security diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 2f0199d928..05b8ba52fe 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -65,7 +65,7 @@ Ssl::PeerConnector::start() { AsyncJob::start(); - if (prepareSocket() && (initializeSsl() != NULL)) + if (prepareSocket() && initializeSsl()) negotiateSsl(); } @@ -99,7 +99,7 @@ Ssl::PeerConnector::prepareSocket() return true; } -SSL * +Security::SessionPtr Ssl::PeerConnector::initializeSsl() { Security::ContextPtr sslContext(getSslContext()); @@ -107,7 +107,7 @@ Ssl::PeerConnector::initializeSsl() const int fd = serverConnection()->fd; - SSL *ssl = Ssl::CreateClient(sslContext, fd, "server https start"); + auto ssl = Ssl::CreateClient(sslContext, fd, "server https start"); if (!ssl) { ErrorState *anErr = new ErrorState(ERR_SOCKET_FAILURE, Http::scInternalServerError, request.getRaw()); anErr->xerrno = errno; @@ -115,7 +115,7 @@ Ssl::PeerConnector::initializeSsl() noteNegotiationDone(anErr); bail(anErr); - return NULL; + return nullptr; } // If CertValidation Helper used do not lookup checklist for errors, @@ -153,7 +153,7 @@ Ssl::PeerConnector::negotiateSsl() return; const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); const int result = SSL_connect(ssl); if (result <= 0) { handleNegotiateError(result); @@ -171,7 +171,7 @@ Ssl::PeerConnector::sslFinalized() { if (Ssl::TheConfig.ssl_crt_validator && useCertValidator_) { const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); Ssl::CertValidationRequest validationRequest; // WARNING: Currently we do not use any locking for any of the @@ -248,7 +248,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpStare)); acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst)); acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst)); - SSL *ssl = fd_table[serverConn->fd].ssl; + Security::SessionPtr ssl = fd_table[serverConn->fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); Ssl::ServerBio *srvBio = static_cast(b->ptr); if (!srvBio->canSplice()) @@ -261,7 +261,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() void Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode action) { - SSL *ssl = fd_table[serverConn->fd].ssl; + Security::SessionPtr ssl = fd_table[serverConn->fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); Ssl::ServerBio *srvBio = static_cast(b->ptr); debugs(83,5, "Will check for peek and splice on FD " << serverConn->fd); @@ -368,7 +368,7 @@ Ssl::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &re if (acl_access *acl = ::Config.ssl_client.cert_error) check = new ACLFilledChecklist(acl, request.getRaw(), dash_str); - SSL *ssl = fd_table[serverConnection()->fd].ssl; + Security::SessionPtr ssl = fd_table[serverConnection()->fd].ssl.get(); typedef Ssl::CertValidationResponse::RecvdErrors::const_iterator SVCRECI; for (SVCRECI i = resp.errors.begin(); i != resp.errors.end(); ++i) { debugs(83, 7, "Error item: " << i->error_no << " " << i->error_reason); @@ -425,7 +425,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret) { const int fd = serverConnection()->fd; unsigned long ssl_lib_error = SSL_ERROR_NONE; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); const int ssl_error = SSL_get_error(ssl, ret); switch (ssl_error) { @@ -490,7 +490,7 @@ Ssl::PeerConnector::noteSslNegotiationError(const int ret, const int ssl_error, anErr = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, NULL); anErr->xerrno = sysErrNo; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); Ssl::ErrorDetail *errFromFailure = (Ssl::ErrorDetail *)SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_detail); if (errFromFailure != NULL) { // The errFromFailure is attached to the ssl object @@ -597,12 +597,12 @@ Ssl::BlindPeerConnector::getSslContext() return ::Config.ssl_client.sslContext; } -SSL * +Security::SessionPtr Ssl::BlindPeerConnector::initializeSsl() { - SSL *ssl = Ssl::PeerConnector::initializeSsl(); + auto ssl = Ssl::PeerConnector::initializeSsl(); if (!ssl) - return NULL; + return nullptr; if (const CachePeer *peer = serverConnection()->getPeer()) { assert(peer); @@ -639,7 +639,7 @@ Ssl::BlindPeerConnector::noteNegotiationDone(ErrorState *error) } const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); if (serverConnection()->getPeer() && !SSL_session_reused(ssl)) { if (serverConnection()->getPeer()->sslSession) SSL_SESSION_free(serverConnection()->getPeer()->sslSession); @@ -655,12 +655,12 @@ Ssl::PeekingPeerConnector::getSslContext() return ::Config.ssl_client.sslContext; } -SSL * +Security::SessionPtr Ssl::PeekingPeerConnector::initializeSsl() { - SSL *ssl = Ssl::PeerConnector::initializeSsl(); + auto ssl = Ssl::PeerConnector::initializeSsl(); if (!ssl) - return NULL; + return nullptr; if (ConnStateData *csd = request->clientConnectionManager.valid()) { @@ -674,7 +674,7 @@ Ssl::PeekingPeerConnector::initializeSsl() SSL_set_tlsext_status_type(ssl, TLSEXT_STATUSTYPE_ocsp); // In server-first bumping mode, clientSsl is NULL. - if (SSL *clientSsl = fd_table[clientConn->fd].ssl) { + if (auto clientSsl = fd_table[clientConn->fd].ssl.get()) { BIO *b = SSL_get_rbio(clientSsl); cltBio = static_cast(b->ptr); const Ssl::Bio::sslFeatures &features = cltBio->receivedHelloFeatures(); @@ -742,7 +742,7 @@ Ssl::PeekingPeerConnector::initializeSsl() void Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) { - SSL *ssl = fd_table[serverConnection()->fd].ssl; + Security::SessionPtr ssl = fd_table[serverConnection()->fd].ssl.get(); // Check the list error with if (!request->clientConnectionManager.valid() || ! ssl) @@ -788,7 +788,7 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) serverCertificateVerified(); if (splice) { //retrieved received TLS client informations - SSL *clientSsl = fd_table[clientConn->fd].ssl; + auto clientSsl = fd_table[clientConn->fd].ssl.get(); clientConn->tlsNegotiations()->fillWith(clientSsl); switchToTunnel(request.getRaw(), clientConn, serverConn); } @@ -799,7 +799,7 @@ void Ssl::PeekingPeerConnector::noteWantWrite() { const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); Ssl::ServerBio *srvBio = static_cast(b->ptr); @@ -816,7 +816,7 @@ void Ssl::PeekingPeerConnector::noteSslNegotiationError(const int result, const int ssl_error, const int ssl_lib_error) { const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); BIO *b = SSL_get_rbio(ssl); Ssl::ServerBio *srvBio = static_cast(b->ptr); @@ -865,7 +865,7 @@ Ssl::PeekingPeerConnector::handleServerCertificate() if (ConnStateData *csd = request->clientConnectionManager.valid()) { const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); Security::CertPointer serverCert(SSL_get_peer_certificate(ssl)); if (!serverCert.get()) return; @@ -888,7 +888,7 @@ Ssl::PeekingPeerConnector::serverCertificateVerified() serverCert.resetAndLock(serverBump->serverCert.get()); else { const int fd = serverConnection()->fd; - SSL *ssl = fd_table[fd].ssl; + Security::SessionPtr ssl = fd_table[fd].ssl.get(); serverCert.reset(SSL_get_peer_certificate(ssl)); } if (serverCert.get()) { diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h index 3fd0fc4f33..c48321e8d4 100644 --- a/src/ssl/PeerConnector.h +++ b/src/ssl/PeerConnector.h @@ -99,7 +99,7 @@ protected: /// silent server void setReadTimeout(); - virtual SSL *initializeSsl(); ///< Initializes SSL state + virtual Security::SessionPtr initializeSsl(); ///< Initializes SSL state /// Performs a single secure connection negotiation step. /// It is called multiple times untill the negotiation finish or aborted. @@ -193,7 +193,7 @@ public: /// Calls parent initializeSSL, configure the created SSL object to try reuse SSL session /// and sets the hostname to use for certificates validation - virtual SSL *initializeSsl(); + virtual Security::SessionPtr initializeSsl(); /// Return the configured Security::ContextPtr object virtual Security::ContextPtr getSslContext(); @@ -222,7 +222,7 @@ public: } /* PeerConnector API */ - virtual SSL *initializeSsl(); + virtual Security::SessionPtr initializeSsl(); virtual Security::ContextPtr getSslContext(); virtual void noteWantWrite(); virtual void noteSslNegotiationError(const int result, const int ssl_error, const int ssl_lib_error); diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index 84c0592c7f..55e49fc07c 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -440,7 +440,7 @@ Ssl::ServerBio::write(const char *buf, int size, BIO *table) //Hello message is the first message we write to server assert(helloMsg.isEmpty()); - SSL *ssl = fd_table[fd_].ssl; + auto ssl = fd_table[fd_].ssl.get(); if (clientFeatures.initialized_ && ssl) { if (bumpMode_ == Ssl::bumpPeek) { if (adjustSSL(ssl, clientFeatures)) diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index d1111a2008..30c24b1b86 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -69,9 +69,6 @@ typedef TidyPointer X509_REQ_Pointer; CtoCpp1(SSL_CTX_free, SSL_CTX *) typedef TidyPointer SSL_CTX_Pointer; -CtoCpp1(SSL_free, SSL *) -typedef TidyPointer SSL_Pointer; - sk_free_wrapper(sk_X509_NAME, STACK_OF(X509_NAME) *, X509_NAME_free) typedef TidyPointer X509_NAME_STACK_Pointer; diff --git a/src/ssl/support.cc b/src/ssl/support.cc index d4fc4c8196..6b9688fb36 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -697,8 +697,7 @@ sslCreateClientContext(Security::PeerOptions &peer, long options, long fl) int ssl_read_method(int fd, char *buf, int len) { - SSL *ssl = fd_table[fd].ssl; - int i; + auto ssl = fd_table[fd].ssl.get(); #if DONT_DO_THIS @@ -709,7 +708,7 @@ ssl_read_method(int fd, char *buf, int len) #endif - i = SSL_read(ssl, buf, len); + int i = SSL_read(ssl, buf, len); if (i > 0 && SSL_pending(ssl) > 0) { debugs(83, 2, "SSL FD " << fd << " is pending"); @@ -724,16 +723,13 @@ ssl_read_method(int fd, char *buf, int len) int ssl_write_method(int fd, const char *buf, int len) { - SSL *ssl = fd_table[fd].ssl; - int i; - + auto ssl = fd_table[fd].ssl.get(); if (!SSL_is_init_finished(ssl)) { errno = ENOTCONN; return -1; } - i = SSL_write(ssl, buf, len); - + int i = SSL_write(ssl, buf, len); return i; } @@ -1035,7 +1031,7 @@ bool Ssl::verifySslCertificate(Security::ContextPtr sslContext, CertificatePrope assert(0); #else // Temporary ssl for getting X509 certificate from SSL_CTX. - Ssl::SSL_Pointer ssl(SSL_new(sslContext)); + Security::SessionPointer ssl(SSL_new(sslContext)); X509 * cert = SSL_get_certificate(ssl.get()); #endif if (!cert) @@ -1303,16 +1299,15 @@ SslCreate(Security::ContextPtr sslContext, const int fd, Ssl::Bio::Type type, co const char *errAction = NULL; int errCode = 0; - if (SSL *ssl = SSL_new(sslContext)) { + if (auto ssl = SSL_new(sslContext)) { // without BIO, we would call SSL_set_fd(ssl, fd) instead if (BIO *bio = Ssl::Bio::Create(fd, type)) { Ssl::Bio::Link(ssl, bio); // cannot fail - fd_table[fd].ssl = ssl; + fd_table[fd].ssl.reset(ssl); fd_table[fd].read_method = &ssl_read_method; fd_table[fd].write_method = &ssl_write_method; fd_note(fd, squidCtx); - return ssl; } errCode = ERR_get_error(); diff --git a/src/stat.cc b/src/stat.cc index c62ae45d6c..e3f170b890 100644 --- a/src/stat.cc +++ b/src/stat.cc @@ -1889,10 +1889,8 @@ statClientRequests(StoreEntry * s) p = conn->clientConnection->rfc931; #if USE_OPENSSL - if (!p && conn != NULL && Comm::IsConnOpen(conn->clientConnection)) - p = sslGetUserEmail(fd_table[conn->clientConnection->fd].ssl); - + p = sslGetUserEmail(fd_table[conn->clientConnection->fd].ssl.get()); #endif if (!p) diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 6c09cefcc8..5d296a6cb3 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -37,11 +37,8 @@ Security::ContextPtr Security::ServerOptions::createBlankContext() const STUB void Security::ServerOptions::updateContextEecdh(Security::ContextPtr &) STUB #include "security/NegotiationHistory.h" - Security::NegotiationHistory::NegotiationHistory() STUB -#if USE_OPENSSL -void Security::NegotiationHistory::fillWith(SSL *) STUB -#endif +void Security::NegotiationHistory::fillWith(Security::SessionPtr) STUB const char *Security::NegotiationHistory::cipherName() const STUB const char *Security::NegotiationHistory::printTlsVersion(int) const STUB diff --git a/src/tunnel.cc b/src/tunnel.cc index 961949c340..2aae972b9a 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1297,7 +1297,7 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm:: fd_table[srvConn->fd].read_method = &default_read_method; fd_table[srvConn->fd].write_method = &default_write_method; - auto ssl = fd_table[srvConn->fd].ssl; + auto ssl = fd_table[srvConn->fd].ssl.get(); assert(ssl); BIO *b = SSL_get_rbio(ssl); Ssl::ServerBio *srvBio = static_cast(b->ptr); -- 2.47.3