From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Fri, 27 Jan 2017 17:02:52 +0000 (+1300)
Subject: SSLv2 records force SslBump bumping despite a matching step2 peek rule.
X-Git-Tag: SQUID_3_5_24~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8d7a7f3a7c06d1e0f64b1f9e68be0cbfe2a59874;p=thirdparty%2Fsquid.git

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/PeerConnector.cc b/src/ssl/PeerConnector.cc
index 9dbb367502..f5d4c813e0 100644
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -171,18 +171,15 @@ Ssl::PeerConnector::initializeSsl()
         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<Ssl::ServerBio *>(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<Ssl::ServerBio *>(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);
diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc
index 6347a6756f..2b983e12b0 100644
--- a/src/ssl/bio.cc
+++ b/src/ssl/bio.cc
@@ -340,6 +340,9 @@ static bool
 adjustSSL(SSL *ssl, Ssl::Bio::sslFeatures &features)
 {
 #if SQUID_USE_OPENSSL_HELLO_OVERWRITE_HACK
+    if (!features.initialized_)
+        return false;
+
     if (!ssl->s3) {
         debugs(83, 5, "No SSLv3 data found!");
         return false;
@@ -472,33 +475,38 @@ 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());
-
-            SSL *ssl = fd_table[fd_].ssl;
-            if (clientFeatures.initialized_ && ssl) {
-                if (bumpMode_ == Ssl::bumpPeek) {
-                    if (adjustSSL(ssl, clientFeatures))
-                        allowBump = true;
+        // 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 (SSL *ssl = fd_table[fd_].ssl) {
+            if (bumpMode_ == Ssl::bumpPeek) {
+                // we should not be here if we failed to parse the client-sent ClientHello
+                Must(clientFeatures.initialized_);
+                if (adjustSSL(ssl, clientFeatures))
+                    allowBump = true;
+                allowSplice = true;
+                 // Replace OpenSSL-generated ClientHello with client-sent one.
+                helloMsg.append(clientFeatures.helloMessage);
+                debugs(83, 7,  "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for peek mode");
+            } else { /*Ssl::bumpStare*/
+                allowBump = true;
+                if (clientFeatures.initialized_ && adjustSSL(ssl, clientFeatures)) {
                     allowSplice = true;
                     helloMsg.append(clientFeatures.helloMessage);
-                    debugs(83, 7,  "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for peek mode");
-                } else { /*Ssl::bumpStare*/
-                    allowBump = true;
-                    if (adjustSSL(ssl, clientFeatures)) {
-                        allowSplice = true;
-                        helloMsg.append(clientFeatures.helloMessage);
-                        debugs(83, 7,  "SSL HELLO message for FD " << fd_ << ": Random number is adjusted for stare mode");
-                    }
+                    debugs(83, 7,  "SSL HELLO message for FD " << fd_ << ": Random number is adjusted 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);