------------------------------------------------------------ revno: 13893 revision-id: squid3@treenet.co.nz-20150821003155-po0yyrs6a9p50dzv parent: squid3@treenet.co.nz-20150820235718-k9ijbnrwujvz8ndn author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Thu 2015-08-20 17:31:55 -0700 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20150821003155-po0yyrs6a9p50dzv # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 213ee6c97abb33911f0841b0523819e9b3920d9e # timestamp: 2015-08-21 00:51:01 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20150820235718-\ # k9ijbnrwujvz8ndn # # Begin patch === modified file 'src/acl/Acl.h' --- src/acl/Acl.h 2015-01-13 09:13:49 +0000 +++ src/acl/Acl.h 2015-08-21 00:31:55 +0000 @@ -167,7 +167,7 @@ { 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) {} @@ -179,6 +179,10 @@ return !(*this == aCode); } + bool operator ==(const allow_t allow) const { + return code == allow.code && kind == allow.kind; + } + operator aclMatchCode() const { return code; } === modified file 'src/acl/BoolOps.cc' --- src/acl/BoolOps.cc 2015-01-13 09:13:49 +0000 +++ src/acl/BoolOps.cc 2015-08-21 00:31:55 +0000 @@ -115,6 +115,12 @@ 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 @@ // 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; === modified file 'src/acl/BoolOps.h' --- src/acl/BoolOps.h 2015-01-13 09:13:49 +0000 +++ src/acl/BoolOps.h 2015-08-21 00:31:55 +0000 @@ -64,6 +64,10 @@ public: MEMPROXY_CLASS(OrNode); + /// 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; === modified file 'src/acl/Checklist.cc' --- src/acl/Checklist.cc 2015-01-13 09:13:49 +0000 +++ src/acl/Checklist.cc 2015-08-21 00:31:55 +0000 @@ -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 @@ 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); +} + === modified file 'src/acl/Checklist.h' --- src/acl/Checklist.h 2015-01-13 09:13:49 +0000 +++ src/acl/Checklist.h 2015-08-21 00:31:55 +0000 @@ -11,6 +11,7 @@ #include "acl/InnerNode.h" #include +#include /// ACL checklist callback typedef void ACLCB(allow_t, void *); @@ -152,6 +153,11 @@ 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 @@ /// 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 */ === modified file 'src/acl/Tree.cc' --- src/acl/Tree.cc 2015-01-13 09:13:49 +0000 +++ src/acl/Tree.cc 2015-08-21 00:31:55 +0000 @@ -7,6 +7,7 @@ */ #include "squid.h" +#include "acl/Checklist.h" #include "acl/Tree.h" #include "wordlist.h" @@ -81,3 +82,14 @@ 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; +} + === modified file 'src/acl/Tree.h' --- src/acl/Tree.h 2015-01-13 09:13:49 +0000 +++ src/acl/Tree.h 2015-08-21 00:31:55 +0000 @@ -36,6 +36,8 @@ void add(ACL *rule); ///< same as InnerNode::add() protected: + /// Acl::OrNode API + virtual bool bannedAction(ACLChecklist *, Nodes::const_iterator) const; allow_t actionAt(const Nodes::size_type pos) const; /// if not empty, contains actions corresponding to InnerNode::nodes === modified file 'src/client_side.cc' --- src/client_side.cc 2015-08-20 13:42:51 +0000 +++ src/client_side.cc 2015-08-21 00:31:55 +0000 @@ -4320,12 +4320,7 @@ 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; @@ -4375,6 +4370,9 @@ 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; } === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2015-08-01 05:59:31 +0000 +++ src/ssl/PeerConnector.cc 2015-08-21 00:31:55 +0000 @@ -388,6 +388,18 @@ 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::PeerConnector::cbCheckForPeekAndSpliceDone, this); } @@ -400,15 +412,7 @@ 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;