[squid-dev] [PATCH] sslproxy_options in peek-and-splice mode

[Amos Jeffries]

On 13/02/2015 4:51 a.m., Tsantilas Christos wrote:
> On 02/12/2015 05:33 PM, Amos Jeffries wrote:
>> On 13/02/2015 3:34 a.m., Tsantilas Christos wrote:
>>> On 02/12/2015 01:48 PM, Amos Jeffries wrote:
>>>> On 12/02/2015 11:31 p.m., Tsantilas Christos wrote:
>>>>> On 02/11/2015 09:48 PM, Amos Jeffries wrote:
>>>>>> On 12/02/2015 12:45 a.m., Tsantilas Christos wrote:
>>>>>>> On 02/11/2015 01:54 AM, Amos Jeffries wrote:
>>>>>>>> On 9/02/2015 6:43 a.m., Tsantilas Christos wrote:
>>>>>>>>> Bug description:
>>>>>>>>>
>>>>>>>>>       - Squid sslproxy_options deny the use of TLSv1_2 SSL
>>>>>>>>> protocol:
>>>>>>>>>                sslproxy_options NO_TLSv1_2
>>>>>>>>>       - Squid uses peek mode for bumped connections.
>>>>>>>>>       - Web client sends an TLSv1_2 hello message and squid in
>>>>>>>>> peek
>>>>>>>>> mode,
>>>>>>>>> forwards the client hello message to server
>>>>>>>>>       - Web server respond with an TLSv1_2 hello message
>>>>>>>>>       - Squid while parsing server hello message aborts with an
>>>>>>>>> error
>>>>>>>>> because  sslproxy_options deny the use ot TLSv1_2 protocol.
>>>>>>>>>
>>>>>>>>> This patch fixes squid to ignore sslproxy_options in peek or stare
>>>>>>>>> bumping mode.
>>>>>>>>
>>>>>>>> As I understand it the action of applying the options to the
>>>>>>>> context
>>>>>>>> removes from the context cipher references etc which are not
>>>>>>>> possible.
>>>>>>>>
>>>>>>>> Since peek and stare are non-final states I can easily imagine that
>>>>>>>> OpenSSL library negotiates ciphers which the options would
>>>>>>>> otherwise
>>>>>>>> prohibit. Then when the options get applied to the context it find
>>>>>>>> itself using an algorithm which does not exist.
>>>>>>>
>>>>>>> The context SSL_CTX objects are bases to create the SSL objects
>>>>>>> which
>>>>>>> are responsible for the negotiation with the other side (server in
>>>>>>> this
>>>>>>> case).
>>>>>>> The SSL created, inherits the options from CTXa nd we are adding our
>>>>>>> options to SSL object.
>>>>>>> The SSL library will use these options to build client hello
>>>>>>> message,
>>>>>>> parse server hello message and select algorithms/ciphers and other
>>>>>>> features to establish SSL connection.
>>>>>>>
>>>>>>> In my patch the options applied to the squid client SSL objects
>>>>>>> immediately after created, in the case of bump-server-first, or
>>>>>>> bump-client-first.
>>>>>>>
>>>>>>> In the cases of peek or stare we are not setting any options.
>>>>>>> This is
>>>>>>> because we are sending a hello message which is the same or similar
>>>>>>> with
>>>>>>> the client hello message, so we can not apply options. Else the
>>>>>>> peek or
>>>>>>> stare will fail...
>>>>>>>
>>>>>>
>>>>>> What I means is the code flow is roughly like this yes?
>>>>>>
>>>>>> 1) bump
>>>>>> 2) splice
>>>>>> 3) peek then bump
>>>>>> 4) stare then bump
>>>>>> 5) peek then splice
>>>>>> 6) stare then splice
>>>>>>
>>>>>> In which of those cases are the options set?
>>>>>>     all cases ending in bump or splice.
>>>>>
>>>>> The important factor here is the bumping step.
>>>>> in bump case (1,3,4)
>>>>>    - if the decision is to bump in bumpStep1 or bumpStep2 then the
>>>>> squid
>>>>> SSL client options are set.
>>>>>    - If the decision for bumping taken in bumpStep3, the the squid SSL
>>>>> client options are NOT set.
>>>>>
>>>>> in splice case (2,5 or 6) :
>>>>>    -  if we splice on bumpStep1 or on bumpStep2, then we are not using
>>>>> squid SSL client code at all, so the options does not play any 
>>>>> role on
>>>>> this case.
>>>>>    - If we splice on bumpStep3 we have use squid SSL client, but in
>>>>> this
>>>>> case the options are not set.
>>>>>
>>>>>
>>>>> After the bumpStep2 completed we have received the client hello
>>>>> message,
>>>>> but we do not sent any response (server hello). At the same time we
>>>>> are
>>>>> starting to initiate the connection to the SSL server. This is mean
>>>>> that
>>>>> we are going to set SSL client hello message which is depends on SSL
>>>>> client code. So:
>>>>>     - If we know that we will going to bump the connection, we can
>>>>> safely
>>>>> set SSL client options. This is because the squid SSL client will
>>>>> initiate a normal SSL connection.
>>>>>     - If we are going to stare, or peek then the client hello
>>>>> message we
>>>>> are going to sent must be similar to the client-to-squid SSL hello
>>>>> message. In this case we can not control SSL features using options,
>>>>> else the SSL negotiation will fail.
>>>>
>>>> So you're saying the src/ssl/PeerConnector.cc else-condition never gets
>>>> run after a peek and/or stare ?
>>>
>>> Do you mean the "if (csd->sslBumpMode == Ssl::bumpPeek ||
>>> csd->sslBumpMode == Ssl::bumpStare)" ?
>>
>>
>> I mean the:
>>
>>       } else {
>> +        // Set client SSL options
>> +        SSL_set_options(ssl, ::Config.ssl_client.parsedOptions);
>> +
>>
>>
>>>
>>> Yes the  "else" never gets run when peek or stare mode selected in
>>> bumpStep2 bumping step.
>>>
>>>
>>>
>>>>
>>>> Then please add a Must(step <= 2) check at the start of that else
>>>> condition right before setting the SSL client options. If that works
>>>> properly I am happy for this to go in.
>>>
>>> Requires a code like the following:
>>> Must(csd->sslServerBump()->step<=Ssl::bumpStep2);
>>
>> Specifically:
>>
>> Must(
>>       !csd->sslServerBump() ||
>>       csd->sslServerBump()->step <= Ssl::bumpStep2
>>   );
>>
>>>
>>> This is will not work. In client-first bumping mode or in server-first
>>> bumping mode where we are not applying the peek-and-splice procedure the
>>> step is not updated.
>>
>> Which would make step 0 or 1 for client-first right? that is fine.
>>
>> For server-first it does need updating at the point the server is given
>> data. A jump right to step 3, or even a new "sslBumpStepServerFirst"
>> value at the end of the enum.
> 
> Exactly.
> But why do you beleive this Must is needed?  Specially inside "else" is
> completely without any interest for development or debugging. Even if
> for a reason this is not set correctly, it will not cause any problem.


To make certain that it *is* always operating right, both now and in
future. We are playing with complex and security critical behaviour here.

There is a quite real risk that IF this stops working properly in the
way you described - such as after future redesign changes, that the TLS
will fail in obscure ways. That is a long way from "not cause any problem".


> 
> The csd->sslServerBump()->step is set to Ssl::bumpStep3 in a clearly
> later step, inside Ssl::PeerConnector::checkForPeekAndSplice method,
> which called from Ssl::PeerConnector::handleNegotiateError.
> 

I assume that means you think the dont need to add the step change to
make the Must() work. Good.

Amos