Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
On 27/01/2017 5:54 a.m., Christos Tsantilas wrote: > The patch applied to squid-5 as r15020 with the fixes suggested by Alex. > > I am attaching the equivalent patch for squid-3.5. > Applied to 3.5 as rev.14139 Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
The patch applied to squid-5 as r15020 with the fixes suggested by Alex.
I am attaching the equivalent patch for squid-3.5.
On 25/01/2017 11:42 μμ, Alex Rousskov wrote:
On 01/25/2017 12:12 PM, Christos Tsantilas wrote:
On 25/01/2017 08:24 μμ, Alex Rousskov wrote:
* 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.
This is the correct.
Great. I have no objections to a commit then.
I suggest replacing the "client-sent ClientHello must be available for
peek" comment with "we should not be here if we failed to parse the
client-sent ClientHello" to clarify, but this is obviously minor.
Now Squid peeks (or stares) at the origin server as configured, even if it
does not fully support the client SSL/TLS record/message.
Perhaps the vagueness of that "does not fully support" phrase drives
some of my questions. Initially, I probably thought that Squid cannot
even parse a TLS Hello in an SSLv2 record. And that failure to parse
triggered the wrong code path in ServerBio::write().
Now I think that we actually parse it just fine and the buggy code was
just too aggressive in reacting to the wrong version (which was already
flagged by the "Should we allow it for all protocols?" comment).
If we ever get to step3, OpenSSL may not be able to successfully
negotiate an SSL connection with a client that is using SSLv2 records.
That potential bumping failure during step3 is not a problem we are
solving here, and it is not a problem in pure peek-and-splice setups
because they do not normally bump and, hence, never ask OpenSSL to
negotiate using SSLv2 records.
Please correct my understanding or detail the commit message a little to
confirm it. For example, we could say "Now Squid peeks (or stares) at
the origin server as configured, even after detecting (and parsing)
SSLv2 records".
Thank you,
Alex.
SSLv2 records force SslBump bumping despite a matching step2 peek rule.
If Squid receives a valid TLS Hello encapsulated into ancient SSLv2
records (observed on Solaris 10), the old code ignored the step2 peek
decision and bumped the transaction instead.
Now Squid peeks (or stares) at the origin server as configured, even
after detecting (and parsing) SSLv2 records.
This is a Measurement Factory project.
=== modified file 'src/ssl/PeerConnector.cc'
--- src/ssl/PeerConnector.cc 2017-01-01 00:16:45 +
+++ src/ssl/PeerConnector.cc 2017-01-26 16:27:32 +
@@ -154,52 +154,49 @@
if (!features.serverName.isEmpty())
hostName = new SBuf(features.serverName);
}
if (!hostName) {
// While we are peeking at the certificate, we may not know the server
// name that the client will request (after interception or CONNECT)
// unless it was the CONNECT request with a user-typed address.
const bool isConnectRequest = !csd->port->flags.isIntercepted();
if (!request->flags.sslPeek || isConnectRequest)
hostName = new SBuf(request->GetHost());
}
if (hostName)
SSL_set_ex_data(ssl, ssl_ex_index_server, (void*)hostName);
Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2);
if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
assert(cltBio);
const Ssl::Bio::sslFeatures &features = cltBio->getFeatures();
-if (features.sslVersion != -1) {
+if (features.sslVersion != -1)
features.applyToSSL(ssl, csd->sslBumpMode);
-// Should we allow it for all protocols?
-if (features.sslVersion >= 3) {
-BIO *b = SSL_get_rbio(ssl);
-Ssl::ServerBio *srvBio = static_cast(b->ptr);
-// Inherite client features, like SSL version, SNI and other
-srvBio->setClientFeatures(features);
-srvBio->recordInput(true);
-srvBio->mode(csd->sslBumpMode);
-}
-}
+
+BIO *b = SSL_get_rbio(ssl);
+Ssl::ServerBio *srvBio = static_cast(b->ptr);
+// Inherite client features, like SSL version, SNI and other
+srvBio->setClientFeatures(features);
+srvBio->recordInput(true);
+srvBio->mode(csd->sslBumpMode);
} else {
// Set client SSL options
SSL_set_options(ssl, ::Config.ssl_client.parsedOptions);
// Use SNI TLS extension only when we connect directly
// to the origin server and we know the server host name.
const char *sniServer = NULL;
const bool redirected = request->flags.redirected && ::Config.onoff.redir_rewrites_host;
if (!hostName || redirected)
sniServer = !request->GetHostIsNumeric() ? request->GetHost() : NULL;
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
On 01/25/2017 12:12 PM, Christos Tsantilas wrote: >> On 25/01/2017 08:24 μμ, Alex Rousskov wrote: >> * 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. > This is the correct. Great. I have no objections to a commit then. I suggest replacing the "client-sent ClientHello must be available for peek" comment with "we should not be here if we failed to parse the client-sent ClientHello" to clarify, but this is obviously minor. > Now Squid peeks (or stares) at the origin server as configured, even if it > does not fully support the client SSL/TLS record/message. Perhaps the vagueness of that "does not fully support" phrase drives some of my questions. Initially, I probably thought that Squid cannot even parse a TLS Hello in an SSLv2 record. And that failure to parse triggered the wrong code path in ServerBio::write(). Now I think that we actually parse it just fine and the buggy code was just too aggressive in reacting to the wrong version (which was already flagged by the "Should we allow it for all protocols?" comment). If we ever get to step3, OpenSSL may not be able to successfully negotiate an SSL connection with a client that is using SSLv2 records. That potential bumping failure during step3 is not a problem we are solving here, and it is not a problem in pure peek-and-splice setups because they do not normally bump and, hence, never ask OpenSSL to negotiate using SSLv2 records. Please correct my understanding or detail the commit message a little to confirm it. For example, we could say "Now Squid peeks (or stares) at the origin server as configured, even after detecting (and parsing) SSLv2 records". Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
On 25/01/2017 08:24 μμ, Alex Rousskov wrote:
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?
The must will throw when Squid will be somewhere inside
Security::PeerConnector::NegotiateSsl asyncJob call or similar code and
the AsyncJob code will handle this exception. Actually will abort the
connection.
This is should be enough.
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.
We do not have the ClientHello here only in the case of a squid-bug.
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.
This is the correct.
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.
Actually without a client hello message we should not proceed to step3
peek or stare. We should abort in an earlier step.
The ServerBio::write is called during step3 (from step2 to step3).
and for stare mode allow adjustSSL only if clientSentHello is not null.
That code/case is clear to me.
Thank you,
Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
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.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
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 Cl
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
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 kn
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
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
Re: [squid-dev] [PATCH] SSLv2 records force SslBump bumping despite a matching step2 peek rule.
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.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
