From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Mon, 14 Nov 2016 10:51:24 +0000 (+1300)
Subject: Fix ssl::server_name ACL badly broken since inception.
X-Git-Tag: SQUID_3_5_23~18
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7a8f2f418058c3ac840b4972c03ef614fd2383ac;p=thirdparty%2Fsquid.git

Fix ssl::server_name ACL badly broken since inception.

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 f49209bdc0..ae93e03920 100644
--- a/src/acl/ServerName.cc
+++ b/src/acl/ServerName.cc
@@ -90,27 +90,28 @@ 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 = NULL;
+    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->GetHost();
+            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->GetHost();
-
-    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 4edc18cfcd..44a3290cf5 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1167,6 +1167,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]