From 9a3abf5a3be01c68f456b765314e8dd5599fcff5 Mon Sep 17 00:00:00 2001
From: Otto <otto.moerbeek@open-xchange.com>
Date: Wed, 17 Mar 2021 16:00:16 +0100
Subject: [PATCH] Make sure we take the right minimum for the PC TTL data in
 the SERVFAIL case.

Also add safety belt to the ageing code to not wrap TTLs,
adjust one dnsdist test for ageDNSPacket no longer underflowing, and
stop dnsdist from relying on ageDNSPacket wrapping around.
---
 pdns/dnsdist-cache.cc     | 8 +++++++-
 pdns/dnsparser.hh         | 6 +++++-
 pdns/pdns_recursor.cc     | 5 +++--
 pdns/test-dnsparser_cc.cc | 9 ++++-----
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/pdns/dnsdist-cache.cc b/pdns/dnsdist-cache.cc
index 915fc30d95..c1356a8874 100644
--- a/pdns/dnsdist-cache.cc
+++ b/pdns/dnsdist-cache.cc
@@ -285,7 +285,13 @@ bool DNSDistPacketCache::get(DNSQuestion& dq, uint16_t queryId, uint32_t* keyOut
   }
 
   if (!d_dontAge && !skipAging) {
-    ageDNSPacket(reinterpret_cast<char *>(&response[0]), response.size(), age);
+    if (!stale) {
+      ageDNSPacket(reinterpret_cast<char *>(&response[0]), response.size(), age);
+    }
+    else {
+      editDNSPacketTTL(reinterpret_cast<char *>(&response[0]), response.size(),
+        [staleTTL = d_staleTTL](uint8_t section, uint16_t class_, uint16_t type, uint32_t ttl) { return staleTTL; });
+    }
   }
 
   d_hits++;
diff --git a/pdns/dnsparser.hh b/pdns/dnsparser.hh
index 50c6bfd521..ad2aff9f80 100644
--- a/pdns/dnsparser.hh
+++ b/pdns/dnsparser.hh
@@ -514,7 +514,11 @@ public:
     uint32_t tmp;
     memcpy(&tmp, (void*) p, sizeof(tmp));
     tmp = ntohl(tmp);
-    tmp-=decrease;
+    if (tmp > decrease) {
+      tmp -= decrease;
+    } else {
+      tmp = 0;
+    }
     tmp = htonl(tmp);
     memcpy(d_packet + d_offset-4, (const char*)&tmp, sizeof(tmp));
   }
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index aadd7a2e7c..e29a72e947 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -2125,12 +2125,13 @@ static void startDoResolve(void *p)
       g_stats.variableResponses++;
     }
     if (!SyncRes::s_nopacketcache && !variableAnswer && !sr.wasVariable()) {
+      minTTL = min(minTTL, pw.getHeader()->rcode == RCode::ServFail ? SyncRes::s_packetcacheservfailttl :
+                   SyncRes::s_packetcachettl);
       t_packetCache->insertResponsePacket(dc->d_tag, dc->d_qhash, std::move(dc->d_query), dc->d_mdp.d_qname,
                                           dc->d_mdp.d_qtype, dc->d_mdp.d_qclass,
                                           string((const char*)&*packet.begin(), packet.size()),
                                           g_now.tv_sec,
-                                          pw.getHeader()->rcode == RCode::ServFail ? SyncRes::s_packetcacheservfailttl :
-                                          min(minTTL,SyncRes::s_packetcachettl),
+                                          minTTL,
                                           dq.validationState,
                                           std::move(pbDataForCache), dc->d_tcp);
     }
diff --git a/pdns/test-dnsparser_cc.cc b/pdns/test-dnsparser_cc.cc
index c1ac47079b..e71bf19fdb 100644
--- a/pdns/test-dnsparser_cc.cc
+++ b/pdns/test-dnsparser_cc.cc
@@ -131,13 +131,12 @@ BOOST_AUTO_TEST_CASE(test_ageDNSPacket) {
 
   BOOST_CHECK(firstPacket == expectedAlteredPacket);
 
-  /* now remove more than the remaining TTL, not that while TTL are,
-     per rfc1035 errata, "a 32 bit unsigned integer" so we should be
-     able to expect unsigned overflow to apply, but rfc2181 specifies
-     a maximum of "2^31 - 1". */
+  /* 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);
 
-  uint32_t ttl = std::numeric_limits<uint32_t>::max();
+  uint32_t ttl = 0;
 
   expectedAlteredPacket = generatePacket(ttl);
   BOOST_REQUIRE_EQUAL(firstPacket.size(), expectedAlteredPacket.size());
-- 
2.47.2