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 572dc8f485aefe3466af35adf4df1a249fc592da..9b6680f0efaf5c42babcf4c033daf2c37ea255ef 100644 (file)
--- a/src/acl/Acl.h
+++ b/src/acl/Acl.h
@@ -167,7 +167,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) {}
 
@@ -179,6 +179,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 703c73e63814bf99e938dae972230111b98db3dd..dc664cbeba94045a63e47fffa4e92a37643f3fea 100644 (file)
--- a/src/acl/BoolOps.h
+++ b/src/acl/BoolOps.h
@@ -64,6 +64,10 @@ class OrNode: public InnerNode
 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;

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 3aaf6f560bf6fb6b3f69a6fd915c1f376bf1cb2c..89429876f76f16750f14f31617506a48b2da9980 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"
 
@@ -81,3 +82,14 @@ 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 0b943aae2e67fc8a0e10c7e0ccdba8ea80b4a9a2..ee343ddc80a859b8567c710589a1d75024bf56fb 100644 (file)
--- a/src/acl/Tree.h
+++ b/src/acl/Tree.h
@@ -36,6 +36,8 @@ public:
     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

diff --git a/src/client_side.cc b/src/client_side.cc
index 7544d820b3b4610a9c6353dd3a60101f7f52bbb1..01f668305dee75f80b515c361873bbaa829a64e6 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -4320,12 +4320,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;
 
@@ -4375,6 +4370,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 e20b3cca7b74fc0ad6a9dc4e75f74d4503a70e13..8abae3fb67a09205743bbdd0f4ef7dfeb384b285 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -388,6 +388,18 @@ Ssl::PeerConnector::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::PeerConnector::cbCheckForPeekAndSpliceDone, this);
 }
 
@@ -400,15 +412,7 @@ Ssl::PeerConnector::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;