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().

diff --git a/src/CachePeer.cc b/src/CachePeer.cc
index 11d77164045889547a0b36c87c2f9e89c27e1d27..c0b95cdf527113ca202e66f860fee32bbaa9ac6d 100644 (file)
--- 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 5d0addacae41fa58c7c64f6399aa079ebedecc6f..f5c4cceedafacefabf180582703720de8e7135ad 100644 (file)
--- 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 8f5063d7802a6fa995f36d94ec82f0a3470a50a4..0598a2369b618eab22c0e615f26f08d481eb53d9 100644 (file)
--- 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 ddefdfefaafeab94964c502fe6a38aa489c30c8c..86b05d4aadb145834338000d4bd0dc5d781601e3 100644 (file)
--- 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 7afb3851f8ffd037e455cb9ab558e850258c401b..669bac9b9a9f8a35ecacf519f3c1424017d06e6f 100644 (file)
--- 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 d4ea81b65444090a21780f4822599babf102e095..701e267cf64b44eeffdc37259e031ae9dea0ed64 100644 (file)
--- 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 58f69f43f54e497a226d4940d7351cad120f1eb7..90875f9bdde386c639ab13508c8bab0b01c5f90c 100644 (file)
--- 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 d57a4ea25e58cee023c65bbae72a1839fde41b3a..2d45762cb15fbc0f68aaaadc4ccc2cbde57fa6d0 100644 (file)
--- 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 0001579c7bf5eb0c1c312693deb64cbc3f5eacc6..57f14e65aa14b84213742d270bd9b68acfa3c4b8 100644 (file)
--- 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 7fdffebf7214dd6fd75ae0cb60056b58278a3415..2f2e97ff8a87ebb351037ad4d0a759273443b0f8 100644 (file)
--- 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 b00f3852a85aa2b40085a01b70ed3fb594e4dab7..d9467ebb9b850ea6599c990affad7f81f57f711e 100644 (file)
--- 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 e78081d36efd65c0b0abe83721725bd098d956b6..5db3d5a7167100f3a6a95551e3e7f363d40e39f5 100644 (file)
--- 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 07416873674c1da40f2072a858060a80d383f65d..0530c1ba531f0d2b9628998f011a6c226c9ba86a 100644 (file)
--- 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 28c48fa67d0d4ead60c6f9be1ccc4854b6aef06a..15bb4940f07e2c56a1cbfd69dc8a49d61ccfdcb7 100644 (file)
--- 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 107723d9461be92425705689da2e06da8b3b9f5c..d63e1a946fe9aca5d4a059f5b750435553fc820c 100644 (file)
--- 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 87aaeb1cbb2782a4e7fb1bbecabc18af2cd92076..235ef50d26f8b539e7d6ff8a5d5a36677a2e5d50 100644 (file)
--- 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 1215f407af7d00ed4e26eda338a0672bb797aa2a..82cfec99e4857ef736a04cb318091673ce3d1fed 100644 (file)
--- 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 456f3122ae710f96eb6e59045dbab2a1f7e44ea1..61d8a9e31935f99524f8ca4c85f08e09e8286cff 100644 (file)
--- 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)