From 908634e8c63a33918d7620c8cb44774748b7ebba Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
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 11d7716404..c0b95cdf52 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 5d0addacae..f5c4cceeda 100644
--- a/src/CachePeer.h
+++ b/src/CachePeer.h
@@ -44,6 +44,10 @@ public:
     /// \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 @@ public:
 
     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 8f5063d780..0598a2369b 100644
--- a/src/SquidConfig.h
+++ b/src/SquidConfig.h
@@ -503,6 +503,8 @@ public:
     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 ddefdfefaa..86b05d4aad 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 7afb3851f8..669bac9b9a 100644
--- a/src/adaptation/icap/ServiceRep.h
+++ b/src/adaptation/icap/ServiceRep.h
@@ -112,6 +112,8 @@ public: // treat these as private, they are for callbacks only
     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 d4ea81b654..701e267cf6 100644
--- a/src/adaptation/icap/Xaction.cc
+++ b/src/adaptation/icap/Xaction.cc
@@ -54,9 +54,7 @@ public:
     /* 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 58f69f43f5..90875f9bdd 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 d57a4ea25e..2d45762cb1 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 0001579c7b..57f14e65aa 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 @@ public:
     /// \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 7fdffebf72..2f2e97ff8a 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 b00f3852a8..d9467ebb9b 100644
--- a/src/security/PeerConnector.h
+++ b/src/security/PeerConnector.h
@@ -130,9 +130,9 @@ protected:
     /// \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 e78081d36e..5db3d5a716 100644
--- a/src/security/PeerOptions.h
+++ b/src/security/PeerOptions.h
@@ -147,6 +147,19 @@ public:
     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 0741687367..0530c1ba53 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 28c48fa67d..15bb4940f0 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 107723d946..d63e1a946f 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<ErrorDetail> ErrorDetailPointer;
 
diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index 87aaeb1cbb..235ef50d26 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 1215f407af..82cfec99e4 100644
--- a/src/ssl/PeekingPeerConnector.h
+++ b/src/ssl/PeekingPeerConnector.h
@@ -29,7 +29,7 @@ public:
 
     /* 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 456f3122ae..61d8a9e319 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)
-- 
2.47.2