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

[Christos Tsantilas]

On 12/01/2017 06:48 μμ, Alex Rousskov wrote:
> On 01/12/2017 08:35 AM, Christos Tsantilas wrote:
>
>> The patch fixes Squid to peeks (or stares) at the origin server as
>> configured, even if it does not recognize the client TLS
>> record/message.
>
> s/to peeks (or stares)/to peek (or stare)/
>
> I agree that this is the right thing to do, but I have some concerns
> regarding _how_ we are doing that peeking or staring during step3 when
> we essentially failed to get ClientHello during step2. Specifics are below.

The documentation of the patch is confusing, sorry.

If parsing ClientHello failed, and we are not able to recognize a 
TLS/SSL hello message , we are not bumping.
The on_unsupported_protocol is used to decide how squid will handle this 
request.

I need to recheck how squid-3.5 handle these cases.

>
>
>>      if (!helloBuild && (bumpMode_ == Ssl::bumpPeek || bumpMode_ == Ssl::bumpStare)) {
>> +        // it is an SSL Version3 message and it is a Handshake/Hello message
>> +        Must (buf[1] >= 3 && buf[0] == 0x16);
>
> If this is a Must() now, then we should not combine the two conditions
> so that we know which of them has failed if one of them does. Also, we
> should check the size of the buffer before accessing its contents!
> Finally, it would be good to explain the odd (but correct) order of
> checks. Here is a sketch:

ok

>
>   /* check that we have a v3+ record containing ClientHello */
>   Must(size >= 2); // enough for version and content_type checks below
>   Must(buf[1] >= 3); // record's version.major
>   // only now we know that we are dealing with supported record format
>   Must(buf[0] == 22); // TLSPlaintext.content_type == handshake in v3+
>
> It is not 100% clear to me why we expect these checks to pass now. It
> probably has something to do with the fact that we are inside BIO (and
> so we expect to deal with SSL traffic) but what if this is an
> unsupported SSL version, session resumption, or some other situation
> where there is no ClientHello? Why do we demand a ClientHello presence
> here? But only in peek or stare modes? I think we need to add a comment
> to clarify all that.


Actually these checks are not really needed. To fail here it must be 
something very wrong.

The buf  buffer actually holds the TLS Hello message built by OpenSSL 
subsystem. The Must here actually checks if this is really a  TLS/Hello 
message.
The OpenSSL generated message may replaced by the client hello message 
for stare mode, or leave it as is for peek mode.

>
> On a related note, did not we just try to parse the same information
> already when looking at ClientHello? Why not use that parsed info
> instead of re-parsing the same bytes? And if we did not try to parse
> these bytes earlier (for one reason or another; e.g., server-first
> bumping?), perhaps we should not try parsing it here either??

We are not actually parsing anything. It is just a check, but not on the 
already parsed client hello.


>
>
>> +        auto ssl = fd_table[fd_].ssl.get();
>> +        if (ssl) {
>
> If possible, merge into one statement to avoid leaking the ssl pointer
> name outside the if-statement that ensures it is not nil.

ok

>
>
>>  static bool
>>  adjustSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, SBuf &helloMessage)
>>  {
>> +    if (!details)
>> +        return false;
> ...
>>  }
>
> ... but then ...
>
>> +                helloMsg.append(clientHelloMessage);
>
> If details (i.e., clientTlsDetails) are missing, then
> setClientFeatures() was not called. If setClientFeatures() was not
> called, then clientHelloMessage would be missing as well, but we are
> using it unconditionally above, even when adjustSSL() returns false.
> This does not add up IMO. Perhaps we have to require that
> clientTlsDetails are set before we use it?


The adjustSSL is always return false as is now unless the
SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK is set.
The  "if(!details) return false;" should moved inside 
SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK #ifdef.

The setClientFeatures is always called on peek and stare modes.

>
>
>>          // If we do not build any hello message, copy the current
>>          if (helloMsg.isEmpty())
>>              helloMsg.append(buf, size);
>
> And if we did build a helloMsg ourselves, then what happens to the buf
> contents (that may not match our clientHelloMessage boundary)? For
> example, how do we know that buf does not contain just 50% of the client
> handshake and that the other 50% would not be later sent after our

We know this because we prevent actually writing data unless openSSL 
subsystem generates full hello message. The openSSL buffers the output 
and try to write it in one write. If this is change in the future, yes 
we may have problem.

> handshake, corrupting the stream? I think we know this because our
> handshake parser waits until the end of the client handshake, but is it
> possible that we are building helloMsg even though our parser has failed
> (or was not engaged at all)?

We are talking about Ssl::ServerBio::write. The ServerBio  is used in 
Squid-to-Server communications (still we are confused from server-side 
and client-side names!).
We are not running our parser on Hello messages written to the SSL 
server. The client handshake parser is not involved here.

Did I understand your question?

>
> Perhaps (bumpMode_ == Ssl::bumpPeek || bumpMode_ == Ssl::bumpStare)
> implies that our parser has succeeded?? If yes, then we need a comment
> here to clarify these hidden/assumed dependencies (at least).

Yes actually this is happens. We are here because our parser 
succeeds.But I do not think a comment required here. It should not exist 
any dependency here.
We need to now the bumping mode, because we have to send OpenSSL 
generated client Hello message to SSL server for stare mode, or replace 
with client hello message for peek mode. Only this.

We need the clientDetails, because we are trying to emulate the major 
TLS settings of the squid client.



>
>
> Thank you,
>
> Alex.
>
>