Bug 5497: Fix detection of duped IPs returned by getaddrinfo() (#2100)

    WARNING: Ignoring <IP X> because it is already covered by <IP X>

Affects `src`, `dst`, and `localip` ACLs, especially those that use
domain names with multiple DNS A or AAAA records.

IP addresses returned by getaddrinfo(3) may not be sorted (e.g., when a
host has multiple DNS A RRs on FreeBSD). Instead of comparing the
current address with just the previous one, we now check all previously
added addresses (while processing a single getaddrinfo() call output).

This surgical fix minimizes changes without improving surrounding code.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index d63e6635129703a3ccbf04bc09e4153cac16f335..16912d69bdb83c60692a17471c61363d60763c09 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -37,6 +37,7 @@ Thank you!
     Amos Jeffries <squid3@treenet.co.nz>
     Amos Jeffries <yadij@users.noreply.github.com>
     Anatoli <me@anatoli.ws>
+    Andre Albsmeier <45147422+aafbsd@users.noreply.github.com>
     Andre Albsmeier <Andre.Albsmeier@siemens.com>
     Andrea Gagliardi <andrea@netlite.it>
     Andreas Hasenack <panlinux@gmail.com>

diff --git a/src/acl/Ip.cc b/src/acl/Ip.cc
index c0c709ddb16dde8a5ac1665f1a7171abf24106f9..60f83a6c3905ac0f4f2623e433c7ed9255b54486 100644 (file)
--- a/src/acl/Ip.cc
+++ b/src/acl/Ip.cc
@@ -214,6 +214,21 @@ acl_ip_data::DecodeMask(const char *asc, Ip::Address &mask, int ctype)
     return false;
 }
 
+/// whether we have parsed and vetted an item with an addr1 field that matches the needle
+bool
+acl_ip_data::containsVetted(const Ip::Address &needle) const
+{
+    // XXX: FactoryParse() adds an item and only then checks its validity. This
+    // loop excludes the last (i.e. being vetted) item. TODO: Refactor parsing
+    // to use a temporary container of vetted items instead of the current
+    // acl_ip_data::next hack.
+    for (const auto *i = this; i && i->next; i = i->next) {
+        if (i->addr1 == needle)
+            return true;
+    }
+    return false;
+}
+
 /* Handle either type of address, IPv6 will be discarded with a warning if disabled */
 #define SCAN_ACL1_6       "%[0123456789ABCDEFabcdef:]-%[0123456789ABCDEFabcdef:]/%[0123456789]"
 #define SCAN_ACL2_6       "%[0123456789ABCDEFabcdef:]-%[0123456789ABCDEFabcdef:]%c"
@@ -290,7 +305,6 @@ acl_ip_data::FactoryParse(const char *t)
         debugs(28, 5, "aclIpParseIpData: Lookup Host/IP " << addr1);
         struct addrinfo *hp = nullptr, *x = nullptr;
         struct addrinfo hints;
-        Ip::Address *prev_addr = nullptr;
 
         memset(&hints, 0, sizeof(struct addrinfo));
 
@@ -314,17 +328,15 @@ acl_ip_data::FactoryParse(const char *t)
             if ((r = *Q) == nullptr)
                 r = *Q = new acl_ip_data;
 
-            /* getaddrinfo given a host has a nasty tendency to return duplicate addr's */
-            /* BUT sorted fortunately, so we can drop most of them easily */
             r->addr1 = *x;
             x = x->ai_next;
-            if ( prev_addr && r->addr1 == *prev_addr) {
+            if (q->containsVetted(r->addr1)) {
+                // getaddrinfo() returned duplicate ai_addr values; we have already added one of them
                 debugs(28, 3, "aclIpParseIpData: Duplicate host/IP: '" << r->addr1 << "' dropped.");
                 delete r;
                 *Q = nullptr;
                 continue;
-            } else
-                prev_addr = &r->addr1;
+            }
 
             debugs(28, 3, "aclIpParseIpData: Located host/IP: '" << r->addr1 << "'");
 

diff --git a/src/acl/Ip.h b/src/acl/Ip.h
index cf320befa73c417047c373cf8352dc2fe2f442c7..2f4b8306c586b21091378294aa8e3a2ea383aec8 100644 (file)
--- a/src/acl/Ip.h
+++ b/src/acl/Ip.h
@@ -44,6 +44,8 @@ public:
 private:
 
     static bool DecodeMask(const char *asc, Ip::Address &mask, int string_format_type);
+
+    bool containsVetted(const Ip::Address &needle) const;
 };
 
 class ACLIP : public Acl::Node