From 7267213ac6c8431a4da6d93674d67ae855079f49 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 22 Feb 2016 19:22:55 +0100
Subject: [PATCH] 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).
---
 pdns/dnsdist.cc                         | 29 +++++++++++++++----------
 regression-tests.dnsdist/test_Basics.py | 28 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 4ce06dab9..b0b7d642e 100644
--- 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 73cc3cb6c..2f1b1de6a 100644
--- 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()
-- 
2.40.0