From: Christos Tsantilas Date: Wed, 3 Nov 2010 16:32:59 +0000 (+0200) Subject: Fixed DNS query leaks and increased defense against DNS cache poisoning. X-Git-Tag: take1~108 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=91dcbd43265a4a5886d68a993ebdc47f2713b62b;p=thirdparty%2Fsquid.git 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 bcc8f2f9d8..db88c54d00 100644 --- 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 91cf352376..eec881cad5 100644 --- 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 @@ -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 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);