From: Christos Tsantilas Date: Mon, 17 Aug 2015 07:16:17 +0000 (+0300) Subject: Ignore impossible SSL bumping actions, as intended and documented. X-Git-Tag: SQUID_4_0_1~129 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=640fe8fbc5bd48c127f8641ee340d7904e99236b;p=thirdparty%2Fsquid.git Ignore impossible SSL bumping actions, as intended and documented. According to Squid wiki: "Some actions are not possible during certain processing steps. During a given processing step, Squid ignores ssl_bump lines with impossible actions". The distributed squid.conf.documented has similar text. Current Squid violates the above rule. Squid considers all actions, and if an impossible action matches first, Squid guesses what the true configuration intent was. Squid may guess wrong. For example, depending on the transaction, Squid may guess that a matching stare or peek action during bumping step3 means "bump", breaking peeked connections that cannot be bumped. This unintended but gross configuration semantics violation remained invisible until bug 4237, probably because most configurations in most environments either worked around the problem (where admins experimented to "make it work") or did not result in visible errors (where Squid guesses did not lead to terminated connections). While configuration workarounds are possible, the current implementation is very wrong and leads to overly complex and, hence, often wrong configurations. It is also nearly impossible to document accurately because the guessing logic depends on too many factors. To fix this, we add an action filtering/banning mechanism to Squid ACL code. This mechanism is then used to: - ban client-first and server-first on bumping steps 2 and 3. - ban peek and stare actions on bumping step 3. - ban splice on step3 if stare is selected on step2 and Squid cannot splice the SSL connection any more. - ban bump on step3 if peek is selected on step2 and Squid cannot bump the connection any more. The same action filtering mechanism may be useful for other ACL-driven directives with state-dependent custom actions. This change adds a runtime performance overhead of a single virtual method call to all ORed ACLs that do not use banned actions. That method itself just returns false unless the ACL represents a whole directive rule. In the latter case, an std::vector size() is also checked. It is possible to avoid this overhead by adding a boolean "I may ban actions" flag to Acl::OrNode, but we decided the small performance harm is not worth the extra code to set that flag. This is a Measurement Factory project. --- diff --git a/src/acl/Acl.h b/src/acl/Acl.h index 70831062bc..bedb4d50d3 100644 --- a/src/acl/Acl.h +++ b/src/acl/Acl.h @@ -166,7 +166,7 @@ class allow_t { public: // not explicit: allow "aclMatchCode to allow_t" conversions (for now) - allow_t(const aclMatchCode aCode): code(aCode), kind(0) {} + allow_t(const aclMatchCode aCode, int aKind = 0): code(aCode), kind(aKind) {} allow_t(): code(ACCESS_DUNNO), kind(0) {} @@ -178,6 +178,10 @@ public: return !(*this == aCode); } + bool operator ==(const allow_t allow) const { + return code == allow.code && kind == allow.kind; + } + operator aclMatchCode() const { return code; } diff --git a/src/acl/BoolOps.cc b/src/acl/BoolOps.cc index 0e838024bf..8a9b63d8b3 100644 --- a/src/acl/BoolOps.cc +++ b/src/acl/BoolOps.cc @@ -115,6 +115,12 @@ Acl::OrNode::clone() const return new OrNode; } +bool +Acl::OrNode::bannedAction(ACLChecklist *, Nodes::const_iterator) const +{ + return false; +} + int Acl::OrNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const { @@ -122,6 +128,8 @@ Acl::OrNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const // find the first node that matches, but stop if things go wrong for (Nodes::const_iterator i = start; i != nodes.end(); ++i) { + if (bannedAction(checklist, i)) + continue; if (checklist->matchChild(this, i, *i)) { lastMatch_ = i; return 1; diff --git a/src/acl/BoolOps.h b/src/acl/BoolOps.h index bd5271d75f..085537a76e 100644 --- a/src/acl/BoolOps.h +++ b/src/acl/BoolOps.h @@ -62,6 +62,10 @@ class OrNode: public InnerNode MEMPROXY_CLASS(OrNode); public: + /// whether the given rule should be excluded from matching tests based + /// on its action + virtual bool bannedAction(ACLChecklist *, Nodes::const_iterator) const; + /* ACL API */ virtual char const *typeString() const; virtual ACL *clone() const; diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index 9179adfe15..b48f4b8f9f 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -14,6 +14,8 @@ #include "Debug.h" #include "profiler/Profiler.h" +#include + /// common parts of nonBlockingCheck() and resumeNonBlockingCheck() bool ACLChecklist::prepNonBlocking() @@ -391,3 +393,17 @@ ACLChecklist::callerGone() return !cbdataReferenceValid(callback_data); } +bool +ACLChecklist::bannedAction(const allow_t &action) const +{ + const bool found = std::find(bannedActions_.begin(), bannedActions_.end(), action) != bannedActions_.end(); + debugs(28, 5, "Action '" << action << "/" << action.kind << (found ? " is " : "is not") << " banned"); + return found; +} + +void +ACLChecklist::banAction(const allow_t &action) +{ + bannedActions_.push_back(action); +} + diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 4d6afbaacf..837aa0c689 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -11,6 +11,7 @@ #include "acl/InnerNode.h" #include +#include /// ACL checklist callback typedef void ACLCB(allow_t, void *); @@ -152,6 +153,11 @@ public: const allow_t ¤tAnswer() const { return allow_; } + /// whether the action is banned or not + bool bannedAction(const allow_t &action) const; + /// add action to the list of banned actions + void banAction(const allow_t &action); + // XXX: ACLs that need request or reply have to use ACLFilledChecklist and // should do their own checks so that we do not have to povide these two // for ACL::checklistMatches to use @@ -217,6 +223,8 @@ private: /* internal methods */ /// suspended (due to an async lookup) matches() in the ACL tree std::stack matchPath; + /// the list of actions which must ignored during acl checks + std::vector bannedActions_; }; #endif /* SQUID_ACLCHECKLIST_H */ diff --git a/src/acl/Tree.cc b/src/acl/Tree.cc index d1b5b7fe29..bb98c4aed5 100644 --- a/src/acl/Tree.cc +++ b/src/acl/Tree.cc @@ -7,6 +7,7 @@ */ #include "squid.h" +#include "acl/Checklist.h" #include "acl/Tree.h" #include "wordlist.h" @@ -85,3 +86,13 @@ Acl::Tree::treeDump(const char *prefix, const ActionToString &convert) const return text; } +bool +Acl::Tree::bannedAction(ACLChecklist *checklist, Nodes::const_iterator node) const +{ + if (actions.size()) { + assert(actions.size() == nodes.size()); + const Nodes::size_type pos = node - nodes.begin(); + return checklist->bannedAction(actions.at(pos)); + } + return false; +} diff --git a/src/acl/Tree.h b/src/acl/Tree.h index 0bd9e15dfb..c13e7afdfb 100644 --- a/src/acl/Tree.h +++ b/src/acl/Tree.h @@ -40,6 +40,8 @@ public: void add(ACL *rule); ///< same as InnerNode::add() protected: + /// Acl::OrNode API + virtual bool bannedAction(ACLChecklist *, Nodes::const_iterator) const override; allow_t actionAt(const Nodes::size_type pos) const; /// if not empty, contains actions corresponding to InnerNode::nodes diff --git a/src/client_side.cc b/src/client_side.cc index 5bbf755757..d21fa51e2b 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -4198,12 +4198,7 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data) assert(connState->serverBump()); Ssl::BumpMode bumpAction; if (answer == ACCESS_ALLOWED) { - if (answer.kind == Ssl::bumpNone) - bumpAction = Ssl::bumpSplice; - else if (answer.kind == Ssl::bumpClientFirst || answer.kind == Ssl::bumpServerFirst) - bumpAction = Ssl::bumpBump; - else - bumpAction = (Ssl::BumpMode)answer.kind; + bumpAction = (Ssl::BumpMode)answer.kind; } else bumpAction = Ssl::bumpSplice; @@ -4264,6 +4259,9 @@ ConnStateData::startPeekAndSpliceDone() ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw(), NULL); //acl_checklist->src_addr = params.conn->remote; //acl_checklist->my_addr = s->s; + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst)); acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this); return; } diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index 10c7ed6e47..4466805c1d 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -231,6 +231,18 @@ Ssl::PeekingPeerConnector::checkForPeekAndSplice() ACLFilledChecklist *acl_checklist = new ACLFilledChecklist( ::Config.accessList.ssl_bump, request.getRaw(), NULL); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpPeek)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpStare)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst)); + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst)); + SSL *ssl = fd_table[serverConn->fd].ssl; + BIO *b = SSL_get_rbio(ssl); + Ssl::ServerBio *srvBio = static_cast(b->ptr); + if (!srvBio->canSplice()) + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpSplice)); + if (!srvBio->canBump()) + acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpBump)); acl_checklist->nonBlockingCheck(Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone, this); } @@ -243,15 +255,7 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(Ssl::BumpMode const action) debugs(83,5, "Will check for peek and splice on FD " << serverConn->fd); Ssl::BumpMode finalAction = action; - // adjust the final bumping mode if needed - if (finalAction < Ssl::bumpSplice) - finalAction = Ssl::bumpBump; - - if (finalAction == Ssl::bumpSplice && !srvBio->canSplice()) - finalAction = Ssl::bumpBump; - else if (finalAction == Ssl::bumpBump && !srvBio->canBump()) - finalAction = Ssl::bumpSplice; - + Must(finalAction == Ssl::bumpSplice || finalAction == Ssl::bumpBump || finalAction == Ssl::bumpTerminate); // Record final decision if (request->clientConnectionManager.valid()) { request->clientConnectionManager->sslBumpMode = finalAction;