Fixed DNS query leaks and increased defense against DNS cache poisoning.

We were leaking (i.e. forgetting about) DNS queries under several conditions.
The most realistic leak case would go like this:

  - We send UDP query1.
    No response.

  - We send UDP query2.
    The response for query1 comes, with TC bit.

  - We try to connect over TCP, sending TCP query3.
    The response for query2 comes, with TC bit, matching TCP query3 ID.
    Since we are waiting a response over TCP, we drop the UDP response,
    and delete the query from the queue. We leak.

This change avoids forgetting the query under the above scenario.

Moreover, the above steps are hiding another problem: we are accepting responses
to timed out queries, making DNS cache poisoning easier. This change avoids
that by using unique query ID for each sent query. We have also added an
instance ID so that we still can track/identify a single "transaction" from
Squid point of view, even when that transaction involves many DNS query
messages.

When we forget about a DNS query, the caller may get stuck, holding a cbdata
lock. This is typical for ACLs that require domain name resolution, for example.
On a busy server with a long ACL list, the lock counter keeps growing due to
forgotten requests and may overflow, causing a "c->locks < 65535" assertion.
This change fixes the assertion unless there are more DNS leaks or different
lock leaks present.

This is a Measurement Factory project.

diff --git a/src/base/InstanceId.h b/src/base/InstanceId.h
index bcc8f2f9d868e70e0252c7e730d3aa2f548f2b23..db88c54d00a1703d97f32ffe618eae705d792a71 100644 (file)
--- a/src/base/InstanceId.h
+++ b/src/base/InstanceId.h
@@ -23,13 +23,14 @@ public:
     operator Value() const { return value; }
     bool operator ==(const InstanceId &o) const { return value == o.value; }
     bool operator !=(const InstanceId &o) const { return !(*this == o); }
+    void change() {value = ++Last ? Last : ++Last;}
 
     /// prints Prefix followed by ID value; \todo: use HEX for value printing?
     std::ostream &print(std::ostream &os) const;
 
 public:
     static const char *Prefix; ///< Class shorthand string for debugging
-    const Value value; ///< instance identifier
+    Value value; ///< instance identifier
 
 private:
     InstanceId(const InstanceId& right); ///< not implemented; IDs are unique

diff --git a/src/dns_internal.cc b/src/dns_internal.cc
index 91cf3523760ca14455422167474e736865dd8701..eec881cad5fde059ad014d8576e0fe7dc041ea45 100644 (file)
--- a/src/dns_internal.cc
+++ b/src/dns_internal.cc
@@ -45,6 +45,7 @@
 #include "mgr/Registration.h"
 #include "util.h"
 #include "wordlist.h"
+#include "base/InstanceId.h"
 
 #if HAVE_ARPA_NAMESER_H
 #include <arpa/nameser.h>
@@ -124,7 +125,9 @@ struct _idns_query {
     char name[NS_MAXDNAME + 1];
     char orig[NS_MAXDNAME + 1];
     size_t sz;
-    unsigned short id;
+    unsigned short msg_id; /// random query ID sent to server; changes with every query sent 
+    InstanceId<idns_query> xact_id; /// identifies our "transaction", stays constant when query is retried 
+
     int nsends;
     int need_vc;
 
@@ -145,6 +148,7 @@ struct _idns_query {
         rfc1035_rr *answers;
     } initial_AAAA;
 };
+InstanceIdDefinitions(idns_query,  "dns");
 
 struct _nsvc {
     int ns;
@@ -238,6 +242,7 @@ static PF idnsRead;
 static EVH idnsCheckQueue;
 static void idnsTickleQueue(void);
 static void idnsRcodeCount(int, int);
+static unsigned short idnsQueryID(void);
 
 static void
 idnsAddNameserver(const char *buf)
@@ -660,7 +665,7 @@ idnsStats(StoreEntry * sentry)
     for (n = lru_list.head; n; n = n->next) {
         q = (idns_query *)n->data;
         storeAppendPrintf(sentry, "%#06x %4d %5d %10.3f %9.3f\n",
-                          (int) q->id, (int) q->sz, q->nsends,
+                          (int) q->msg_id, (int) q->sz, q->nsends,
                           tvSubDsec(q->start_t, current_time),
                           tvSubDsec(q->sent_t, current_time));
     }
@@ -887,6 +892,10 @@ idnsSendQuery(idns_query * q)
     int x = -1, y = -1;
     int ns;
 
+    q->start_t = current_time;
+    q->msg_id = idnsQueryID();
+    rfc1035SetQueryID(q->buf, q->msg_id);
+
     do {
         ns = q->nsends % nns;
 
@@ -951,7 +960,7 @@ idnsFindQuery(unsigned short id)
     for (n = lru_list.tail; n; n = n->prev) {
         q = (idns_query*)n->data;
 
-        if (q->id == id)
+        if (q->msg_id == id)
             return q;
     }
 
@@ -1030,7 +1039,7 @@ idnsGrokReply(const char *buf, size_t sz, int from_ns)
         return;
     }
 
-    debugs(78, 3, "idnsGrokReply: ID 0x" << std::hex << message->id << ", " << std::dec << n << " answers");
+    debugs(78, 3, "idnsGrokReply: QID 0x" << std::hex <<   message->id << ", " << std::dec << n << " answers");
 
     q = idnsFindQuery(message->id);
 
@@ -1079,7 +1088,13 @@ idnsGrokReply(const char *buf, size_t sz, int from_ns)
             q->need_vc = 1;
             q->nsends--;
             idnsSendQuery(q);
-        }
+        } else {
+            // Strange: A TCP DNS response with the truncation bit (TC) set.
+            // Return an error and cleanup; no point in trying TCP again.
+            debugs(78, 3, HERE << "TCP DNS response");
+            idnsCallback(q, NULL, 0, "Truncated TCP DNS response");
+            cbdataFree(q);
+         }
 
         return;
     }
@@ -1099,9 +1114,6 @@ idnsGrokReply(const char *buf, size_t sz, int from_ns)
              */
             debugs(78, 3, "idnsGrokReply: Query result: SERV_FAIL");
             rfc1035MessageDestroy(&message);
-            q->start_t = current_time;
-            q->id = idnsQueryID();
-            rfc1035SetQueryID(q->buf, q->id);
             idnsSendQuery(q);
             return;
         }
@@ -1123,16 +1135,13 @@ idnsGrokReply(const char *buf, size_t sz, int from_ns)
 
             idnsDropMessage(message, q);
 
-            q->start_t = current_time;
-            q->id = idnsQueryID();
-            rfc1035SetQueryID(q->buf, q->id);
             if (Ip::EnableIpv6 && q->query.qtype == RFC1035_TYPE_AAAA) {
                 debugs(78, 3, "idnsGrokReply: Trying AAAA Query for " << q->name);
-                q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->id, &q->query, Config.dns.packet_max);
+                q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), 0, &q->query, Config.dns.packet_max);
             } else {
                 debugs(78, 3, "idnsGrokReply: Trying A Query for " << q->name);
                 // see EDNS notes at top of file why this sends 0
-                q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->id, &q->query, 0);
+                q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), 0, &q->query, 0);
             }
             idnsCacheQuery(q);
             idnsSendQuery(q);
@@ -1167,11 +1176,8 @@ idnsGrokReply(const char *buf, size_t sz, int from_ns)
 
         // reset the query as an A query
         q->nsends = 0;
-        q->start_t = current_time;
-        q->id = idnsQueryID();
-        rfc1035SetQueryID(q->buf, q->id);
         // see EDNS notes at top of file why this sends 0
-        q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->id, &q->query, 0);
+        q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), 0, &q->query, 0);
         q->need_A = false;
         idnsCacheQuery(q);
         idnsSendQuery(q);
@@ -1328,15 +1334,18 @@ idnsCheckQueue(void *unused)
             continue;
         }
 
-        debugs(78, 3, "idnsCheckQueue: ID 0x" << std::hex << std::setfill('0') << std::setw(4) << q->id << "timeout" );
+        debugs(78, 3, "idnsCheckQueue: ID " << q->xact_id <<
+               " QID 0x"  << std::hex << std::setfill('0')  <<
+               std::setw(4) << q->msg_id << ": timeout" );
 
         dlinkDelete(&q->lru, &lru_list);
 
         if (tvSubDsec(q->start_t, current_time) < Config.Timeout.idns_query) {
             idnsSendQuery(q);
         } else {
-            debugs(78, 2, "idnsCheckQueue: ID " << std::hex << q->id <<
-                   ": giving up after " << std::dec << q->nsends << " tries and " <<
+            debugs(78, 2, "idnsCheckQueue: ID " << q->xact_id <<
+                   " QID 0x" << std::hex << q->msg_id <<
+                   " : giving up after " << std::dec << q->nsends << " tries and " <<
                    std::setw(5)<< std::setprecision(2) << tvSubDsec(q->start_t, current_time) << " seconds");
 
             if (q->rcode != 0)
@@ -1577,6 +1586,8 @@ idnsCachedLookup(const char *key, IDNSCB * callback, void *data)
         return 0;
 
     q = cbdataAlloc(idns_query);
+    // idns_query is POD so no constructors are called after allocation
+    q->xact_id.change();
 
     q->callback = callback;
 
@@ -1607,8 +1618,8 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
         return;
 
     q = cbdataAlloc(idns_query);
-
-    q->id = idnsQueryID();
+    // idns_query is POD so no constructors are called after allocation
+    q->xact_id.change();
 
     for (i = 0; i < strlen(name); i++)
         if (name[i] == '.')
@@ -1631,11 +1642,11 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
     }
 
     if (Ip::EnableIpv6) {
-        q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), q->id, &q->query, Config.dns.packet_max);
+        q->sz = rfc3596BuildAAAAQuery(q->name, q->buf, sizeof(q->buf), 0, &q->query, Config.dns.packet_max);
         q->need_A = true;
     } else {
         // see EDNS notes at top of file why this sends 0
-        q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), q->id, &q->query, 0);
+        q->sz = rfc3596BuildAQuery(q->name, q->buf, sizeof(q->buf), 0, &q->query, 0);
         q->need_A = false;
     }
 
@@ -1647,14 +1658,12 @@ idnsALookup(const char *name, IDNSCB * callback, void *data)
     }
 
     debugs(78, 3, "idnsALookup: buf is " << q->sz << " bytes for " << q->name <<
-           ", id = 0x" << std::hex << q->id);
+           ", id = 0x" << std::hex << q->msg_id);
 
     q->callback = callback;
 
     q->callback_data = cbdataReference(data);
 
-    q->start_t = current_time;
-
     idnsCacheQuery(q);
 
     idnsSendQuery(q);
@@ -1671,17 +1680,18 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data)
 
     q = cbdataAlloc(idns_query);
 
-    q->id = idnsQueryID();
+    // idns_query is POD so no constructors are called after allocation
+    q->xact_id.change();
 
     if (Ip::EnableIpv6 && addr.IsIPv6()) {
         struct in6_addr addr6;
         addr.GetInAddr(addr6);
-        q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), q->id, &q->query, Config.dns.packet_max);
+        q->sz = rfc3596BuildPTRQuery6(addr6, q->buf, sizeof(q->buf), 0, &q->query, Config.dns.packet_max);
     } else {
         struct in_addr addr4;
         addr.GetInAddr(addr4);
         // see EDNS notes at top of file why this sends 0
-        q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), q->id, &q->query, 0);
+        q->sz = rfc3596BuildPTRQuery4(addr4, q->buf, sizeof(q->buf), 0, &q->query, 0);
     }
 
     /* PTR does not do inbound A/AAAA */
@@ -1700,14 +1710,12 @@ idnsPTRLookup(const Ip::Address &addr, IDNSCB * callback, void *data)
     }
 
     debugs(78, 3, "idnsPTRLookup: buf is " << q->sz << " bytes for " << ip <<
-           ", id = 0x" << std::hex << q->id);
+           ", id = 0x" << std::hex << q->msg_id);
 
     q->callback = callback;
 
     q->callback_data = cbdataReference(data);
 
-    q->start_t = current_time;
-
     idnsCacheQuery(q);
 
     idnsSendQuery(q);