dnsdist: Check that the answer matches the initial query over UDP

If we wrap around our maxOutstanding counter too fast, we need
to check that the answer we get is for the right query.
In order to do that, we now parse the question section in the
response and compare it to the one we expect (type, class and
name).

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 4ce06dab97c5c123bdf7a31d15086c4876a742e5..b0b7d642eaaa530040babf37f9bea6f80869d538 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -160,7 +160,8 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
   const uint16_t cdMask = 1 << FLAGS_CD_OFFSET;
   const uint16_t restoreFlagsMask = UINT16_MAX & ~(rdMask | cdMask);
   vector<uint8_t> rewrittenResponse;
-  
+  uint16_t qtype, qclass;
+
   struct dnsheader* dh = (struct dnsheader*)packet;
   for(;;) {
     ssize_t got = recv(state->fd, packet, sizeof(packet), 0);
@@ -182,15 +183,19 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
 
     if(origFD < 0) // duplicate
       continue;
-    else {
-      /* setting age to 0 to prevent the maintainer thread from
-         cleaning this IDS while we process the response.
-         We have already a copy of the origFD, so it would
-         mostly mess up the outstanding counter.
-      */
-      ids->age = 0;
-      --state->outstanding;  // you'd think an attacker could game this, but we're using connected socket
-    }
+
+    /* setting age to 0 to prevent the maintainer thread from
+       cleaning this IDS while we process the response.
+       We have already a copy of the origFD, so it would
+       mostly mess up the outstanding counter.
+    */
+    ids->age = 0;
+    unsigned int consumed;
+    DNSName qname(packet, responseLen, sizeof(dnsheader), false, &qtype, &qclass, &consumed);
+    if (qtype != ids->qtype || qclass != ids->qclass || qname != ids->qname)
+      continue;
+
+    --state->outstanding;  // you'd think an attacker could game this, but we're using connected socket
 
     if(g_fixupCase) {
       string realname = ids->qname.toDNSString();
@@ -248,7 +253,7 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
     g_stats.responses++;
 
     if (ids->packetCache && !ids->skipCache) {
-      ids->packetCache->insert(ids->cacheKey, ids->qname, ids->qtype, ids->qclass, response, responseLen, false);
+      ids->packetCache->insert(ids->cacheKey, qname, qtype, qclass, response, responseLen, false);
     }
 
 #ifdef HAVE_DNSCRYPT
@@ -284,7 +289,7 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
       struct timespec ts;
       clock_gettime(CLOCK_MONOTONIC, &ts);
       std::lock_guard<std::mutex> lock(g_rings.respMutex);
-      g_rings.respRing.push_back({ts, ids->origRemote, ids->qname, ids->qtype, (unsigned int)udiff, (unsigned int)got, *dh, state->remote});
+      g_rings.respRing.push_back({ts, ids->origRemote, qname, qtype, (unsigned int)udiff, (unsigned int)got, *dh, state->remote});
     }
     if(dh->rcode == RCode::ServFail)
       g_stats.servfailResponses++;

diff --git a/regression-tests.dnsdist/test_Basics.py b/regression-tests.dnsdist/test_Basics.py
index 73cc3cb6c24862a8a9518dc1a04bb68e2aa523c4..2f1b1de6aca3128d9f47edcded9b6ed678b050a8 100644 (file)
--- a/regression-tests.dnsdist/test_Basics.py
+++ b/regression-tests.dnsdist/test_Basics.py
@@ -261,6 +261,34 @@ class TestBasics(DNSDistTest):
         self.assertEquals(query, receivedQuery)
         self.assertEquals(response, receivedResponse)
 
+    def testWrongResponse(self):
+        """
+        Basics: Unrelated response from the backend
+
+        The backend send an unrelated answer over UDP, it should
+        be discarded by dnsdist. It could happen if we wrap around
+        maxOutstanding queries too quickly or have more than maxOustanding
+        queries to a specific backend in the air over UDP,
+        but does not really make sense over TCP.
+        """
+        name = 'query.unrelated.tests.powerdns.com.'
+        unrelatedName = 'answer.unrelated.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'TXT', 'IN')
+        unrelatedQuery = dns.message.make_query(unrelatedName, 'TXT', 'IN')
+        unrelatedResponse = dns.message.make_response(unrelatedQuery)
+        rrset = dns.rrset.from_text(unrelatedName,
+                                    3600,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.TXT,
+                                    'nothing to see here')
+        unrelatedResponse.answer.append(rrset)
+
+        (receivedQuery, receivedResponse) = self.sendUDPQuery(query, unrelatedResponse)
+        self.assertTrue(receivedQuery)
+        self.assertEquals(receivedResponse, None)
+        receivedQuery.id = query.id
+        self.assertEquals(query, receivedQuery)
+
 
 if __name__ == '__main__':
     unittest.main()