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.

diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index d3e6249f956ecedc0315589657f0e40603ecb9e2..645e000b3e67a5bdcecbdd45fe24e8dd171a4659 100644 (file)
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -171,18 +171,16 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
             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);

diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc
index 3a6e0a48f2dabb38d7cd7ce3051e1f6ffb5d6c62..a91b03cbbdc3f48ccf47addbd38784391a597a79 100644 (file)
--- a/src/ssl/bio.cc
+++ b/src/ssl/bio.cc
@@ -268,7 +268,7 @@ void
 Ssl::ServerBio::setClientFeatures(Security::TlsDetails::Pointer const &details, SBuf const &aHello)
 {
     clientTlsDetails = details;
-    clientHelloMessage = aHello;
+    clientSentHello = aHello;
 };
 
 int
@@ -372,6 +372,9 @@ 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;
@@ -475,33 +478,37 @@ Ssl::ServerBio::write(const char *buf, int size, BIO *table)
     }
 
     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) {
+                // we should not be here if we failed to parse the client-sent ClientHello
+                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);
 

diff --git a/src/ssl/bio.h b/src/ssl/bio.h
index 1c13938feccf68630d8dca6defdffd6ccb32d93f..512a3b5a4503cf53fdf007bb4d987668405ba20b 100644 (file)
--- a/src/ssl/bio.h
+++ b/src/ssl/bio.h
@@ -182,7 +182,7 @@ private:
     /// 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