From 32151ebded1afd840e8fdee00b36af7a0d08795f Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 10 Nov 2018 17:00:12 +1300 Subject: [PATCH] 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. --- src/cache_cf.cc | 4 ++ src/client_side.cc | 11 +++-- src/security/PeerOptions.cc | 85 ++++++++++++++++++++------------- src/security/PeerOptions.h | 26 ++++++++-- src/security/Session.cc | 19 ++++---- src/security/Session.h | 4 +- src/ssl/PeekingPeerConnector.cc | 2 +- src/tests/stub_libsecurity.cc | 2 +- 8 files changed, 99 insertions(+), 54 deletions(-) diff --git a/src/cache_cf.cc b/src/cache_cf.cc index f685e11d0d..9165ef99c1 100644 --- 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 d032c13580..ba1006dce3 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2522,9 +2522,10 @@ httpAccept(const CommAcceptCbParams ¶ms) /// 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 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 13b1e1cff9..51f9c18ef3 100644 --- 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 43da1afec1..928295b0af 100644 --- 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 certs; ///< details from the cert= and file= config parameters @@ -93,13 +107,15 @@ protected: template 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 9a9d90904c..6698d26457 100644 --- 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 723b9b83c0..7ba40d89d7 100644 --- 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 SessionPointer; diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 8531398e53..1a6f996c3d 100644 --- 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 a917ecbaf2..07ee576ce7 100644 --- 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 -- 2.47.3