Merge pull request #16891 from omoerbeek/rec-aggr-cache-wrap

rec: handle NSEC3 records where hash(owner) > hash(next) in aggressive cache decision

Author: omoerbeek

pdns/recursordist/aggressive_nsec.cc

@@ -23,6 +23,7 @@
 #include <climits>
 
 #include "aggressive_nsec.hh"
+#include "base64.hh"
 #include "cachecleaner.hh"
 #include "recursor_cache.hh"
 #include "logger.hh"
@@ -231,16 +232,19 @@ static bool commonPrefixIsLong(const string& one, const string& two, size_t boun
   const auto minLength = std::min(one.length(), two.length());
 
   for (size_t i = 0; i < minLength; i++) {
-    const auto byte1 = one.at(i);
-    const auto byte2 = two.at(i);
+    const uint8_t byte1 = one.at(i);
+    const uint8_t byte2 = two.at(i);
     // shortcut
     if (byte1 == byte2) {
       length += CHAR_BIT;
-      if (length > bound) {
-        return true;
-      }
       continue;
     }
+    if (byte1 > byte2) { // order is reversed, implies large number of hashes covered
+      return false;
+    }
+    if (length > bound) {
+      return true;
+    }
     // bytes differ, let's look at the bits
     for (ssize_t j = CHAR_BIT - 1; j >= 0; j--) {
       const auto bit1 = byte1 & (1 << j);
@@ -322,11 +326,6 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner,
         return;
       }
 
-      if (isSmallCoveringNSEC3(owner, content->d_nexthash)) {
-        /* not accepting small covering answers since they only deny a small subset */
-        return;
-      }
-
       // XXX: Ponder storing everything in raw form, without the zone instead. It still needs to be a DNSName for NSEC, though,
       // but doing the conversion on cache hits only might be faster
       next = DNSName(toBase32Hex(content->d_nexthash)) + zone;

pdns/recursordist/aggressive_nsec.hh

@@ -92,7 +92,6 @@ public:
     return d_nsec3WildcardHits;
   }
 
-  // exported for unit test purposes
   static bool isSmallCoveringNSEC3(const DNSName& owner, const std::string& nextHash);
 
   void prune(time_t now);

pdns/recursordist/syncres.cc

@@ -4689,6 +4689,8 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
   }
 
   bool seenBogusRRSet = false;
+  std::vector<tcache_t::value_type> aggrCacheRecords;
+  bool insertIntoAggressiveCache = false;
   for (auto tCacheEntry = tcache.begin(); tCacheEntry != tcache.end(); ++tCacheEntry) {
 
     if (tCacheEntry->second.records.empty()) { // this happens when we did store signatures, but passed on the records themselves
@@ -4893,14 +4895,35 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, const string&
 
     if (g_aggressiveNSECCache && (tCacheEntry->first.type == QType::NSEC || tCacheEntry->first.type == QType::NSEC3) && recordState == vState::Secure && !seenAuth.empty()) {
       // Good candidate for NSEC{,3} caching
-      g_aggressiveNSECCache->insertNSEC(seenAuth, tCacheEntry->first.name, tCacheEntry->second.records.at(0), tCacheEntry->second.signatures, tCacheEntry->first.type == QType::NSEC3, qname, qtype);
+      aggrCacheRecords.emplace_back(*tCacheEntry);
+      if (!insertIntoAggressiveCache) {
+        if (tCacheEntry->first.type == QType::NSEC) {
+          // NSECs have no additonal condition, just take them
+          insertIntoAggressiveCache = true;
+        }
+        else if (tCacheEntry->first.type == QType::NSEC3 && !AggressiveNSECCache::nsec3Disabled()) {
+          // If at least one of the NSEC3s covers quite some names, we will take all of them, as likely all of them are needed for a denial proof.
+          if (auto content = getRR<NSEC3RecordContent>(tCacheEntry->second.records.at(0)); content != nullptr) {
+            if (!AggressiveNSECCache::isSmallCoveringNSEC3(tCacheEntry->first.name, content->d_nexthash)) {
+              insertIntoAggressiveCache = true;
+            }
+          }
+        }
+      }
     }
 
     if (tCacheEntry->first.place == DNSResourceRecord::ANSWER && ednsmask) {
       d_wasVariable = true;
     }
   }
 
+  // The primary loop determined if we want to take the NSEC(3) records
+  if (insertIntoAggressiveCache) {
+    for (const auto& entry : aggrCacheRecords) {
+      g_aggressiveNSECCache->insertNSEC(seenAuth, entry.first.name, entry.second.records.at(0), entry.second.signatures, entry.first.type == QType::NSEC3, qname, qtype);
+    }
+  }
+
   if (gatherWildcardProof) {
     if (auto wcIt = wildcardCandidates.find(qname); wcIt != wildcardCandidates.end() && !wcIt->second) {
       // the queried name was not expanded from a wildcard, a record in the CNAME chain was, so we don't need to gather wildcard proof now: we will do that when looking up the CNAME chain

pdns/recursordist/test-aggressive_nsec_cc.cc

@@ -24,6 +24,7 @@ BOOST_AUTO_TEST_CASE(test_small_covering_nsec3)
     {"8ujhshp2lhmnpoo9qde4blg4gq3hgl99", "8ujhshp2lhmnpoo9qde4blg4gq3hgl99", 0, false},
     {"8ujhshp2lhmnpoo9qde4blg4gq3hgl99", "8ujhshp2lhmnpoo9qde4blg4gq3hgl99", 1, false},
     {"8ujhshp2lhmnpoo9qde4blg4gq3hgl99", "8ujhshp2lhmnpoo9qde4blg4gq3hgl99", 157, false},
+    {"e731tdlrdip60smlihgnprqdspq0idlp", "e731tcnkl5al8t4r64b292blt4c37j3h", 1, false}, // note order: 1st > 2nd
   };
 
   for (const auto& [owner, next, boundary, result] : table) {