From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Tue, 21 Mar 2023 12:34:35 +0000 (+0100)
Subject: rec and dnsdist: fix a case of potential unaligned header access
X-Git-Tag: dnsdist-1.8.0~5^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=100ff9c71085c0418f2cced4d2054210af01d1b8;p=thirdparty%2Fpdns.git

rec and dnsdist: fix a case of potential unaligned header access

I addded an argument to ageDNSPacket to circumvent having to do it in
two places in rec.

I am also wondering if the break in ageDNSPakcet() is right.
I suspect we want to continue with other records even if we see an OPT
(which does not *have* to be the last as far as I know)
---

diff --git a/pdns/dnsdist-cache.cc b/pdns/dnsdist-cache.cc
index a5bd944ee9..7ca9be2f6e 100644
--- a/pdns/dnsdist-cache.cc
+++ b/pdns/dnsdist-cache.cc
@@ -296,7 +296,8 @@ bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut
   if (!d_dontAge && !skipAging) {
     if (!stale) {
       // coverity[store_truncates_time_t]
-      ageDNSPacket(reinterpret_cast<char *>(&response[0]), response.size(), age);
+      dnsheader_aligned dh_aligned(response.data());
+      ageDNSPacket(reinterpret_cast<char *>(&response[0]), response.size(), age, dh_aligned);
     }
     else {
       editDNSPacketTTL(reinterpret_cast<char*>(&response[0]), response.size(),
diff --git a/pdns/dnsparser.cc b/pdns/dnsparser.cc
index 0b486edef3..e726ef24e2 100644
--- a/pdns/dnsparser.cc
+++ b/pdns/dnsparser.cc
@@ -925,47 +925,45 @@ void clearDNSPacketRecordTypes(PacketBuffer& packet, const std::unordered_set<QT
 }
 
 // method of operation: silently fail if it doesn't work - we're only trying to be nice, don't fall over on it
-void ageDNSPacket(char* packet, size_t length, uint32_t seconds)
+void ageDNSPacket(char* packet, size_t length, uint32_t seconds, const dnsheader_aligned& aligned_dh)
 {
-  if(length < sizeof(dnsheader))
+  if (length < sizeof(dnsheader)) {
     return;
-  try
-  {
-    const dnsheader* dh = reinterpret_cast<const dnsheader*>(packet);
-    const uint64_t dqcount = ntohs(dh->qdcount);
-    const uint64_t numrecords = ntohs(dh->ancount) + ntohs(dh->nscount) + ntohs(dh->arcount);
+  }
+  try {
+    const dnsheader* dhp = aligned_dh.get();
+    const uint64_t dqcount = ntohs(dhp->qdcount);
+    const uint64_t numrecords = ntohs(dhp->ancount) + ntohs(dhp->nscount) + ntohs(dhp->arcount);
     DNSPacketMangler dpm(packet, length);
 
-    uint64_t n;
-    for(n=0; n < dqcount; ++n) {
+    for (uint64_t rec = 0; rec < dqcount; ++rec) {
       dpm.skipDomainName();
       /* type and class */
       dpm.skipBytes(4);
     }
-   // cerr<<"Skipped "<<n<<" questions, now parsing "<<numrecords<<" records"<<endl;
-    for(n=0; n < numrecords; ++n) {
+
+    for(uint64_t rec = 0; rec < numrecords; ++rec) {
       dpm.skipDomainName();
 
       uint16_t dnstype = dpm.get16BitInt();
       /* class */
       dpm.skipBytes(2);
 
-      if(dnstype == QType::OPT) // not aging that one with a stick
+      if (dnstype == QType::OPT) { // not aging that one with a stick
         break;
+      }
 
       dpm.decreaseAndSkip32BitInt(seconds);
       dpm.skipRData();
     }
   }
-  catch(...)
-  {
-    return;
+  catch(...) {
   }
 }
 
-void ageDNSPacket(std::string& packet, uint32_t seconds)
+void ageDNSPacket(std::string& packet, uint32_t seconds, const dnsheader_aligned& aligned_dh)
 {
-  ageDNSPacket((char*)packet.c_str(), packet.length(), seconds);
+  ageDNSPacket(packet.data(), packet.length(), seconds, aligned_dh);
 }
 
 uint32_t getDNSPacketMinTTL(const char* packet, size_t length, bool* seenAuthSOA)
diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh
index c19c766c5e..e6ce0fa11d 100644
--- a/pdns/dnsparser.hh
+++ b/pdns/dnsparser.hh
@@ -471,8 +471,8 @@ private:
 };
 
 string simpleCompress(const string& label, const string& root="");
-void ageDNSPacket(char* packet, size_t length, uint32_t seconds);
-void ageDNSPacket(std::string& packet, uint32_t seconds);
+void ageDNSPacket(char* packet, size_t length, uint32_t seconds, const dnsheader_aligned&);
+void ageDNSPacket(std::string& packet, uint32_t seconds, const dnsheader_aligned&);
 void editDNSPacketTTL(char* packet, size_t length, const std::function<uint32_t(uint8_t, uint16_t, uint16_t, uint32_t)>& visitor);
 void clearDNSPacketRecordTypes(vector<uint8_t>& packet, const std::unordered_set<QType>& qtypes);
 void clearDNSPacketRecordTypes(PacketBuffer& packet, const std::unordered_set<QType>& qtypes);
diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc
index 64d56f4376..4ee85eddf5 100644
--- a/pdns/recursordist/pdns_recursor.cc
+++ b/pdns/recursordist/pdns_recursor.cc
@@ -1979,11 +1979,12 @@ bool checkForCacheHit(bool qnameParsed, unsigned int tag, const string& data,
 
     t_Counters.at(rec::Counter::packetCacheHits)++;
     t_Counters.at(rec::Counter::syncresqueries)++; // XXX
-    ageDNSPacket(response, age);
     if (response.length() >= sizeof(struct dnsheader)) {
-      const struct dnsheader* dh = reinterpret_cast<const dnsheader*>(response.data());
-      updateResponseStats(dh->rcode, source, response.length(), nullptr, 0);
-      t_Counters.at(rec::ResponseStats::responseStats).submitResponse(qtype, response.length(), dh->rcode);
+      dnsheader_aligned dh_aligned(response.data());
+      ageDNSPacket(response, age, dh_aligned);
+      const auto* dhp = dh_aligned.get();
+      updateResponseStats(dhp->rcode, source, response.length(), nullptr, 0);
+      t_Counters.at(rec::ResponseStats::responseStats).submitResponse(qtype, response.length(), dhp->rcode);
     }
 
     // we assume 0 usec
diff --git a/pdns/test-dnsparser_cc.cc b/pdns/test-dnsparser_cc.cc
index 37f97d29fe..05894f3343 100644
--- a/pdns/test-dnsparser_cc.cc
+++ b/pdns/test-dnsparser_cc.cc
@@ -117,7 +117,8 @@ BOOST_AUTO_TEST_CASE(test_ageDNSPacket) {
   auto firstPacket = generatePacket(3600);
   auto expectedAlteredPacket = generatePacket(1800);
 
-  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size(), 1800);
+  dnsheader_aligned dh_aligned(firstPacket.data());
+  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size(), 1800, dh_aligned);
 
   BOOST_REQUIRE_EQUAL(firstPacket.size(), expectedAlteredPacket.size());
   for (size_t idx = 0; idx < firstPacket.size(); idx++) {
@@ -127,14 +128,14 @@ BOOST_AUTO_TEST_CASE(test_ageDNSPacket) {
 
   /* now call it with a truncated packet, missing the last TTL and rdata,
      the packet should not be altered. */
-  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size() - sizeof(uint32_t) - /* rdata length */ sizeof (uint16_t) - /* IPv4 payload in rdata */ 4 - /* size of OPT record */ 11, 900);
+  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size() - sizeof(uint32_t) - /* rdata length */ sizeof (uint16_t) - /* IPv4 payload in rdata */ 4 - /* size of OPT record */ 11, 900, dh_aligned);
 
   BOOST_CHECK(firstPacket == expectedAlteredPacket);
 
   /* now remove more than the remaining TTL. We expect ageDNSPacket
      to cap this at zero and not cause an unsigned underflow into
      the 2^32-1 neighbourhood */
-  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size(), 1801);
+  ageDNSPacket(reinterpret_cast<char*>(firstPacket.data()), firstPacket.size(), 1801, dh_aligned);
 
   uint32_t ttl = 0;