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

[Tsantilas Christos]

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
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ignore-bumping-actions-t5.patch
Type: text/x-patch
Size: 23020 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150811/f0fdc357/attachment-0001.bin>