From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 5 Jul 2021 16:01:37 +0000 (+0200)
Subject: rec: Make really sure we did not miss a cut on validation failure
X-Git-Tag: dnsdist-1.7.0-alpha1~84^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc50925a50c6e956f612bb03db15893279fd052d;p=thirdparty%2Fpdns.git

rec: Make really sure we did not miss a cut on validation failure
---

diff --git a/pdns/recursordist/test-syncres_cc6.cc b/pdns/recursordist/test-syncres_cc6.cc
index 9810e0161f..c83e1e9551 100644
--- a/pdns/recursordist/test-syncres_cc6.cc
+++ b/pdns/recursordist/test-syncres_cc6.cc
@@ -227,7 +227,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusSelfSignedDS);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 7U);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
 
   /* again, to test the cache */
   ret.clear();
@@ -235,7 +235,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusSelfSignedDS);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 7U);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_ds_root)
@@ -393,7 +393,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoValidRRSIG);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 6U);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
 
   /* again, to test the cache */
   ret.clear();
@@ -401,7 +401,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child)
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoValidRRSIG);
   BOOST_REQUIRE_EQUAL(ret.size(), 2U);
-  BOOST_CHECK_EQUAL(queriesCount, 6U);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_unpublished)
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 4c34384137..a60339e062 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1714,7 +1714,7 @@ void SyncRes::computeNegCacheValidationStatus(const NegCache::NegCacheEntry& ne,
     vState neValidationState = ne.d_validationState;
     dState expectedState = res == RCode::NXDomain ? dState::NXDOMAIN : dState::NXQTYPE;
     dState denialState = getDenialValidationState(ne, expectedState, false);
-    updateDenialValidationState(neValidationState, ne.d_name, state, denialState, expectedState);
+    updateDenialValidationState(neValidationState, ne.d_name, state, denialState, expectedState, qtype == QType::DS, depth);
   }
   if (state != vState::Indeterminate) {
     /* validation succeeded, let's update the cache entry so we don't have to validate again */
@@ -2711,24 +2711,32 @@ vState SyncRes::getValidationStatus(const DNSName& name, bool hasSignatures, boo
 vState SyncRes::validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord>& dnskeys, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures, unsigned int depth)
 {
   dsmap_t ds;
-  if (!signatures.empty()) {
-    DNSName signer = getSigner(signatures);
+  if (signatures.empty()) {
+    LOG(d_prefix<<": we have "<<std::to_string(dnskeys.size())<<" DNSKEYs but no signature, going Bogus!"<<endl);
+    return vState::BogusNoRRSIG;
+  }
 
-    if (!signer.empty() && zone.isPartOf(signer)) {
-      vState state = getDSRecords(signer, ds, false, depth);
+  DNSName signer = getSigner(signatures);
 
-      if (state != vState::Secure) {
-        return state;
-      }
-    }
-    else {
-      LOG(d_prefix<<": we have "<<std::to_string(dnskeys.size())<<" DNSKEYs but the zone ("<<zone<<") is not part of the signer ("<<signer<<"), going Bogus!"<<endl);
-      return vState::BogusNoValidRRSIG;
+  if (!signer.empty() && zone.isPartOf(signer)) {
+    vState state = getDSRecords(signer, ds, false, depth);
+
+    if (state != vState::Secure) {
+      return state;
     }
   }
   else {
-    LOG(d_prefix<<": we have "<<std::to_string(dnskeys.size())<<" DNSKEYs but no signature, going Bogus!"<<endl);
-    return vState::BogusNoRRSIG;
+    LOG(d_prefix<<": we have "<<std::to_string(dnskeys.size())<<" DNSKEYs but the zone ("<<zone<<") is not part of the signer ("<<signer<<"), check that we did not miss a zone cut"<<endl);
+    /* try again to get the missed cuts, harder this time */
+    auto zState = getValidationStatus(zone, false, false, depth);
+    if (zState == vState::Secure) {
+      /* too bad */
+      LOG(d_prefix<<": after checking the zone cuts again, we still have "<<std::to_string(dnskeys.size())<<" DNSKEYs and the zone ("<<zone<<") is still not part of the signer ("<<signer<<"), going Bogus!"<<endl);
+      return vState::BogusNoValidRRSIG;
+    }
+    else {
+      return zState;
+    }
   }
 
   skeyset_t tentativeKeys;
@@ -2755,8 +2763,17 @@ vState SyncRes::validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord
      we haven't found at least one DNSKEY and a matching RRSIG
      covering this set, this looks Bogus. */
   if (validatedKeys.size() != tentativeKeys.size()) {
-    LOG(d_prefix<<": returning Bogus state from "<<__func__<<"("<<zone<<")"<<endl);
-    return state;
+    LOG(d_prefix<<": let's check whether we missed a zone cut before returning a Bogus state from "<<__func__<<"("<<zone<<")"<<endl);
+    /* try again to get the missed cuts, harder this time */
+    auto zState = getValidationStatus(zone, false, false, depth);
+    if (zState == vState::Secure) {
+      /* too bad */
+      LOG(d_prefix<<": after checking the zone cuts we are still in a Secure zone, returning Bogus state from "<<__func__<<"("<<zone<<")"<<endl);
+      return state;
+    }
+    else {
+      return zState;
+    }
   }
 
   return state;
@@ -2799,45 +2816,70 @@ vState SyncRes::getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int
 vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname, const QType qtype, const DNSName& name, const QType type, const std::vector<DNSRecord>& records, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures)
 {
   skeyset_t keys;
-  if (!signatures.empty()) {
-    const DNSName signer = getSigner(signatures);
-    if (!signer.empty() && name.isPartOf(signer)) {
-      if ((qtype == QType::DNSKEY || qtype == QType::DS) && signer == qname) {
-        /* we are already retrieving those keys, sorry */
-        if (type == QType::DS && signer == name && !signer.isRoot()) {
-          /* Unless we are getting the DS of the root zone, we should never see a
-             DS (or a denial of a DS) signed by the DS itself, since we should be
-             requesting it from the parent zone. Something is very wrong */
-          LOG(d_prefix<<"The DS for "<<qname<<" is signed by itself, going Bogus"<<endl);
-          return vState::BogusSelfSignedDS;
-        }
-        if (qtype == QType::DNSKEY && signer == qname) {
-          /* that actually does happen when a server returns NS records in authority
-             along with the DNSKEY, leading us to trying to validate the RRSIGs for
-             the NS with the DNSKEY that we are about to process. */
-          if ((name == signer && type == QType::NSEC) || type == QType::NSEC3) {
-            /* if we are trying to validate the DNSKEY (should not happen here),
-               or more likely NSEC(3)s proving that it does not exist, we have a problem.
-               In that case let's see if the DS does exist, and if it does let's go Bogus
-            */
-            dsmap_t results;
-            vState dsState = getDSRecords(signer, results, false, depth, true);
-            if (vStateIsBogus(dsState) || dsState == vState::Insecure) {
-              return dsState;
-            }
-            return vState::BogusUnableToGetDNSKEYs;
+  if (signatures.empty()) {
+    LOG(d_prefix<<"Bogus!"<<endl);
+    return vState::BogusNoRRSIG;
+  }
+
+  const DNSName signer = getSigner(signatures);
+  if (!signer.empty() && name.isPartOf(signer)) {
+    vState state = vState::Secure;
+
+    if ((qtype == QType::DNSKEY || qtype == QType::DS) && signer == qname) {
+      /* we are already retrieving those keys, sorry */
+      if (type == QType::DS && signer == name && !signer.isRoot()) {
+        /* Unless we are getting the DS of the root zone, we should never see a
+           DS (or a denial of a DS) signed by the DS itself, since we should be
+           requesting it from the parent zone. Something is very wrong */
+        LOG(d_prefix<<"The DS for "<<qname<<" is signed by itself"<<endl);
+        state = vState::BogusSelfSignedDS;
+      }
+      else if (qtype == QType::DNSKEY && signer == qname) {
+        /* that actually does happen when a server returns NS records in authority
+           along with the DNSKEY, leading us to trying to validate the RRSIGs for
+           the NS with the DNSKEY that we are about to process. */
+        if ((name == signer && type == QType::NSEC) || type == QType::NSEC3) {
+          /* if we are trying to validate the DNSKEY (should not happen here),
+             or more likely NSEC(3)s proving that it does not exist, we have a problem.
+             In that case let's see if the DS does exist, and if it does let's go Bogus
+          */
+          dsmap_t results;
+          vState dsState = getDSRecords(signer, results, false, depth, true);
+          if (vStateIsBogus(dsState) || dsState == vState::Insecure) {
+            state = dsState;
           }
+          else {
+            LOG(d_prefix<<"Unable to get the DS for "<<signer<<endl);
+            state = vState::BogusUnableToGetDNSKEYs;
+          }
+        }
+        else {
+          /* return immediately since looking at the cuts is not going to change the
+             fact that we are looking at a signature done with the key we are trying to
+             obtain */
+          LOG(d_prefix<<"we are looking at a signature done with the key we are trying to obtain "<<signer<<endl);
           return vState::Indeterminate;
         }
       }
-      vState state = getDNSKeys(signer, keys, depth);
-      if (state != vState::Secure) {
+    }
+    if (state == vState::Secure) {
+      LOG(d_prefix<<"retrieving the DNSKEYs for "<<signer<<endl);
+      state = getDNSKeys(signer, keys, depth);
+    }
+
+    if (state != vState::Secure) {
+      /* try again to get the missed cuts, harder this time */
+      LOG(d_prefix<<"checking whether we missed a zone cut before returning a Bogus state"<<endl);
+      auto zState = getValidationStatus(name, false, type == QType::DS, depth);
+      if (zState == vState::Secure) {
+        LOG(d_prefix<<"we are still in a Secure zone, returning "<<vStateToString(state)<<endl);
+        /* too bad */
         return state;
       }
+      else {
+        return zState;
+      }
     }
-  } else {
-    LOG(d_prefix<<"Bogus!"<<endl);
-    return vState::BogusNoRRSIG;
   }
 
   sortedRecords_t recordcontents;
@@ -2853,7 +2895,17 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname
   }
 
   LOG(d_prefix<<vStateToString(state)<<"!"<<endl);
-  return state;
+  /* try again to get the missed cuts, harder this time */
+  auto zState = getValidationStatus(name, false, type == QType::DS, depth);
+  LOG(d_prefix<<"checking whether we missed a zone cut before returning a Bogus state"<<endl);
+  if (zState == vState::Secure) {
+    /* too bad */
+    LOG(d_prefix<<"we are still in a Secure zone, returning "<<vStateToString(state)<<endl);
+    return state;
+  }
+  else {
+    return zState;
+  }
 }
 
 static bool allowAdditionalEntry(std::unordered_set<DNSName>& allowedAdditionals, const DNSRecord& rec)
@@ -3340,8 +3392,10 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
           /* we have an answer synthesized from a wildcard and aggressive NSEC is enabled, we need to store the
              wildcard in its non-expanded form in the cache to be able to synthesize wildcard answers later */
           const auto& rrsig = i->second.signatures.at(0);
+
           if (isWildcardExpanded(labelCount, rrsig) && !isWildcardExpandedOntoItself(i->first.name, labelCount, rrsig)) {
             DNSName realOwner = getNSECOwnerName(i->first.name, i->second.signatures);
+
             std::vector<DNSRecord> content;
             content.reserve(i->second.records.size());
             for (const auto& record : i->second.records) {
@@ -3373,7 +3427,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
   return RCode::NoError;
 }
 
-void SyncRes::updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState)
+void SyncRes::updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState, bool isDS, unsigned int depth)
 {
   if (denialState == expectedState) {
     neValidationState = vState::Secure;
@@ -3402,8 +3456,16 @@ void SyncRes::updateDenialValidationState(vState& neValidationState, const DNSNa
       neValidationState = vState::Insecure;
     }
     else {
-      LOG(d_prefix<<"Invalid denial found for "<<neName<<", returning Bogus, res="<<denialState<<", expectedState="<<expectedState<<endl);
-      neValidationState = vState::BogusInvalidDenial;
+      LOG(d_prefix<<"Invalid denial found for "<<neName<<", res="<<denialState<<", expectedState="<<expectedState<<", checking whether we have missed a zone cut before returning a Bogus state"<<endl);
+      /* try again to get the missed cuts, harder this time */
+      auto zState = getValidationStatus(neName, false, isDS, depth);
+      if (zState != vState::Secure) {
+        neValidationState = zState;
+      }
+      else {
+        LOG(d_prefix<<"Still in a secure zone with an invalid denial for "<<neName<<", returning "<<vStateToString(vState::BogusInvalidDenial)<<endl);
+        neValidationState = vState::BogusInvalidDenial;
+      }
     }
   }
   updateValidationState(state, neValidationState);
@@ -3471,7 +3533,7 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
         auto recordState = getValidationStatus(rec.d_name, !ne.authoritySOA.signatures.empty() || !ne.DNSSECRecords.signatures.empty(), false, depth);
         if (recordState == vState::Secure) {
           dState denialState = getDenialValidationState(ne, dState::NXDOMAIN, false);
-          updateDenialValidationState(ne.d_validationState, ne.d_name, state, denialState, dState::NXDOMAIN);
+          updateDenialValidationState(ne.d_validationState, ne.d_name, state, denialState, dState::NXDOMAIN, false, depth);
         }
         else {
           ne.d_validationState = recordState;
@@ -3704,7 +3766,7 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
           auto recordState = getValidationStatus(qname, !ne.authoritySOA.signatures.empty() || !ne.DNSSECRecords.signatures.empty(), qtype == QType::DS, depth);
           if (recordState == vState::Secure) {
             dState denialState = getDenialValidationState(ne, dState::NXQTYPE, false);
-            updateDenialValidationState(ne.d_validationState, ne.d_name, state, denialState, dState::NXQTYPE);
+            updateDenialValidationState(ne.d_validationState, ne.d_name, state, denialState, dState::NXQTYPE, qtype == QType::DS, depth);
           } else {
             ne.d_validationState = recordState;
             updateValidationState(state, ne.d_validationState);
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index 9396c315a1..bb34dff87e 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -876,7 +876,7 @@ private:
   vState validateDNSKeys(const DNSName& zone, const std::vector<DNSRecord>& dnskeys, const std::vector<std::shared_ptr<RRSIGRecordContent> >& signatures, unsigned int depth);
   vState getDNSKeys(const DNSName& signer, skeyset_t& keys, unsigned int depth);
   dState getDenialValidationState(const NegCache::NegCacheEntry& ne, const dState expectedState, bool referralToUnsigned);
-  void updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState);
+  void updateDenialValidationState(vState& neValidationState, const DNSName& neName, vState& state, const dState denialState, const dState expectedState, bool isDS, unsigned int depth);
   void computeNegCacheValidationStatus(const NegCache::NegCacheEntry& ne, const DNSName& qname, QType qtype, const int res, vState& state, unsigned int depth);
   vState getTA(const DNSName& zone, dsmap_t& ds);
   vState getValidationStatus(const DNSName& subdomain, bool hasSignatures, bool typeIsDS, unsigned int depth);