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:[email protected]
web:http://www.chtsanti.net
Phone:+30 6977678842
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 if it
does not fully support the client SSL/TLS record/message.
This is a Measurement Factory project.
=== modified file 'src/ssl/PeekingPeerConnector.cc'
--- src/ssl/PeekingPeerConnector.cc 2016-11-19 13:25:15 +0000
+++ src/ssl/PeekingPeerConnector.cc 2017-01-12 10:30:38 +0000
@@ -154,52 +154,50 @@
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->url.host());
}
if (hostName)
SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
Must(!csd->serverBump() || csd->serverBump()->step <= Ssl::bumpStep2);
if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
auto clientSession = fd_table[clientConn->fd].ssl.get();
Must(clientSession);
BIO *bc = SSL_get_rbio(clientSession);
Ssl::ClientBio *cltBio = static_cast<Ssl::ClientBio *>(BIO_get_data(bc));
Must(cltBio);
- if (details && details->tlsVersion.protocol != AnyP::PROTO_NONE) {
+ if (details && details->tlsVersion.protocol != AnyP::PROTO_NONE)
applyTlsDetailsToSSL(serverSession.get(), details, csd->sslBumpMode);
- // Should we allow it for all protocols?
- if (details->tlsVersion.protocol == AnyP::PROTO_TLS || details->tlsVersion == AnyP::ProtocolVersion(AnyP::PROTO_SSL, 3, 0)) {
- BIO *b = SSL_get_rbio(serverSession.get());
- Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(BIO_get_data(b));
- // Inherite client features, like SSL version, SNI and other
- srvBio->setClientFeatures(details, cltBio->rBufData());
- srvBio->recordInput(true);
- srvBio->mode(csd->sslBumpMode);
- }
- }
+
+ BIO *b = SSL_get_rbio(serverSession.get());
+ Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(BIO_get_data(b));
+ Must(srvBio);
+ // inherit client features such as TLS version and SNI
+ srvBio->setClientFeatures(details, cltBio->rBufData());
+ srvBio->recordInput(true);
+ srvBio->mode(csd->sslBumpMode);
} else {
// Set client SSL options
SSL_set_options(serverSession.get(), ::Security::ProxyOutgoingConfig.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->url.hostIsNumeric() ? request->url.host() : NULL;
else
sniServer = hostName->c_str();
if (sniServer)
Ssl::setClientSNI(serverSession.get(), sniServer);
}
if (Ssl::ServerBump *serverBump = csd->serverBump()) {
serverBump->attachServerSSL(serverSession.get());
// store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE
=== modified file 'src/ssl/bio.cc'
--- src/ssl/bio.cc 2016-11-19 13:25:15 +0000
+++ src/ssl/bio.cc 2017-01-16 09:27:33 +0000
@@ -210,41 +210,41 @@
holdWrite_(false),
holdRead_(true),
record_(false),
parsedHandshake(false),
parseError(false),
bumpMode_(bumpNone),
rbufConsumePos(0)
{
}
void
Ssl::ServerBio::stateChanged(const SSL *ssl, int where, int ret)
{
Ssl::Bio::stateChanged(ssl, where, ret);
}
void
Ssl::ServerBio::setClientFeatures(Security::TlsDetails::Pointer const &details, SBuf const &aHello)
{
clientTlsDetails = details;
- clientHelloMessage = aHello;
+ clientSentHello = aHello;
};
int
Ssl::ServerBio::read(char *buf, int size, BIO *table)
{
if (parsedHandshake) // done parsing TLS Hello
return readAndGive(buf, size, table);
else
return readAndParse(buf, size, table);
}
/// Read and give everything to OpenSSL.
int
Ssl::ServerBio::readAndGive(char *buf, const int size, BIO *table)
{
// If we have unused buffered bytes, give those bytes to OpenSSL now,
// before reading more. TODO: Read if we have buffered less than size?
if (rbufConsumePos < rbuf.length())
return giveBuffered(buf, size);
@@ -314,40 +314,43 @@
const int unsent = rbuf.length() - rbufConsumePos;
const int bytes = (size <= unsent ? size : unsent);
memcpy(buf, rbuf.rawContent() + rbufConsumePos, bytes);
rbufConsumePos += bytes;
debugs(83, 7, bytes << "<=" << size << " bytes to OpenSSL");
return bytes;
}
// This function makes the required checks to examine if the client hello
// message is compatible with the features provided by OpenSSL toolkit.
// If the features are compatible and can be supported it tries to rewrite SSL
// structure members, to replace the hello message created by openSSL, with the
// web client SSL hello message.
// This is mostly possible in the cases where the web client uses openSSL
// library similar with this one used by squid.
static bool
adjustSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, SBuf &helloMessage)
{
#if SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK
+ if (!details)
+ return false;
+
if (!ssl->s3) {
debugs(83, 5, "No SSLv3 data found!");
return false;
}
// If the client supports compression but our context does not support
// we can not adjust.
#if !defined(OPENSSL_NO_COMP)
const bool requireCompression = (details->compressionSupported && ssl->ctx->comp_methods == nullptr);
#else
const bool requireCompression = details->compressionSupported;
#endif
if (requireCompression) {
debugs(83, 5, "Client Hello Data supports compression, but we do not!");
return false;
}
#if !defined(SSL_TLSEXT_HB_ENABLED)
if (details->doHeartBeats) {
debugs(83, 5, "Client Hello Data supports HeartBeats but we do not support!");
@@ -417,67 +420,71 @@
ssl->init_num = mainHelloSize;
ssl->s3->wpend_ret = mainHelloSize;
ssl->s3->wpend_tot = mainHelloSize;
return true;
#else
return false;
#endif
}
int
Ssl::ServerBio::write(const char *buf, int size, BIO *table)
{
if (holdWrite_) {
debugs(83, 7, "postpone writing " << size << " bytes to SSL FD " << fd_);
BIO_set_retry_write(table);
return -1;
}
if (!helloBuild && (bumpMode_ == Ssl::bumpPeek || bumpMode_ == Ssl::bumpStare)) {
- if (
- buf[1] >= 3 //it is an SSL Version3 message
- && buf[0] == 0x16 // and it is a Handshake/Hello message
- ) {
-
- //Hello message is the first message we write to server
- assert(helloMsg.isEmpty());
-
- auto ssl = fd_table[fd_].ssl.get();
- if (ssl) {
- if (bumpMode_ == Ssl::bumpPeek) {
- if (adjustSSL(ssl, clientTlsDetails, clientHelloMessage))
- allowBump = true;
- allowSplice = true;
- helloMsg.append(clientHelloMessage);
- debugs(83, 7, "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for peek mode");
- } else { /*Ssl::bumpStare*/
+ // 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+
+
+ //Hello message is the first message we write to server
+ assert(helloMsg.isEmpty());
+
+ if (auto ssl = fd_table[fd_].ssl.get()) {
+ if (bumpMode_ == Ssl::bumpPeek) {
+ // client-sent ClientHello must be available for peek
+ Must(!clientSentHello.isEmpty());
+ if (adjustSSL(ssl, clientTlsDetails, clientSentHello))
allowBump = true;
- if (adjustSSL(ssl, clientTlsDetails, clientHelloMessage)) {
- allowSplice = true;
- helloMsg.append(clientHelloMessage);
- debugs(83, 7, "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for stare mode");
- }
+ 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");
+ } else { /*Ssl::bumpStare*/
+ allowBump = true;
+ if (!clientSentHello.isEmpty() && adjustSSL(ssl, clientTlsDetails, clientSentHello)) {
+ allowSplice = true;
+ helloMsg.append(clientSentHello);
+ debugs(83, 7, "FD " << fd_ << ": Using client-sent ClientHello for stare mode");
}
}
}
- // If we do not build any hello message, copy the current
+ // if we did not use the client-sent ClientHello, then use the OpenSSL-generated one
if (helloMsg.isEmpty())
helloMsg.append(buf, size);
helloBuild = true;
helloMsgSize = helloMsg.length();
//allowBump = true;
if (allowSplice) {
// Do not write yet.....
BIO_set_retry_write(table);
return -1;
}
}
if (!helloMsg.isEmpty()) {
debugs(83, 7, "buffered write for FD " << fd_);
int ret = Ssl::Bio::write(helloMsg.rawContent(), helloMsg.length(), table);
helloMsg.consume(ret);
if (!helloMsg.isEmpty()) {
// We need to retry sendind data.
=== modified file 'src/ssl/bio.h'
--- src/ssl/bio.h 2016-11-19 13:25:15 +0000
+++ src/ssl/bio.h 2017-01-16 09:08:02 +0000
@@ -154,41 +154,41 @@
bool gotHello() const { return (parsedHandshake && !parseError); }
/// Return true if the Server Hello parsing failed
bool gotHelloFailed() const { return (parsedHandshake && parseError); }
/// \return the server certificates list if received and parsed correctly
const Security::CertList &serverCertificatesIfAny() { return parser_.serverCertificates; }
/// \return the TLS Details advertised by TLS server.
const Security::TlsDetails::Pointer &receivedHelloDetails() const {return parser_.details;}
private:
int readAndGive(char *buf, const int size, BIO *table);
int readAndParse(char *buf, const int size, BIO *table);
int readAndBuffer(BIO *table);
int giveBuffered(char *buf, const int size);
/// SSL client features extracted from ClientHello message or SSL object
Security::TlsDetails::Pointer clientTlsDetails;
/// TLS client hello message, used to adapt our tls Hello message to the server
- SBuf clientHelloMessage;
+ SBuf clientSentHello;
SBuf helloMsg; ///< Used to buffer output data.
mb_size_t helloMsgSize;
bool helloBuild; ///< True if the client hello message sent to the server
bool allowSplice; ///< True if the SSL stream can be spliced
bool allowBump; ///< True if the SSL stream can be bumped
bool holdWrite_; ///< The write hold state of the bio.
bool holdRead_; ///< The read hold state of the bio.
bool record_; ///< If true the input data recorded to rbuf for internal use
bool parsedHandshake; ///< whether we are done parsing TLS Hello
bool parseError; ///< error while parsing server hello message
Ssl::BumpMode bumpMode_;
/// The size of data stored in rbuf which passed to the openSSL
size_t rbufConsumePos;
Security::HandshakeParser parser_; ///< The TLS/SSL messages parser.
};
} // namespace Ssl
void
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
