[squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.

[Christos Tsantilas]

On 25/01/2017 08:24 μμ, Alex Rousskov wrote:
> On 01/16/2017 04:38 AM, Christos Tsantilas wrote:
>> On 13/01/2017 07:04 μμ, Alex Rousskov wrote:
>>> The dependency here is that clientHelloMessage comes from our parser. We
>>> can substitute OpenSSL-generated ClientHello with client-sent
>>> ClientHello because/if we successfully parsed and stored the latter. I
>>> think we should validate clientHelloMessage/parsing state before using
>>> clientHelloMessage.
>
>
>> +            if (bumpMode_ == Ssl::bumpPeek) {
>> +                // client-sent ClientHello must be available for peek
>> +                Must(!clientSentHello.isEmpty());
>> +                if (adjustSSL(ssl, clientTlsDetails, clientSentHello))
>>                      allowBump = true;
>> +                allowSplice = true;
>> +                // Replace OpenSSL-generated ClientHello with client-sent one.
>> +                helloMsg.append(clientSentHello);
>> +                debugs(83, 7,  "FD " << fd_ << ": Using client-sent ClientHello for peek mode");
>
> What happens if that new Must() throws?
>
> May Squid reach that Must() and will that Must() throw if Squid receives
> a valid TLS Hello encapsulated into ancient SSLv2 records?

The must will throw when Squid will be somewhere inside 
Security::PeerConnector::NegotiateSsl asyncJob call or similar code and 
the AsyncJob code will handle this exception. Actually will abort the 
connection.
This is should be enough.

>
> Our [fixed?] handling of situations like this is still not clear to me.
> My concern does not imply that your code is wrong. It is just not clear
> to me whether "client-sent ClientHello must be available for peek"
> really means
>
> * A client-sent ClientHello is required for peeking, but we might get
> here without it in some unusual cases. Throw so that FooBar will see the
> problem and handle these unusual cases.

We do not have the ClientHello here only in the case of a squid-bug.

>
>  or
>
> * A client-sent ClientHello is required for peeking. The calling code
> must ensure that we never get here without it. Throw if our calling code
> is buggy.

This is the correct.

>
> Please note that by "calling code" I do not mean some direct
> ServerBio::write() caller. I mean any Squid code that sets up the BIO
> object and allows it to proceed to write() under said circumstances.

Actually without a client hello message we should not proceed to step3 
peek or stare. We should abort in an earlier step.
The ServerBio::write is called during step3 (from step2 to step3).

>
>
>> and for stare mode allow adjustSSL only if clientSentHello is not null.
>
> That code/case is clear to me.
>
>
> Thank you,
>
> Alex.
>
>