[squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
Christos Tsantilas
christos at chtsanti.net
Mon Jan 16 11:38:17 UTC 2017
I am attaching a new patch based on Alex comments.
I also changed the patch preamble a little to much better what squid does.
Please see my comments bellow.
On 13/01/2017 07:04 μμ, Alex Rousskov wrote:
> 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
Yep this is true.
Currently the code considers unknown protocol if we failed to parse
Hello message. But I hope we are able to parse (all of? / most of?)
SSLv2 and SSLv3/TLS messages.
> 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.
Yes, I agree.
The Ssl::PeekingPeerConnector class and Ssl::ServerBio is better to
assume that it may receive requests initiated from unsupported TLS messages.
>
>>>> + 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,
Currently if we fail to parse client-sent clientHello message we are not
going to proceed with peek/stare modes. It will be considered as foreign
protocol.
But as you said, we must not assume we have parsed client-sent hello.
At least for PeekingPeerConnector and ServerBio 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).
yep.
>
> 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?
True.
>
> 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.
Yes we must add such checks.
> 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".
I rename it to clientSentHello
>>>> // 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+
OK
>
>
>> 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"?
I select a little different form.
a Must(clientSentHello) for peek mode and for stare mode allow adjustSSL
only if clientSentHello is not null.
Actually the clientSentHello required only for peek mode.
>
>
> 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.
fixed
>
>
>> // 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.
>
fixed
>
>
>> The "if(!details) return false;" should moved inside
>> SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK #ifdef.
fixed.
>
> Yes, please.
>
>
> Thank you,
>
> Alex.
>
>
--
Tsantilas Christos
Network and Systems Engineer
email:christos at chtsanti.net
web:http://www.chtsanti.net
Phone:+30 6977678842
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-241-SSL-v2-records-force-step2-bumping-t3.patch
Type: text/x-patch
Size: 12952 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170116/20a165fd/attachment-0001.bin>
More information about the squid-dev
mailing list