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

[Amos Jeffries]

On 11/08/2015 11:24 p.m., 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.

Gah. So for the last year-ish it has been published in wiki
SslPeekAndSplice page as an available config option. But is actually
nothing more than an unused enum value ?

Great. Please fix *that*.

Amos