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

Fri Aug 14 14:41:18 UTC 2015

Hi all,
  The wiki pages are fixed.
Is it OK to commit this patch?

On 08/11/2015 02:24 PM, Tsantilas Christos wrote:
> 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 at lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
>>
>
>
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>