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

[Alex Rousskov]

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?

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.

 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.

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.


> and for stare mode allow adjustSSL only if clientSentHello is not null.

That code/case is clear to me.


Thank you,

Alex.