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

[Alex Rousskov]

On 01/12/2017 02:28 PM, Christos Tsantilas wrote:
> 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.

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

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

I understand what you are saying, and agree with you in principle, but
the code does not express [y]our intentions/understanding well IMO. The
code is currently written as if we may not have a parsed ClientHello but
are still looking at the server and as if we are doubting OpenSSL
abilities to generate ClientHello. The later part I learned/realized
from your email response, which was very helpful, thank you!

Moreover, the very bug you are fixing and the patch preamble clearly
indicate that there are situations where we did not parse the
client-sent ClientHello but still want to look at the server. If we are
actually able to pull that combination off, then our code needs to be
careful not to assume that we always have the parsed client-sent
ClientHello when we are looking at the server.


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


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

Aha, I was missing this critical piece of information. It still leaves
questions about session resumption and other esoteric scenarios but it
would be easier to address them now.


> The OpenSSL generated message may replaced by the client hello message
> for stare mode, or leave it as is for peek mode.

I think you meant it the other way around (replace for peeking and leave
for staring). If so, then yes, this is understood. It is the fact that
the message was generated by OpenSSL itself that I was missing in this
context.


>>> +                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 setClientFeatures is always called on peek and stare modes.

AFAICT, setClientFeatures() was not called in peek and stare modes if we
failed to parse client-sent ClientHello. In the patched code,
setClientFeatures() is always called in those modes, but possibly with
nil/missing details and perhaps even with a partial or empty client-sent
ClientHello buffer (that becomes clientHelloMessage).

If we failed to parse the client-sent ClientHello, we cannot be sure
that cltBio->rBufData() contains some ClientHello bytes, contains the
entire client-sent ClientHello, and nothing else, right?

For example, we _cannot_ add the following check to
Ssl::ServerBio::write() because it will fail in some peeking/staring
cases, right?

  // client-sent ClientHello must be successfully parsed for peek/stare
  Must(!clientHelloMessage.isEmpty());


If my understanding is correct, then the code in Ssl::ServerBio::write()
has to check clientHelloMessage before using it. I do not see those
checks in the patched code.


I also suggest renaming clientHelloMessage to clientSentHelloMessage or
just clientSentHello to remove the current clash between two "clients"
(one agent as in "client-sent" and one the type of the Hello message as
in "ClientHello".


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

I would document that. For example:

  // buf contains OpenSSL-generated ClientHello. We assume it has a
  // complete ClientHello and nothing else, but cannot fully verify
  // that quickly. We only verify that buf starts with a v3+ record
  // containing ClientHello.
  Must(size >= 2); // enough for version and content_type checks below
  Must(buf[1] >= 3); // record's version.major; determines buf[0] meaning
  Must(buf[0] == 22); // TLSPlaintext.content_type == handshake in v3+


> It should not exist any dependency here.

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. For example, we should do either something like this

  // Replace OpenSSL-generated ClientHello with client-sent one.
  // It must be available for peek and stare modes.
  // XXX: Staring does not really require the client-sent ClientHello.
  Must(!clientHelloMessage.isEmpty());
  helloMsg.append(clientHelloMessage);

or something like that

  if (clientHelloMessage.isEmpty()) {
      /* failed to parse client-sent ClientHello */
      if (bumpMode_ == Ssl::bumpPeek)
          ... cannot peek at the server (should not even get here??) ...
      else /* stare */
          ... can stare, but with OpenSSL-generated ClientHello only ...
  } else { /* have parsed client-sent ClientHello */
      if (bumpMode_ == Ssl::bumpPeek) {
          allowBump = adjustSsl();
          helloMsg.append(clientHelloMessage);
      } else /* stare */
      if (adjustSSL) {
          allowSplice = true
          helloMsg.append(clientHelloMessage);
      } else
          ... can stare, but with OpenSSL-generated ClientHello only ...
  }

As you can see, I am still struggling with the desire to peek at the
server even if we failed to peek at the client (the core issue you are
trying to address). Peeking essentially requires forwarding client-sent
data, right? Can we forward the bytes that we failed to parse? How would
we know how many bytes to send (i.e., when to stop accumulating
client-sent bytes that we cannot parse)?! I have a feeling that
something is still missing in this area. Perhaps "failure to parse" is
not a yes/no binary state but has some shades like "failed to parse the
details but know where the ClientHello record ends"?


Finally, if we are not replacing and/or adjusting buf (for any reason),
then we should probably _not_ check buf contents and clientHelloMessage
presence. The lack of unnecessary checks will make the code more robust.


> debugs(83, 7,  "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for ... mode");

I would replace the old "Random number..." text with "Using client-sent
ClientHello for ... mode" text. We are replacing the whole ClientHello
here, and not just adjusting the random number. There are two such
debugging lines, one for each mode.


>          // If we do not build any hello message, copy the current

Similarly, I would say "if we did not use the client-sent ClientHello,
then use the OpenSSL-generated one" here to clarify.



> The  "if(!details) return false;" should moved inside
> SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK #ifdef.

Yes, please.


Thank you,

Alex.