On 08/11/2015 07:30 AM, Amos Jeffries wrote:
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.
If we decide to remove client-first, we should implement a separate
patch. It has some work.
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
The reconnect is not yet implemented.
About the "none" you are right it should handled.
(<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" ?
fixed.
in src/cache_cf.cc:
* revert all changes
oops, sorry, forgot to remove before post the patch...
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.
Squid does not start because of FATAL:
"FATAL: No valid signing SSL certificate configured for HTTP_port
[::]:8082"
in src/client_side.cc:
* not banning other non-available options (see comment above about none
and reconnect).
ok
* looks like httpsSslBumpStep2AccessCheckDone() is still making bad
assumptions about "none" action == "splice". When its documented as
being not available at step 2 & 3.
fixed in this patch
- also reconnect action is actually performed? at step2 when documented
as not available until step3.
reconnect is not yet implemented.
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.
fixed.
* the Must() will throw on receiving final action "reconnect" which is
documented as being most relevant here on peer connections.
reconnect is not yet implemented. There is not such option...
- if that is actually right for ths bit of code please add a comment to
document why.
Amos
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
