Fix tls-min-version= being ignored

Audit required change to make PeerOptions::parse() call
parseOptions() when 'options=' altered sslOptions instead of
delaying the parse to context creation.
This missed the fact that for GnuTLS the tlsMinVersion was
also updating the sslOptions string rather than the
parsedOptions variable later in the configuration process.

Call parseOptions() to reset the parsedOptions value whenever
sslOptions string is altered.

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index f685e11d0da16b739849fe9c486042e60ef69262..9165ef99c1c64e1d55a7100dbee6a225ede96386 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -2302,6 +2302,9 @@ parse_peer(CachePeer ** head)
         peerDigestCreate(p);
 #endif
 
+    if (p->secure.encryptTransport)
+        p->secure.parseOptions();
+
     p->index =  ++Config.npeers;
 
     while (*head != NULL)
@@ -3772,6 +3775,7 @@ parsePortCfg(AnyP::PortCfgPointer *head, const char *optionName)
             self_destruct();
             return;
         }
+        s->secure.parseOptions();
     }
 
     // *_port line should now be fully valid so we can clone it if necessary

diff --git a/src/client_side.cc b/src/client_side.cc
index d032c135802651223dd4c29d45c3dc24486f54ce..ba1006dce3d99f8f58d313d0f57c5b34b66ab287 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2522,9 +2522,10 @@ httpAccept(const CommAcceptCbParams &params)
 
 /// Create TLS connection structure and update fd_table
 static bool
-httpsCreate(const Comm::ConnectionPointer &conn, const Security::ContextPointer &ctx)
+httpsCreate(const ConnStateData *connState, const Security::ContextPointer &ctx)
 {
-    if (Security::CreateServerSession(ctx, conn, "client https start")) {
+    const auto conn = connState->clientConnection;
+    if (Security::CreateServerSession(ctx, conn, connState->port->secure, "client https start")) {
         debugs(33, 5, "will negotiate TLS on " << conn);
         return true;
     }
@@ -2709,7 +2710,7 @@ httpsEstablish(ConnStateData *connState, const Security::ContextPointer &ctx)
     assert(connState);
     const Comm::ConnectionPointer &details = connState->clientConnection;
 
-    if (!ctx || !httpsCreate(details, ctx))
+    if (!ctx || !httpsCreate(connState, ctx))
         return;
 
     typedef CommCbMemFunT<ConnStateData, CommTimeoutCbParams> TimeoutDialer;
@@ -3072,7 +3073,7 @@ ConnStateData::getSslContextDone(Security::ContextPointer &ctx)
         }
     }
 
-    if (!httpsCreate(clientConnection, ctx))
+    if (!httpsCreate(this, ctx))
         return;
 
     // bumped intercepted conns should already have Config.Timeout.request set
@@ -3293,7 +3294,7 @@ ConnStateData::startPeekAndSplice()
     Security::ContextPointer unConfiguredCTX(Ssl::createSSLContext(port->secure.signingCa.cert, port->secure.signingCa.pkey, port->secure));
     fd_table[clientConnection->fd].dynamicTlsContext = unConfiguredCTX;
 
-    if (!httpsCreate(clientConnection, unConfiguredCTX))
+    if (!httpsCreate(this, unConfiguredCTX))
         return;
 
     switchedToHttps_ = true;

diff --git a/src/security/PeerOptions.cc b/src/security/PeerOptions.cc
index 13b1e1cff94ad164e5354130712153135ba462b7..51f9c18ef36b69cdf78a413b1f326f5f5899cb33 100644 (file)
--- a/src/security/PeerOptions.cc
+++ b/src/security/PeerOptions.cc
@@ -53,13 +53,14 @@ Security::PeerOptions::parse(const char *token)
         KeyData &t = certs.back();
         t.privateKeyFile = SBuf(token + 4);
     } else if (strncmp(token, "version=", 8) == 0) {
-        debugs(0, DBG_PARSE_NOTE(1), "UPGRADE WARNING: SSL version= is deprecated. Use options= to limit protocols instead.");
+        debugs(0, DBG_PARSE_NOTE(1), "UPGRADE WARNING: SSL version= is deprecated. Use options= and tls-min-version= to limit protocols instead.");
         sslVersion = xatoi(token + 8);
     } else if (strncmp(token, "min-version=", 12) == 0) {
         tlsMinVersion = SBuf(token + 12);
+        optsReparse = true;
     } else if (strncmp(token, "options=", 8) == 0) {
         sslOptions = SBuf(token + 8);
-        parseOptions();
+        optsReparse = true;
     } else if (strncmp(token, "cipher=", 7) == 0) {
         sslCipher = SBuf(token + 7);
     } else if (strncmp(token, "cafile=", 7) == 0) {
@@ -152,37 +153,31 @@ Security::PeerOptions::updateTlsVersionLimits()
     if (!tlsMinVersion.isEmpty()) {
         ::Parser::Tokenizer tok(tlsMinVersion);
         int64_t v = 0;
+        tlsMinOptions.clear();
         if (tok.skip('1') && tok.skip('.') && tok.int64(v, 10, false, 1) && v <= 3) {
             // only account for TLS here - SSL versions are handled by options= parameter
             // avoid affecting options= parameter in cachemgr config report
+            SBuf add;
 #if USE_OPENSSL
-#if SSL_OP_NO_TLSv1
             if (v > 0)
-                parsedOptions |= SSL_OP_NO_TLSv1;
-#endif
-#if SSL_OP_NO_TLSv1_1
+                add.append(":NO_TLSv1");
             if (v > 1)
-                parsedOptions |= SSL_OP_NO_TLSv1_1;
-#endif
-#if SSL_OP_NO_TLSv1_2
+                add.append(":NO_TLSv1_1");
             if (v > 2)
-                parsedOptions |= SSL_OP_NO_TLSv1_2;
-#endif
-
+                add.append(":NO_TLSv1_2");
 #elif USE_GNUTLS
-            // XXX: update parsedOptions directly to avoid polluting 'options=' dumps
-            SBuf add;
             if (v > 0)
                 add.append(":-VERS-TLS1.0");
             if (v > 1)
                 add.append(":-VERS-TLS1.1");
             if (v > 2)
                 add.append(":-VERS-TLS1.2");
+#endif
 
-            if (sslOptions.isEmpty())
+            if (!tlsMinOptions.isEmpty())
                 add.chop(1); // remove the initial ':'
-            sslOptions.append(add);
-#endif
+            tlsMinOptions.append(add);
+            optsReparse = true;
 
         } else {
             debugs(0, DBG_PARSE_NOTE(1), "WARNING: Unknown TLS minimum version: " << tlsMinVersion);
@@ -200,28 +195,28 @@ Security::PeerOptions::updateTlsVersionLimits()
         switch (sslVersion) {
         case 3:
 #if USE_OPENSSL
-            parsedOptions |= (SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_1|SSL_OP_NO_TLSv1_2);
+            add = ":NO_TLSv1:NO_TLSv1_1:NO_TLSv1_2";
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:-VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 4:
 #if USE_OPENSSL
-            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1_1|SSL_OP_NO_TLSv1_2);
+            add = ":NO_SSLv3:NO_TLSv1_1:NO_TLSv1_2";
 #elif USE_GNUTLS
             add = ":+VERS-TLS1.0:-VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 5:
 #if USE_OPENSSL
-            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_2);
+            add = ":NO_SSLv3:NO_TLSv1:NO_TLSv1_2";
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:+VERS-TLS1.1:-VERS-TLS1.2";
 #endif
             break;
         case 6:
 #if USE_OPENSSL
-            parsedOptions |= (SSL_OP_NO_SSLv3|SSL_OP_NO_TLSv1|SSL_OP_NO_TLSv1_1);
+            add = ":NO_SSLv3:NO_TLSv1:NO_TLSv1_1";
 #elif USE_GNUTLS
             add = ":-VERS-TLS1.0:-VERS-TLS1.1";
 #endif
@@ -230,12 +225,11 @@ Security::PeerOptions::updateTlsVersionLimits()
             break;
         }
         if (add) {
-#if USE_GNUTLS // do not bother otherwise
             if (sslOptions.isEmpty())
                 sslOptions.append(add+1, strlen(add+1));
             else
                 sslOptions.append(add, strlen(add));
-#endif
+            optsReparse = true;
         }
         sslVersion = 0; // prevent sslOptions being repeatedly appended
     }
@@ -390,16 +384,22 @@ static struct ssl_option {
     {
         "NO_TLSv1", SSL_OP_NO_TLSv1
     },
+#else
+    { "NO_TLSv1", 0 },
 #endif
 #if SSL_OP_NO_TLSv1_1
     {
         "NO_TLSv1_1", SSL_OP_NO_TLSv1_1
     },
+#else
+    { "NO_TLSv1_1", 0 },
 #endif
 #if SSL_OP_NO_TLSv1_2
     {
         "NO_TLSv1_2", SSL_OP_NO_TLSv1_2
     },
+#else
+    { "NO_TLSv1_2", 0 },
 #endif
 #if SSL_OP_NO_COMPRESSION
     {
@@ -432,8 +432,20 @@ static struct ssl_option {
 void
 Security::PeerOptions::parseOptions()
 {
+    // do not allow repeated parsing when multiple contexts are created
+    // NP: we cannot use !parsedOptions because a nil value does have meaning there
+    if (!optsReparse)
+        return;
+    optsReparse = false;
+
+    // combination of settings we have to set via parsedOptions.
+    // options= with override by tls-min-version=
+    SBuf str;
+    str.append(sslOptions);
+    str.append(tlsMinOptions);
+
 #if USE_OPENSSL
-    ::Parser::Tokenizer tok(sslOptions);
+    ::Parser::Tokenizer tok(str);
     long op = 0;
 
     while (!tok.atEnd()) {
@@ -498,16 +510,17 @@ Security::PeerOptions::parseOptions()
     parsedOptions = op;
 
 #elif USE_GNUTLS
-    if (sslOptions.isEmpty()) {
+    if (str.isEmpty()) {
         parsedOptions.reset();
         return;
     }
 
     const char *err = nullptr;
-    const char *priorities = sslOptions.c_str();
+    const char *priorities = str.c_str();
     gnutls_priority_t op;
-    if (gnutls_priority_init(&op, priorities, &err) != GNUTLS_E_SUCCESS) {
-        fatalf("Unknown TLS option '%s'", err);
+    int x = gnutls_priority_init(&op, priorities, &err);
+    if (x != GNUTLS_E_SUCCESS) {
+        fatalf("(%s) in TLS options '%s'", ErrorString(x), err);
     }
     parsedOptions = Security::ParsedOptions(op, [](gnutls_priority_t p) {
         debugs(83, 5, "gnutls_priority_deinit p=" << (void*)p);
@@ -591,12 +604,13 @@ Security::PeerOptions::loadCrlFile()
 }
 
 void
-Security::PeerOptions::updateContextOptions(Security::ContextPointer &ctx) const
+Security::PeerOptions::updateContextOptions(Security::ContextPointer &ctx)
 {
+    parseOptions();
 #if USE_OPENSSL
     SSL_CTX_set_options(ctx.get(), parsedOptions);
 #elif USE_GNUTLS
-    // NP: GnuTLS uses 'priorities' which are set per-session instead.
+    // NP: GnuTLS uses 'priorities' which are set only per-session instead.
 #endif
 }
 
@@ -723,8 +737,12 @@ Security::PeerOptions::updateContextTrust(Security::ContextPointer &ctx)
 void
 Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s)
 {
+    parseOptions();
 #if USE_OPENSSL
-    // 'options=' value being set to session is a GnuTLS specific thing.
+    debugs(83, 5, "set OpenSSL options for session=" << s << ", parsedOptions=" << parsedOptions);
+    // XXX: Options already set before (via the context) are not cleared!
+    SSL_set_options(s.get(), parsedOptions);
+
 #elif USE_GNUTLS
     int x;
     SBuf errMsg;
@@ -734,13 +752,13 @@ Security::PeerOptions::updateSessionOptions(Security::SessionPointer &s)
         static const SBuf defaults("default");
         errMsg = defaults;
     } else {
-        debugs(83, 5, "set GnuTLS options '" << sslOptions << "' for session=" << s);
+        debugs(83, 5, "set GnuTLS session=" << s << ", options='" << sslOptions << ":" << tlsMinOptions << "'");
         x = gnutls_priority_set(s.get(), parsedOptions.get());
         errMsg = sslOptions;
     }
 
     if (x != GNUTLS_E_SUCCESS) {
-        debugs(83, DBG_IMPORTANT, "ERROR: Failed to set TLS options (" << errMsg << "). error: " << Security::ErrorString(x));
+        debugs(83, DBG_IMPORTANT, "ERROR: session=" << s << " Failed to set TLS options (" << errMsg << ":" << tlsMinVersion << "). error: " << Security::ErrorString(x));
     }
 #endif
 }
@@ -750,5 +768,6 @@ parse_securePeerOptions(Security::PeerOptions *opt)
 {
     while(const char *token = ConfigParser::NextToken())
         opt->parse(token);
+    opt->parseOptions();
 }
 

diff --git a/src/security/PeerOptions.h b/src/security/PeerOptions.h
index 43da1afec187fd9ef5c7949c620f64709cb314a2..928295b0afafc8b79aedf4484ec4c5a34145f9af 100644 (file)
--- a/src/security/PeerOptions.h
+++ b/src/security/PeerOptions.h
@@ -32,6 +32,9 @@ public:
     /// parse a TLS squid.conf option
     virtual void parse(const char *);
 
+    /// parse and verify the [tls-]options= string in sslOptions
+    void parseOptions();
+
     /// reset the configuration details to default
     virtual void clear() {*this = PeerOptions();}
 
@@ -45,7 +48,7 @@ public:
     void updateTlsVersionLimits();
 
     /// Setup the library specific 'options=' parameters for the given context.
-    void updateContextOptions(Security::ContextPointer &) const;
+    void updateContextOptions(Security::ContextPointer &);
 
     /// setup the NPN extension details for the given context
     void updateContextNpn(Security::ContextPointer &);
@@ -66,7 +69,6 @@ public:
     virtual void dumpCfg(Packable *, const char *pfx) const;
 
 private:
-    void parseOptions(); ///< parsed value of sslOptions
     long parseFlags();
     void loadCrlFile();
     void loadKeysFile();
@@ -82,7 +84,19 @@ public:
 
     SBuf tlsMinVersion;  ///< version label for minimum TLS version to permit
 
-    Security::ParsedOptions parsedOptions; ///< parsed value of sslOptions
+private:
+    /// Library-specific options string generated from tlsMinVersion.
+    /// Call updateTlsVersionLimits() to regenerate this string.
+    SBuf tlsMinOptions;
+
+    /// Parsed value of sslOptions + tlsMinOptions settings.
+    /// Set optsReparse=true to have this re-parsed before next use.
+    Security::ParsedOptions parsedOptions;
+
+    /// whether parsedOptions content needs to be regenerated
+    bool optsReparse = true;
+
+public:
     long parsedFlags = 0;   ///< parsed value of sslFlags
 
     std::list<Security::KeyData> certs; ///< details from the cert= and file= config parameters
@@ -93,13 +107,15 @@ protected:
     template<typename T>
     Security::ContextPointer convertContextFromRawPtr(T ctx) const {
 #if USE_OPENSSL
+        debugs(83, 5, "SSL_CTX construct, this=" << (void*)ctx);
         return ContextPointer(ctx, [](SSL_CTX *p) {
-            debugs(83, 5, "SSL_free ctx=" << (void*)p);
+            debugs(83, 5, "SSL_CTX destruct, this=" << (void*)p);
             SSL_CTX_free(p);
         });
 #elif USE_GNUTLS
+        debugs(83, 5, "gnutls_certificate_credentials construct, this=" << (void*)ctx);
         return Security::ContextPointer(ctx, [](gnutls_certificate_credentials_t p) {
-            debugs(83, 5, "gnutls_certificate_free_credentials ctx=" << (void*)p);
+            debugs(83, 0, "gnutls_certificate_credentials destruct this=" << (void*)p);
             gnutls_certificate_free_credentials(p);
         });
 #else

diff --git a/src/security/Session.cc b/src/security/Session.cc
index 9a9d90904c5f261f34e17215868054ef293aba19..6698d26457b58e1fe0cfeb9941d7326436481e03 100644 (file)
--- a/src/security/Session.cc
+++ b/src/security/Session.cc
@@ -106,7 +106,7 @@ Security::NewSessionObject(const Security::ContextPointer &ctx)
 #endif
 
 static bool
-CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &conn, Security::Io::Type type, const char *squidCtx)
+CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &conn, Security::PeerOptions &opts, Security::Io::Type type, const char *squidCtx)
 {
     if (!Comm::IsConnOpen(conn)) {
         debugs(83, DBG_IMPORTANT, "Gone connection");
@@ -122,6 +122,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
     if (!session) {
         errCode = ERR_get_error();
         errAction = "failed to allocate handle";
+        debugs(83, DBG_IMPORTANT, "TLS error: " << errAction << ": " << Security::ErrorString(errCode));
     }
 #elif USE_GNUTLS
     gnutls_session_t tmp;
@@ -134,6 +135,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
     if (errCode != GNUTLS_E_SUCCESS) {
         session.reset();
         errAction = "failed to initialize session";
+        debugs(83, DBG_IMPORTANT, "TLS error: " << errAction << ": " << Security::ErrorString(errCode));
     }
 #endif
 
@@ -148,10 +150,7 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
         errCode = gnutls_credentials_set(session.get(), GNUTLS_CRD_CERTIFICATE, ctx.get());
         if (errCode == GNUTLS_E_SUCCESS) {
 
-            if (auto *peer = conn->getPeer())
-                peer->secure.updateSessionOptions(session);
-            else
-                Security::ProxyOutgoingConfig.updateSessionOptions(session);
+            opts.updateSessionOptions(session);
 
             // NP: GnuTLS does not yet support the BIO operations
             //     this does the equivalent of SSL_set_fd() for now.
@@ -184,13 +183,17 @@ CreateSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer
 bool
 Security::CreateClientSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx)
 {
-    return CreateSession(ctx, c, Security::Io::BIO_TO_SERVER, 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);
 }
 
 bool
-Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, const char *squidCtx)
+Security::CreateServerSession(const Security::ContextPointer &ctx, const Comm::ConnectionPointer &c, Security::PeerOptions &o, const char *squidCtx)
 {
-    return CreateSession(ctx, c, Security::Io::BIO_TO_CLIENT, squidCtx);
+    return CreateSession(ctx, c, o, Security::Io::BIO_TO_CLIENT, squidCtx);
 }
 
 void

diff --git a/src/security/Session.h b/src/security/Session.h
index 723b9b83c0114f2c98cf077c41eaf638ad10793c..7ba40d89d70b75117f37120e2746ef341b7376c9 100644 (file)
--- a/src/security/Session.h
+++ b/src/security/Session.h
@@ -34,9 +34,11 @@ namespace Security {
 /// On errors, emits DBG_IMPORTANT with details and returns false.
 bool CreateClientSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx);
 
+class PeerOptions;
+
 /// Creates TLS Server 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 CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, const char *squidCtx);
+bool CreateServerSession(const Security::ContextPointer &, const Comm::ConnectionPointer &, Security::PeerOptions &, const char *squidCtx);
 
 #if USE_OPENSSL
 typedef std::shared_ptr<SSL> SessionPointer;

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index 8531398e539391abea3f90fb341de5f9b17b2213..1a6f996c3da6d29e451f3ffdf2ea2a57caf20239 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -185,7 +185,7 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
             srvBio->mode(csd->sslBumpMode);
         } else {
             // Set client SSL options
-            SSL_set_options(serverSession.get(), ::Security::ProxyOutgoingConfig.parsedOptions);
+            ::Security::ProxyOutgoingConfig.updateSessionOptions(serverSession);
 
             const bool redirected = request->flags.redirected && ::Config.onoff.redir_rewrites_host;
             const char *sniServer = (!hostName || redirected) ?

diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc
index a917ecbaf24cd162aadf878a9f36259698261dcf..07ee576ce72e0044076f67b1b0159dc37c7cb658 100644 (file)
--- a/src/tests/stub_libsecurity.cc
+++ b/src/tests/stub_libsecurity.cc
@@ -110,7 +110,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 CreateServerSession(const Security::ContextPointer &, 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)
 void MaybeGetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &) STUB