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

@@ -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()
 {

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

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;

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),

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:

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 */

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

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

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.

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
 

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; }

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;
 

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

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;
 

src/security/forward.h

@@ -209,6 +209,8 @@ class PeerOptions;
 
 class ServerOptions;
 
+class FuturePeerContext;
+
 class ErrorDetail;
 typedef RefCount<ErrorDetail> ErrorDetailPointer;
 

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

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;

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)