ssl::server_name ACL badly broken since inception (trunk r14008).

The original server_name code mishandled all SNI checks and some rare
host checks:

* The SNI-derived value was pointing to an already freed memory storage.
* Missing host-derived values were not detected (host() is never nil).
* Mismatches were re-checked with an undocumented "none" value
  instead of being treated as mismatches.

Same for ssl::server_name_regex.

Also set SNI for more server-first and client-first transactions.

This is a Measurement Factory project.

diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc
index 0ddd77ccaee585ad63ca59dc0e11d39509543812..da4cf1ba5829d5a11551a5586ccf64b17012629c 100644 (file)
--- a/src/acl/ServerName.cc
+++ b/src/acl/ServerName.cc
@@ -91,27 +91,29 @@ ACLServerNameStrategy::match (ACLData<MatchType> * &data, ACLFilledChecklist *ch
 {
     assert(checklist != NULL && checklist->request != NULL);
 
-    if (checklist->conn() && checklist->conn()->serverBump()) {
-        if (X509 *peer_cert = checklist->conn()->serverBump()->serverCert.get()) {
-            if (Ssl::matchX509CommonNames(peer_cert, (void *)data, check_cert_domain<MatchType>))
-                return 1;
+    const char *serverName = nullptr;
+    SBuf serverNameKeeper; // because c_str() is not constant
+    if (ConnStateData *conn = checklist->conn()) {
+
+        if (conn->serverBump()) {
+            if (X509 *peer_cert = conn->serverBump()->serverCert.get())
+                return Ssl::matchX509CommonNames(peer_cert, (void *)data, check_cert_domain<MatchType>);
         }
-    }
 
-    const char *serverName = NULL;
-    if (checklist->conn() && !checklist->conn()->sslCommonName().isEmpty()) {
-        SBuf scn = checklist->conn()->sslCommonName();
-        serverName = scn.c_str();
+        if (conn->sslCommonName().isEmpty()) {
+            const char *host = checklist->request->url.host();
+            if (host && *host) // paranoid first condition: host() is never nil
+                serverName = host;
+        } else {
+            serverNameKeeper = conn->sslCommonName();
+            serverName = serverNameKeeper.c_str();
+        }
     }
 
-    if (serverName == NULL)
-        serverName = checklist->request->url.host();
-
-    if (serverName && data->match(serverName)) {
-        return 1;
-    }
+    if (!serverName)
+        serverName = "none";
 
-    return data->match("none");
+    return data->match(serverName);
 }
 
 ACLServerNameStrategy *

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 0cda978351689501d81e9544e2567025171601f4..380a8a6c8d0b11e4e2e99f4c72017488a675651e 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1292,6 +1292,9 @@ IF USE_OPENSSL
          # During each Ssl-Bump step, Squid may improve its understanding of a
          # "true server name". Unlike dstdomain, this ACL does not perform
          # DNS lookups.
+         # The "none" name can be used to match transactions where Squid
+         # could not compute the server name using any information source
+         # already available at the ACL evaluation time.
 
        acl aclname ssl::server_name_regex [-i] \.foo\.com ...
          # regex matches server name obtained from various sources [fast]

diff --git a/src/client_side.cc b/src/client_side.cc
index 4987fc407aeaa9955a26a3c86e9c741827f2a02e..fd69d212777b0de0c0932d6dc83a2449a9ac118c 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2719,14 +2719,6 @@ clientNegotiateSSL(int fd, void *data)
         debugs(83, 5, "FD " << fd << " has no certificate.");
     }
 
-#if defined(TLSEXT_NAMETYPE_host_name)
-    if (!conn->serverBump()) {
-        // when in bumpClientFirst mode, get the server name from SNI
-        if (const char *server = SSL_get_servername(session.get(), TLSEXT_NAMETYPE_host_name))
-            conn->resetSslCommonName(server);
-    }
-#endif
-
     conn->readSomeData();
 }
 
@@ -3186,7 +3178,13 @@ ConnStateData::parseTlsHandshake()
 
     // Even if the parser failed, each TLS detail should either be set
     // correctly or still be "unknown"; copying unknown detail is a no-op.
-    clientConnection->tlsNegotiations()->retrieveParsedInfo(tlsParser.details);
+    Security::TlsDetails::Pointer const &details = tlsParser.details;
+    clientConnection->tlsNegotiations()->retrieveParsedInfo(details);
+    if (details && !details->serverName.isEmpty()) {
+        resetSslCommonName(details->serverName.c_str());
+        if (sslServerBump)
+            sslServerBump->clientSni = details->serverName;
+    }
 
     // We should disable read/write handlers
     Comm::SetSelect(clientConnection->fd, COMM_SELECT_READ, NULL, NULL, 0);
@@ -3229,14 +3227,6 @@ ConnStateData::startPeekAndSplice(const bool unsupportedProtocol)
         return;
     }
 
-    if (serverBump()) {
-        Security::TlsDetails::Pointer const &details = tlsParser.details;
-        if (details && !details->serverName.isEmpty()) {
-            serverBump()->clientSni = details->serverName;
-            resetSslCommonName(details->serverName.c_str());
-        }
-    }
-
     startPeekAndSpliceDone();
 }