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 70831062bc9466091232f6ccea262d915dbdb1f1..bedb4d50d37a04df0e8737910eb95901e9cf95ec 100644 (file)
--- 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 0e838024bfc6995767c5c9e7da05d034c909ba13..8a9b63d8b39ed56f5027afa091dfd9df1d15d069 100644 (file)
--- 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 bd5271d75ffb64c548364597f79acbbc8eb90df3..085537a76e44dbc8426e9342ddae40d9716c6085 100644 (file)
--- 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 9179adfe154728f2e4f13f71a0da9f43d9558743..b48f4b8f9fc498c88e4a4a7b84e2937ac2191b06 100644 (file)
--- a/src/acl/Checklist.cc
+++ b/src/acl/Checklist.cc
@@ -14,6 +14,8 @@
 #include "Debug.h"
 #include "profiler/Profiler.h"
 
+#include <algorithm>
+
 /// 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 4d6afbaacf21687c9728d81dccc3e23ba9b596c8..837aa0c6899b25bdbd3229473e09d472e62010b8 100644 (file)
--- a/src/acl/Checklist.h
+++ b/src/acl/Checklist.h
@@ -11,6 +11,7 @@
 
 #include "acl/InnerNode.h"
 #include <stack>
+#include <vector>
 
 /// ACL checklist callback
 typedef void ACLCB(allow_t, void *);
@@ -152,6 +153,11 @@ public:
 
     const allow_t &currentAnswer() 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<Breadcrumb> matchPath;
+    /// the list of actions which must ignored during acl checks
+    std::vector<allow_t> bannedActions_;
 };
 
 #endif /* SQUID_ACLCHECKLIST_H */

diff --git a/src/acl/Tree.cc b/src/acl/Tree.cc
index d1b5b7fe29f7c0f5859893c508172e28757ed309..bb98c4aed594993027c7016c4791253714d3c441 100644 (file)
--- 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 0bd9e15dfb46251ddd56d8671e1a3f29c80bdb85..c13e7afdfbfd9f9ee4379ab14a1f2fe18811666f 100644 (file)
--- 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 5bbf755757f468bafb63c2209ef14cdcdcf2ea1b..d21fa51e2b90e13f358702d03aa5ff78ae94375a 100644 (file)
--- 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 10c7ed6e47cf46067d392e8680a96b6d336dbc2d..4466805c1dad9a1539b7aadbff01385a1d8f74d5 100644 (file)
--- 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<Ssl::ServerBio *>(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;