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

[Alex Rousskov]

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.


>      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:

  /* 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.

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??


> +        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.


>  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?


>          // 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
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)?

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).


Thank you,

Alex.