From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Thu, 16 Mar 2023 07:41:43 +0000 (+0100)
Subject: PowerDNS Security Advisory 2023-02: Deterred spoofing attempts can lead to authoritat... 
X-Git-Tag: rec-4.6.6^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F12702%2Fhead;p=thirdparty%2Fpdns.git

PowerDNS Security Advisory 2023-02: Deterred spoofing attempts can lead to authoritative servers being marked unavailable (CVE-2023-26437)
---

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index c0a62d2f2b..78907dc25f 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -4525,35 +4525,39 @@ static void doResends(MT_t::waiters_t::iterator& iter, const std::shared_ptr<Pac
 static void handleUDPServerResponse(int fd, FDMultiplexer::funcparam_t& var)
 {
   std::shared_ptr<PacketID> pid = boost::any_cast<std::shared_ptr<PacketID>>(var);
-  ssize_t len;
   PacketBuffer packet;
   packet.resize(g_outgoingEDNSBufsize);
   ComboAddress fromaddr;
-  socklen_t addrlen=sizeof(fromaddr);
+  socklen_t addrlen = sizeof(fromaddr);
 
-  len=recvfrom(fd, &packet.at(0), packet.size(), 0, (sockaddr *)&fromaddr, &addrlen);
+  ssize_t len = recvfrom(fd, &packet.at(0), packet.size(), 0, reinterpret_cast<sockaddr*>(&fromaddr), &addrlen);
 
-  if(len < (ssize_t) sizeof(dnsheader)) {
-    if(len < 0)
-      ; //      cerr<<"Error on fd "<<fd<<": "<<stringerror()<<"\n";
-    else {
-      g_stats.serverParseError++;
-      if(g_logCommonErrors)
-        g_log<<Logger::Error<<"Unable to parse packet from remote UDP server "<< fromaddr.toString() <<
-          ": packet smaller than DNS header"<<endl;
-    }
+  const ssize_t signed_sizeof_sdnsheader = sizeof(dnsheader);
 
+  if (len < 0) {
+    // len < 0: error on socket
     t_udpclientsocks->returnSocket(fd);
-    PacketBuffer empty;
 
-    MT_t::waiters_t::iterator iter=MT->d_waiters.find(pid);
-    if(iter != MT->d_waiters.end())
+    PacketBuffer empty;
+    MT_t::waiters_t::iterator iter = MT->d_waiters.find(pid);
+    if (iter != MT->d_waiters.end()) {
       doResends(iter, pid, empty);
+    }
+    MT->sendEvent(pid, &empty); // this denotes error (does retry lookup using other NS)
+    return;
+  }
 
-    MT->sendEvent(pid, &empty); // this denotes error (does lookup again.. at least L1 will be hot)
+  if (len < signed_sizeof_sdnsheader) {
+    // We have received a packet that cannot be a valid DNS packet, as it has no complete header
+    // Drop it, but continue to wait for other packets
+    g_stats.serverParseError++;
+    if (g_logCommonErrors) {
+      g_log << Logger::Error << "Unable to parse too short packet from remote UDP server " << fromaddr.toString() << ": packet smaller than DNS header" << endl;
+    }
     return;
   }
 
+  // We have at least a full header
   packet.resize(len);
   dnsheader dh;
   memcpy(&dh, &packet.at(0), sizeof(dh));
@@ -4563,41 +4567,50 @@ static void handleUDPServerResponse(int fd, FDMultiplexer::funcparam_t& var)
   pident->id = dh.id;
   pident->fd = fd;
 
-  if(!dh.qr && g_logCommonErrors) {
-    g_log<<Logger::Notice<<"Not taking data from question on outgoing socket from "<< fromaddr.toStringWithPort()  <<endl;
+  if (!dh.qr && g_logCommonErrors) {
+    g_log << Logger::Notice << "Not taking data from question on outgoing socket from " << fromaddr.toStringWithPort() << endl;
   }
 
-  if(!dh.qdcount || // UPC, Nominum, very old BIND on FormErr, NSD
-     !dh.qr) {      // one weird server
+  if (!dh.qdcount || // UPC, Nominum, very old BIND on FormErr, NSD
+      !dh.qr) { // one weird server
     pident->domain.clear();
     pident->type = 0;
   }
   else {
     try {
-      if(len > 12)
-        pident->domain=DNSName(reinterpret_cast<const char *>(packet.data()), len, 12, false, &pident->type); // don't copy this from above - we need to do the actual read
+      if (len > signed_sizeof_sdnsheader) {
+        pident->domain = DNSName(reinterpret_cast<const char*>(packet.data()), len, static_cast<int>(sizeof(dnsheader)), false, &pident->type); // don't copy this from above - we need to do the actual read
+      }
+      else {
+        // len == sizeof(dnsheader), only header case
+        // We will do a full scan search later to see if we can match this reply even without a domain
+        pident->domain.clear();
+        pident->type = 0;
+      }
     }
-    catch(std::exception& e) {
+    catch (std::exception& e) {
+      // Parse error, continue waiting for other packets
       g_stats.serverParseError++; // won't be fed to lwres.cc, so we have to increment
-      g_log<<Logger::Warning<<"Error in packet from remote nameserver "<< fromaddr.toStringWithPort() << ": "<<e.what() << endl;
+      g_log << Logger::Warning << "Error in packet from remote nameserver " << fromaddr.toStringWithPort() << ": " << e.what() << endl;
       return;
     }
   }
 
-  MT_t::waiters_t::iterator iter=MT->d_waiters.find(pident);
-  if(iter != MT->d_waiters.end()) {
-    doResends(iter, pident, packet);
+  if (!pident->domain.empty()) {
+    MT_t::waiters_t::iterator iter = MT->d_waiters.find(pident);
+    if (iter != MT->d_waiters.end()) {
+      doResends(iter, pident, packet);
+    }
   }
 
 retryWithName:
 
-  if(!MT->sendEvent(pident, &packet)) {
+  if (pident->domain.empty() || MT->sendEvent(pident, &packet) == 0) {
     /* we did not find a match for this response, something is wrong */
 
     // we do a full scan for outstanding queries on unexpected answers. not too bad since we only accept them on the right port number, which is hard enough to guess
     for (MT_t::waiters_t::iterator mthread = MT->d_waiters.begin(); mthread != MT->d_waiters.end(); ++mthread) {
-      if (pident->fd == mthread->key->fd && mthread->key->remote == pident->remote &&  mthread->key->type == pident->type &&
-         pident->domain == mthread->key->domain) {
+      if (pident->fd == mthread->key->fd && mthread->key->remote == pident->remote && mthread->key->type == pident->type && pident->domain == mthread->key->domain) {
         /* we are expecting an answer from that exact source, on that exact port (since we are using connected sockets), for that qname/qtype,
            but with a different message ID. That smells like a spoofing attempt. For now we will just increase the counter and will deal with
            that later. */
@@ -4605,8 +4618,7 @@ retryWithName:
       }
 
       // be a bit paranoid here since we're weakening our matching
-      if(pident->domain.empty() && !mthread->key->domain.empty() && !pident->type && mthread->key->type &&
-         pident->id  == mthread->key->id && mthread->key->remote == pident->remote) {
+      if (pident->domain.empty() && !mthread->key->domain.empty() && !pident->type && mthread->key->type && pident->id == mthread->key->id && mthread->key->remote == pident->remote) {
         // cerr<<"Empty response, rest matches though, sending to a waiter"<<endl;
         pident->domain = mthread->key->domain;
         pident->type = mthread->key->type;
@@ -4614,11 +4626,11 @@ retryWithName:
       }
     }
     g_stats.unexpectedCount++; // if we made it here, it really is an unexpected answer
-    if(g_logCommonErrors) {
-      g_log<<Logger::Warning<<"Discarding unexpected packet from "<<fromaddr.toStringWithPort()<<": "<< (pident->domain.empty() ? "<empty>" : pident->domain.toString())<<", "<<pident->type<<", "<<MT->d_waiters.size()<<" waiters"<<endl;
+    if (g_logCommonErrors) {
+      g_log << Logger::Warning << "Discarding unexpected packet from " << fromaddr.toStringWithPort() << ": " << (pident->domain.empty() ? "<empty>" : pident->domain.toString()) << ", " << pident->type << ", " << MT->d_waiters.size() << " waiters" << endl;
     }
   }
-  else if(fd >= 0) {
+  else if (fd >= 0) {
     /* we either found a waiter (1) or encountered an issue (-1), it's up to us to clean the socket anyway */
     t_udpclientsocks->returnSocket(fd);
   }
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 5860aaa420..bab4beda4a 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -3984,6 +3984,12 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname,
   }
 
   d_totUsec += lwr.d_usec;
+
+  if (resolveret == LWResult::Result::Spoofed) {
+    spoofed = true;
+    return false;
+  }
+
   accountAuthLatency(lwr.d_usec, remoteIP.sin4.sin_family);
 
   bool dontThrottle = false;
@@ -4015,9 +4021,6 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname,
       LOG(prefix<<qname<<": hit a local resource limit resolving"<< (doTCP ? " over TCP" : "")<<", probable error: "<<stringerror()<<endl);
       g_stats.resourceLimits++;
     }
-    else if (resolveret == LWResult::Result::Spoofed) {
-      spoofed = true;
-    }
     else {
       /* -1 means server unreachable */
       s_unreachables++;