Fix ipv4 and expand ipv6 ACL parameter matching (#1568)

The undocumented "ipv4", "ipv6", and "all" parameters can be used in
src, dst, and localip ACLs. This refactoring fixes "ipv4" matching and
expands "ipv6" scope as detailed further below. These parameters remain
undocumented.

Together, the fixed ipv4 parameter and the adjusted ipv6 parameter now
match any IP address known to Squid. So does the undocumented "all"
parameter (which is currently used to implement the "all" dst ACL).

Also speed up matching of ACLs containing "ipv4", "ipv6", and "all"
parameters (at the cost of adding two boolean tests to other src, dst,
and localip ACLs). This performance improvement is a side effect of the
corresponding code improvements and is not the focus of these changes.

Also improved mgr:config reporting of ACLs containing these special
parameters. They are now reported using their names (e.g., "all") rather
than IP addresses (e.g., `::/0` instead of "all").

### Fix ipv4 ACL parameter to match (any IPv4 address)

The "ipv4" parameter is supposed to match any IPv4 address, but it
matched none because an unmasked[^1] IPv4 input address was compared to
an all-0s address[^2], while any IPv4 address in Squid has some 1s due
to IPv4-to-IPv6 mapping used by Ip::Address. Those 1s could not match.

The "ipv4" parameter did match one address -- an IPv6 address `::`[^2].

This matching was possibly broken since inception (2009 commit 7764e92).

[^1]: Calling mask.setNoAddr() sets all sin6_addr bits to 1 while
calling mask.applyMask(0, AF_INET) does nothing at all.

[^2]: We did not set q->addr1. Ip::Address default constructor sets all
Ip::Address bits to 0. For ACL IP comparison purposes, that is
equivalent to an IPv6 `::` address (all sin6_addr bits are zero).

### Expand ipv6 ACL parameter scope to match all IPv6 addresses

The "ipv6" parameter was not matching many IPv6 addresses, including
some frequently seen ones like `::1` (i.e. IPv6 localhost). Now it
matches any IP address that Squid code itself considers an IPv6 address,
reducing surprises among admins that need to treat all IPv6 addresses
specially.

diff --git a/src/acl/Ip.cc b/src/acl/Ip.cc
index 6d2d202db72049ae48cd42305616dc80718421f8..657b817f350084639b3aefb1198a3e207f22aad7 100644 (file)
--- a/src/acl/Ip.cc
+++ b/src/acl/Ip.cc
@@ -221,103 +221,6 @@ acl_ip_data::FactoryParse(const char *t)
 
     debugs(28, 5, "aclIpParseIpData: " << t);
 
-    /* Special ACL RHS "all" matches entire Internet */
-    if (strcmp(t, "all") == 0) {
-        debugs(28, 9, "aclIpParseIpData: magic 'all' found.");
-        q->addr1.setAnyAddr();
-        q->addr2.setEmpty();
-        q->mask.setAnyAddr();
-        return q;
-    }
-
-    /* Detect some old broken strings equivalent to 'all'.
-     * treat them nicely. But be loud until its fixed.  */
-    if (strcmp(t, "0/0") == 0 || strcmp(t, "0.0.0.0/0") == 0 || strcmp(t, "0.0.0.0/0.0.0.0") == 0 ||
-            strcmp(t, "0.0.0.0-255.255.255.255") == 0 || strcmp(t, "0.0.0.0-0.0.0.0/0") == 0) {
-
-        debugs(28,DBG_CRITICAL, "ERROR: '" << t << "' needs to be replaced by the term 'all'.");
-        debugs(28,DBG_CRITICAL, "SECURITY NOTICE: Overriding config setting. Using 'all' instead.");
-        q->addr1.setAnyAddr();
-        q->addr2.setEmpty();
-        q->mask.setAnyAddr();
-        return q;
-    }
-
-    /* Special ACL RHS "ipv4" matches IPv4 Internet
-     * A nod to IANA; we include the entire class space in case
-     * they manage to find a way to recover and use it */
-    if (strcmp(t, "ipv4") == 0) {
-        q->mask.setNoAddr();
-        q->mask.applyMask(0, AF_INET);
-        return q;
-    }
-
-    /* Special ACL RHS "ipv6" matches IPv6-Unicast Internet */
-    if (strcmp(t, "ipv6") == 0) {
-        debugs(28, 9, "aclIpParseIpData: magic 'ipv6' found.");
-        r = q; // save head of the list for result.
-
-        /* 0000::/4 is a mix of localhost and obsolete IPv4-mapping space. Not valid outside this host. */
-
-        /* Future global unicast space: 1000::/4 */
-        q->addr1 = "1000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(4, AF_INET6);
-
-        /* Current global unicast space: 2000::/4 = (2000::/4 - 3000::/4) */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "2000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(3, AF_INET6);
-
-        /* Future global unicast space: 4000::/2 = (4000::/4 - 7000::/4) */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "4000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(2, AF_INET6);
-
-        /* Future global unicast space: 8000::/2 = (8000::/4 - B000::/4) */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "8000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(2, AF_INET6);
-
-        /* Future global unicast space: C000::/3 = (C000::/4 - D000::/4) */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "C000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(3, AF_INET6);
-
-        /* Future global unicast space: E000::/4 */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "E000::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(4, AF_INET6);
-
-        /* F000::/4 is mostly reserved non-unicast. With some exceptions ... */
-
-        /* RFC 4193 Unique-Local unicast space: FC00::/7 */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "FC00::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(7, AF_INET6);
-
-        /* Link-Local unicast space: FE80::/10 */
-        q->next = new acl_ip_data;
-        q = q->next;
-        q->addr1 = "FE80::";
-        q->mask.setNoAddr();
-        q->mask.applyMask(10, AF_INET6);
-
-        return r;
-    }
-
 // IPv4
     if (sscanf(t, SCAN_ACL1_4, addr1, addr2, mask) == 3) {
         debugs(28, 9, "aclIpParseIpData: '" << t << "' matched: SCAN1-v4: " << SCAN_ACL1_4);
@@ -471,6 +374,54 @@ acl_ip_data::FactoryParse(const char *t)
     return q;
 }
 
+/// handles special ACL data parameters that apply to the whole ACLIP object
+/// \returns true if input token is such a special parameter
+bool
+ACLIP::parseGlobal(const char * const token)
+{
+    // "all" matches entire Internet
+    if (strcmp(token, "all") == 0) {
+        debugs(28, 8, "found " << token);
+        matchAnyIpv4 = true;
+        matchAnyIpv6 = true;
+        // TODO: Ignore all other ACL data parameters, with a once/ACL warning.
+        return true;
+    }
+
+    // "ipv4" matches IPv4 Internet
+    if (strcmp(token, "ipv4") == 0) {
+        debugs(28, 8, "found " << token);
+        matchAnyIpv4 = true;
+        // TODO: Ignore all IPv4 data parameters, with a once/ACL warning.
+        return true;
+    }
+
+    // "ipv4" matches IPv6 Internet
+    if (strcmp(token, "ipv6") == 0) {
+        debugs(28, 8, "found " << token);
+        matchAnyIpv6 = true;
+        // TODO: Ignore all IPv6 data parameters, with a once/ACL warning.
+        return true;
+    }
+
+    /* Detect some old broken strings equivalent to 'all'.
+     * treat them nicely. But be loud until its fixed.  */
+    if (strcmp(token, "0/0") == 0 ||
+            strcmp(token, "0.0.0.0/0") == 0 ||
+            strcmp(token, "0.0.0.0/0.0.0.0") == 0 ||
+            strcmp(token, "0.0.0.0-255.255.255.255") == 0 ||
+            strcmp(token, "0.0.0.0-0.0.0.0/0") == 0) {
+
+        debugs(28,DBG_CRITICAL, "ERROR: '" << token << "' needs to be replaced by the term 'all'.");
+        debugs(28,DBG_CRITICAL, "SECURITY NOTICE: Overriding config setting. Using 'all' instead.");
+        matchAnyIpv4 = true;
+        matchAnyIpv6 = true;
+        return true;
+    }
+
+    return false;
+}
+
 void
 ACLIP::parse()
 {
@@ -478,6 +429,9 @@ ACLIP::parse()
         data = new IPSplay();
 
     while (char *t = ConfigParser::strtokFile()) {
+        if (parseGlobal(t))
+            continue;
+
         acl_ip_data *q = acl_ip_data::FactoryParse(t);
 
         while (q != nullptr) {
@@ -510,6 +464,14 @@ SBufList
 ACLIP::dump() const
 {
     IpAclDumpVisitor visitor;
+
+    if (matchAnyIpv4 && matchAnyIpv6)
+        visitor.contents.push_back(SBuf("all"));
+    else if (matchAnyIpv4)
+        visitor.contents.push_back(SBuf("ipv4"));
+    else if (matchAnyIpv6)
+        visitor.contents.push_back(SBuf("ipv6"));
+
     data->visit(visitor);
     return visitor.contents;
 }
@@ -517,12 +479,30 @@ ACLIP::dump() const
 bool
 ACLIP::empty() const
 {
-    return data->empty();
+    return data->empty() && !matchAnyIpv4 && !matchAnyIpv6;
 }
 
 int
 ACLIP::match(const Ip::Address &clientip)
 {
+    if (matchAnyIpv4) {
+        if (matchAnyIpv6) {
+            debugs(28, 3, clientip << " found, matched 'all'");
+            return true;
+        }
+        if (clientip.isIPv4()) {
+            debugs(28, 3, clientip << " found, matched 'ipv4'");
+            return true;
+        }
+        // fall through to look for an IPv6 match among IP parameters
+    } else if (matchAnyIpv6) {
+        if (clientip.isIPv6()) {
+            debugs(28, 3, clientip << " found, matched 'ipv6'");
+            return true;
+        }
+        // fall through to look for an IPv4 match among IP parameters
+    }
+
     static acl_ip_data ClientAddress;
     /*
      * aclIpAddrNetworkCompare() takes two acl_ip_data pointers as

diff --git a/src/acl/Ip.h b/src/acl/Ip.h
index 86754f46c35d938d163b0999e1e8e3273c15714e..10dd7ce36fb6fa437aac63a211d0023e351747f9 100644 (file)
--- a/src/acl/Ip.h
+++ b/src/acl/Ip.h
@@ -64,6 +64,14 @@ protected:
     int match(const Ip::Address &);
     IPSplay *data;
 
+private:
+    bool parseGlobal(const char *);
+
+    /// whether match() should return 1 for any IPv4 parameter
+    bool matchAnyIpv4 = false;
+
+    /// whether match() should return 1 for any IPv6 parameter
+    bool matchAnyIpv6 = false;
 };
 
 #endif /* SQUID_ACLIP_H */