[squid-dev] [PATCH] Ignore impossible SSL bumping actions, as intended and documented / bug 4237 fix
Amos Jeffries
squid3 at treenet.co.nz
Tue Aug 11 04:30:04 UTC 2015
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
More information about the squid-dev
mailing list