From 908634e8c63a33918d7620c8cb44774748b7ebba Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 10 Sep 2024 01:32:59 +0000 Subject: [PATCH] Bug 5293: Security::CreateClientSession uses wrong TLS options (#1895) When establishing a TLS connection to an origin server _through_ a cache_peer, Security::CreateClientSession() used cache_peer's Security::PeerOptions instead of global ProxyOutgoingConfig (i.e. tls_outgoing_options). Used cache_peer's PeerOptions are unrelated to the (tunneled) TLS connection in question (and are currently empty because Squid does not support TLS inside TLS -- the cache_peer accepts plain HTTP connections). This TLS context:options mismatch exists in both OpenSSL and GnuTLS builds, but it currently does not affect OpenSSL builds where CreateSession() gets TLS options from its Security::Context parameter rather than its (unused) Security::PeerOptions parameter. The mismatch exists on both PeekingPeerConnector/SslBump and BlindPeerConnector code paths. This minimal change pairs TLS context with its TLS options. Long-term, the added FuturePeerContext shim (that stores references to independent context and options objects) should be replaced with a PeerContext class that encapsulates those two objects. We may also be able to avoid re-computing GnuTLS context from options and to simplify code by preventing PeerConnector construction in Squid builds that do not support TLS. That refactoring should be done separately because it triggers many changes unrelated to Bug 5293. Also removed updateSessionOptions() from PeekingPeerConnector::initialize() because Security::CreateSession(), called by our parent initialize() method, already sets session options. It is easier to remove that call/code than keep it up to date. Security::BlindPeerConnector does not updateSessionOptions(). --- src/CachePeer.cc | 11 ++++++++++- src/CachePeer.h | 7 +++++++ src/SquidConfig.h | 2 ++ src/adaptation/icap/ServiceRep.cc | 1 + src/adaptation/icap/ServiceRep.h | 2 ++ src/adaptation/icap/Xaction.cc | 4 +--- src/cache_cf.cc | 3 +++ src/security/BlindPeerConnector.cc | 10 +++++----- src/security/BlindPeerConnector.h | 5 ++--- src/security/PeerConnector.cc | 15 ++++++++++----- src/security/PeerConnector.h | 6 +++--- src/security/PeerOptions.h | 13 +++++++++++++ src/security/Session.cc | 13 +++++++------ src/security/Session.h | 6 +++++- src/security/forward.h | 2 ++ src/ssl/PeekingPeerConnector.cc | 9 +++------ src/ssl/PeekingPeerConnector.h | 2 +- src/tests/stub_libsecurity.cc | 6 +++--- 18 files changed, 80 insertions(+), 37 deletions(-) diff --git a/src/CachePeer.cc b/src/CachePeer.cc index 11d77164045..c0b95cdf527 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -22,7 +22,8 @@ CBDATA_CLASS_INIT(CachePeer); CachePeer::CachePeer(const char * const hostname): name(xstrdup(hostname)), - host(xstrdup(hostname)) + host(xstrdup(hostname)), + tlsContext(secure, sslContext) { Tolower(host); // but .name preserves original spelling } @@ -55,6 +56,14 @@ CachePeer::~CachePeer() xfree(domain); } +Security::FuturePeerContext * +CachePeer::securityContext() +{ + if (secure.encryptTransport) + return &tlsContext; + return nullptr; +} + void CachePeer::noteSuccess() { diff --git a/src/CachePeer.h b/src/CachePeer.h index 5d0addacae4..f5c4cceedaf 100644 --- a/src/CachePeer.h +++ b/src/CachePeer.h @@ -44,6 +44,10 @@ class CachePeer /// \returns the effective connect timeout for the given peer time_t connectTimeout() const; + /// TLS settings for communicating with this TLS cache_peer (if encryption + /// is required; see secure.encryptTransport) or nil (otherwise) + Security::FuturePeerContext *securityContext(); + /// n-th cache_peer directive, starting with 1 u_int index = 0; @@ -209,9 +213,12 @@ class CachePeer char *domain = nullptr; ///< Forced domain + // TODO: Remove secure and sslContext when FuturePeerContext below becomes PeerContext /// security settings for peer connection Security::PeerOptions secure; Security::ContextPointer sslContext; + Security::FuturePeerContext tlsContext; + Security::SessionStatePointer sslSession; int front_end_https = 0; ///< 0 - off, 1 - on, 2 - auto diff --git a/src/SquidConfig.h b/src/SquidConfig.h index 8f5063d7802..0598a2369b6 100644 --- a/src/SquidConfig.h +++ b/src/SquidConfig.h @@ -503,6 +503,8 @@ class SquidConfig external_acl *externalAclHelperList; struct { + Security::FuturePeerContext *defaultPeerContext; + // TODO: Remove when FuturePeerContext above becomes PeerContext Security::ContextPointer sslContext; #if USE_OPENSSL char *foreignIntermediateCertsPath; diff --git a/src/adaptation/icap/ServiceRep.cc b/src/adaptation/icap/ServiceRep.cc index ddefdfefaaf..86b05d4aadb 100644 --- a/src/adaptation/icap/ServiceRep.cc +++ b/src/adaptation/icap/ServiceRep.cc @@ -32,6 +32,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Adaptation::Icap, ServiceRep); Adaptation::Icap::ServiceRep::ServiceRep(const ServiceConfigPointer &svcCfg): AsyncJob("Adaptation::Icap::ServiceRep"), Adaptation::Service(svcCfg), + tlsContext(writeableCfg().secure, sslContext), theOptions(nullptr), theOptionsFetcher(nullptr), theLastUpdate(0), theBusyConns(0), theAllWaiters(0), diff --git a/src/adaptation/icap/ServiceRep.h b/src/adaptation/icap/ServiceRep.h index 7afb3851f8f..669bac9b9a9 100644 --- a/src/adaptation/icap/ServiceRep.h +++ b/src/adaptation/icap/ServiceRep.h @@ -112,6 +112,8 @@ class ServiceRep : public RefCountable, public Adaptation::Service, void noteAdaptationAnswer(const Answer &answer) override; Security::ContextPointer sslContext; + // TODO: Remove sslContext above when FuturePeerContext below becomes PeerContext + Security::FuturePeerContext tlsContext; Security::SessionStatePointer sslSession; private: diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index d4ea81b6544..701e267cf64 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -54,9 +54,7 @@ class IcapPeerConnector: public Security::PeerConnector { /* Security::PeerConnector API */ bool initialize(Security::SessionPointer &) override; void noteNegotiationDone(ErrorState *error) override; - Security::ContextPointer getTlsContext() override { - return icapService->sslContext; - } + Security::FuturePeerContext *peerContext() const override { return &icapService->tlsContext; } private: /* Acl::ChecklistFiller API */ diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 58f69f43f54..90875f9bdde 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -973,6 +973,7 @@ configDoConfigure(void) #if USE_OPENSSL Ssl::useSquidUntrusted(Config.ssl_client.sslContext.get()); #endif + Config.ssl_client.defaultPeerContext = new Security::FuturePeerContext(Security::ProxyOutgoingConfig, Config.ssl_client.sslContext); } for (const auto &p: CurrentCachePeers()) { @@ -3913,6 +3914,8 @@ configFreeMemory(void) { free_all(); Dns::ResolveClientAddressesAsap = false; + delete Config.ssl_client.defaultPeerContext; + Config.ssl_client.defaultPeerContext = nullptr; Config.ssl_client.sslContext.reset(); #if USE_OPENSSL Ssl::unloadSquidUntrusted(); diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index d57a4ea25e5..2d45762cb15 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -20,14 +20,14 @@ CBDATA_NAMESPACED_CLASS_INIT(Security, BlindPeerConnector); -Security::ContextPointer -Security::BlindPeerConnector::getTlsContext() +Security::FuturePeerContext * +Security::BlindPeerConnector::peerContext() const { - const CachePeer *peer = serverConnection()->getPeer(); + const auto peer = serverConnection()->getPeer(); if (peer && peer->secure.encryptTransport) - return peer->sslContext; + return peer->securityContext(); - return ::Config.ssl_client.sslContext; + return Config.ssl_client.defaultPeerContext; } bool diff --git a/src/security/BlindPeerConnector.h b/src/security/BlindPeerConnector.h index 0001579c7bf..57f14e65aa1 100644 --- a/src/security/BlindPeerConnector.h +++ b/src/security/BlindPeerConnector.h @@ -17,7 +17,7 @@ class ErrorState; namespace Security { -/// A simple PeerConnector for SSL/TLS cache_peers. No SslBump capabilities. +/// A PeerConnector for TLS cache_peers and origin servers. No SslBump capabilities. class BlindPeerConnector: public Security::PeerConnector { CBDATA_CHILD(BlindPeerConnector); public: @@ -35,8 +35,7 @@ class BlindPeerConnector: public Security::PeerConnector { /// \returns true on successful initialization bool initialize(Security::SessionPointer &) override; - /// Return the configured TLS context object - Security::ContextPointer getTlsContext() override; + FuturePeerContext *peerContext() const override; /// On success, stores the used TLS session for later use. /// On error, informs the peer. diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 7fdffebf721..2f2e97ff8a8 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -140,10 +140,10 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) { Must(Comm::IsConnOpen(serverConnection())); - Security::ContextPointer ctx(getTlsContext()); - debugs(83, 5, serverConnection() << ", ctx=" << (void*)ctx.get()); + const auto ctx = peerContext(); + debugs(83, 5, serverConnection() << ", ctx=" << ctx); - if (!ctx || !Security::CreateClientSession(ctx, serverConnection(), "server https start")) { + if (!ctx || !Security::CreateClientSession(*ctx, serverConnection(), "server https start")) { const auto xerrno = errno; if (!ctx) { debugs(83, DBG_IMPORTANT, "ERROR: initializing TLS connection: No security context."); @@ -647,7 +647,7 @@ Security::PeerConnector::certDownloadingDone(DownloaderAnswer &downloaderAnswer) downloadedCerts.reset(sk_X509_new_null()); sk_X509_push(downloadedCerts.get(), cert); - ContextPointer ctx(getTlsContext()); + const auto ctx = peerContext()->raw; const auto certsList = SSL_get_peer_cert_chain(&sconn); if (!Ssl::findIssuerCertificate(cert, certsList, ctx)) { if (const auto issuerUri = Ssl::findIssuerUri(cert)) { @@ -712,7 +712,12 @@ Security::PeerConnector::computeMissingCertificateUrls(const Connection &sconn) } debugs(83, 5, "server certificates: " << sk_X509_num(certs)); - const auto ctx = getTlsContext(); + const auto optionalContext = peerContext(); + if (!optionalContext) { + debugs(83, 3, "cannot compute due to disabled TLS support"); + return false; + } + const auto ctx = optionalContext->raw; if (!Ssl::missingChainCertificatesUrls(urlsOfMissingCerts, *certs, ctx)) return false; // missingChainCertificatesUrls() reports the exact reason diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index b00f3852a85..d9467ebb9b8 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -130,9 +130,9 @@ class PeerConnector: virtual public AsyncJob, public Acl::ChecklistFiller /// \param error if not NULL the SSL negotiation was aborted with an error virtual void noteNegotiationDone(ErrorState *) {} - /// Must implemented by the kid classes to return the TLS context object to use - /// for building the encryption context objects. - virtual Security::ContextPointer getTlsContext() = 0; + /// peer's security context + /// \returns nil if Squid is built without TLS support (XXX: Prevent PeerConnector creation in those cases instead) + virtual FuturePeerContext *peerContext() const = 0; /// mimics FwdState to minimize changes to FwdState::initiate/negotiateSsl Comm::ConnectionPointer const &serverConnection() const { return serverConn; } diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h index e78081d36ef..5db3d5a7167 100644 --- a/src/security/PeerOptions.h +++ b/src/security/PeerOptions.h @@ -147,6 +147,19 @@ class PeerOptions bool encryptTransport = false; }; +// XXX: Remove this shim after upgrading legacy code to store PeerContext +// objects instead of disjoint PeerOptons and Context objects (where PeerContext +// is a class that creates and manages {PeerOptions, ContextPointer} pair). +/// A combination of PeerOptions and the corresponding Context. +class FuturePeerContext +{ +public: + FuturePeerContext(PeerOptions &o, const ContextPointer &c): options(o), raw(c) {} + + PeerOptions &options; ///< TLS context configuration + const ContextPointer &raw; ///< TLS context configured using options +}; + /// configuration options for DIRECT server access extern PeerOptions ProxyOutgoingConfig; diff --git a/src/security/Session.cc b/src/security/Session.cc index 07416873674..0530c1ba531 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -180,13 +180,14 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer } bool -Security::CreateClientSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx) +Security::CreateClientSession(FuturePeerContext &ctx, const Comm::ConnectionPointer &c, const char *squidCtx) { - if (!c || !c->getPeer()) - return CreateSession(ctx, c, Security::ProxyOutgoingConfig, Security::Io::BIO_TO_SERVER, squidCtx); - - auto *peer = c->getPeer(); - return CreateSession(ctx, c, peer->secure, Security::Io::BIO_TO_SERVER, squidCtx); + // TODO: We cannot make ctx constant because CreateSession() takes + // non-constant ctx.options (PeerOptions). It does that because GnuTLS + // needs to call PeerOptions::updateSessionOptions(), which is not constant + // because it compiles options (just in case) every time. To achieve + // const-correctness, we should compile PeerOptions once, not every time. + return CreateSession(ctx.raw, c, ctx.options, Security::Io::BIO_TO_SERVER, squidCtx); } bool diff --git a/src/security/Session.h b/src/security/Session.h index 28c48fa67d0..15bb4940f07 100644 --- a/src/security/Session.h +++ b/src/security/Session.h @@ -31,9 +31,13 @@ namespace Security { +// XXX: Should be only in src/security/forward.h (which should not include us +// because that #include creates a circular reference and problems like this). +class FuturePeerContext; + /// Creates TLS Client connection structure (aka 'session' state) and initializes TLS/SSL I/O (Comm and BIO). /// On errors, emits DBG_IMPORTANT with details and returns false. -bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx); +bool CreateClientSession(FuturePeerContext &, const Comm::ConnectionPointer &, const char *squidCtx); class PeerOptions; diff --git a/src/security/forward.h b/src/security/forward.h index 107723d9461..d63e1a946fe 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -209,6 +209,8 @@ class PeerOptions; class ServerOptions; +class FuturePeerContext; + class ErrorDetail; typedef RefCount ErrorDetailPointer; diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 87aaeb1cbb2..235ef50d26f 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -140,10 +140,10 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceGuess() const return Ssl::bumpSplice; } -Security::ContextPointer -Ssl::PeekingPeerConnector::getTlsContext() +Security::FuturePeerContext * +Ssl::PeekingPeerConnector::peerContext() const { - return ::Config.ssl_client.sslContext; + return ::Config.ssl_client.defaultPeerContext; } bool @@ -197,9 +197,6 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession) srvBio->recordInput(true); srvBio->mode(csd->sslBumpMode); } else { - // Set client SSL options - ::Security::ProxyOutgoingConfig.updateSessionOptions(serverSession); - const bool redirected = request->flags.redirected && ::Config.onoff.redir_rewrites_host; const char *sniServer = (!hostName || redirected) ? request->url.host() : diff --git a/src/ssl/PeekingPeerConnector.h b/src/ssl/PeekingPeerConnector.h index 1215f407af7..82cfec99e48 100644 --- a/src/ssl/PeekingPeerConnector.h +++ b/src/ssl/PeekingPeerConnector.h @@ -29,7 +29,7 @@ class PeekingPeerConnector: public Security::PeerConnector { /* Security::PeerConnector API */ bool initialize(Security::SessionPointer &) override; - Security::ContextPointer getTlsContext() override; + Security::FuturePeerContext *peerContext() const override; void noteWantWrite() override; void noteNegotiationError(const Security::ErrorDetailPointer &) override; void noteNegotiationDone(ErrorState *error) override; diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 456f3122ae7..61d8a9e3193 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -27,7 +27,7 @@ BlindPeerConnector::BlindPeerConnector(HttpRequestPointer &, const Comm::Connect {STUB_NOP} bool BlindPeerConnector::initialize(Security::SessionPointer &) STUB_RETVAL(false) -Security::ContextPointer BlindPeerConnector::getTlsContext() STUB_RETVAL(Security::ContextPointer()) +FuturePeerContext *BlindPeerConnector::peerContext() const STUB_RETVAL(nullptr) void BlindPeerConnector::noteNegotiationDone(ErrorState *) STUB } @@ -100,7 +100,6 @@ void PeerConnector::handleNegotiationResult(const Security::IoResult &) STUB; void PeerConnector::noteWantRead() STUB void PeerConnector::noteWantWrite() STUB void PeerConnector::noteNegotiationError(const Security::ErrorDetailPointer &) STUB -// virtual Security::ContextPointer getTlsContext() = 0; void PeerConnector::bail(ErrorState *) STUB void PeerConnector::sendSuccess() STUB void PeerConnector::callBack() STUB @@ -112,6 +111,7 @@ EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer) #include "security/PeerOptions.h" Security::PeerOptions Security::ProxyOutgoingConfig; + Security::PeerOptions::PeerOptions() { #if USE_OPENSSL parsedOptions = 0; @@ -147,7 +147,7 @@ void Security::ServerOptions::updateContextSessionId(Security::ContextPointer &) #include "security/Session.h" namespace Security { -bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false) +bool CreateClientSession(FuturePeerContext &, const Comm::ConnectionPointer &, const char *) STUB_RETVAL(false) bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, Security::PeerOptions &, const char *) STUB_RETVAL(false) void SessionSendGoodbye(const Security::SessionPointer &) STUB bool SessionIsResumed(const Security::SessionPointer &) STUB_RETVAL(false)