[squid-dev] [PATCH] Ignore impossible SSL bumping actions, as intended and documented / bug 4237 fix

[Amos Jeffries]

On 11/08/2015 3:54 a.m., Tsantilas Christos wrote:
> 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).
> 

... and mind this mess and admin confusion is a direct (and predicted)
result of conflating one single access control with all of the TLS
related authentication + authorization + processing control logics.

Thanks for doing this patch anyway. Adjusting allow_t like this has been
on the TODO list for auth related issues a long time before ssl-bump
existed.


> 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.

How about we just remove of client-first entirely?
 It has major security issues for which CVE already exist. The one
remaining use-case AFAICT is malware using Squid.

The attempted seamless upgrade from old configs was an outright failure.
So I think we can shorten the deprecation time without additional pain.


>   - 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.
> 

What about the other documented actions:
 * "reconnect" at step 1 & 2
 * "none" at step 2 and 3

(<http://wiki.squid-cache.org/Features/SslPeekAndSplice>)

> The same action filtering mechanism may be useful for other ACL-driven
> directives with state-dependent custom actions.

So far that is only AUTH_REQUIRED on all non-http_access directives. And
DUNNO results on fast-check access controls.

The former would be a good (quick?) followup patch. The latter will
require careful testing and documentation.

> 
> 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.

Agreed.


So the audit:

in src/acl/Tree.cc:

* src/Checklist.h and include/Checklist.h do not exist.
 - did you mean #include "acl/Checklist.h" ?


in src/cache_cf.cc:

* revert all changes

NP: you may also want to test how ssl_bump works when the admin has
configures "tls_outgoing_options disable" or a min-version value higher
than the server will accept.


in src/client_side.cc:

* not banning other non-available options (see comment above about none
and reconnect).

* looks like httpsSslBumpStep2AccessCheckDone() is still making bad
assumptions about "none" action == "splice". When its documented as
being not available at step 2 & 3.
 - also reconnect action is actually performed? at step2 when documented
as not available until step3.


in src/ssl/PeerConnector.cc:

* Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone() is another
point doing none == splice when PeekingConnector is != step1 right?
being a server/peer job it should only be step2 or step3.

* the Must() will throw on receiving final action "reconnect" which is
documented as being most relevant here on peer connections.
 - if that is actually right for ths bit of code please add a comment to
document why.

Amos