From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 20 Nov 2017 17:12:48 +0000 (+0100)
Subject: rec: Fix DNSSEC validation of DS denial from the negative cache
X-Git-Tag: auth-4.1.0~22^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F5978%2Fhead;p=thirdparty%2Fpdns.git

rec: Fix DNSSEC validation of DS denial from the negative cache

There is two reasons you can get a proper DS denial:
* Secure to insecure cut, and if we are getting a referral with a
DS denial, we know that we have to check that the NS bit is set
as described in section 8.9 of rfc5155
* No zone cut inside a secure zone, and then of course the NS is
not set

When we retrieve the DS denial from the negative cache with a
validation status of Indeterminate, most likely because validation
was not enabled during the query that landed it in the cache, we
don't have enough data to know which case we are looking at, so
let's just skip the NS check.
---

diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index 465bc364c3..c91f95cae2 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -8856,6 +8856,67 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure) {
   BOOST_CHECK_EQUAL(queriesCount, 4);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure_ds) {
+  /*
+    Validation is optional, and the first query does not ask for it,
+    so the answer is negatively cached as Indeterminate.
+    The second query asks for validation, answer should be marked as
+    Secure.
+    The difference with test_dnssec_validation_from_negcache_secure is
+    that have one more level here, so we are going to look for the proof
+    that the DS does not exist for the last level. Since there is no cut,
+    we should accept the fact that the NSEC denies DS and NS both.
+  */
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::Process);
+
+  primeHints();
+  const DNSName target("www.com.");
+  testkeysset_t keys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys);
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([target,&queriesCount,keys](const ComboAddress& ip, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional<Netmask>& srcmask, boost::optional<const ResolveContext&> context, std::shared_ptr<RemoteLogger> outgoingLogger, LWResult* res) {
+      queriesCount++;
+
+      if (type == QType::DS || type == QType::DNSKEY) {
+        if (domain == target) {
+          /* there is no cut */
+          return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false);
+        }
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+      }
+
+      return 0;
+    });
+
+  vector<DNSRecord> ret;
+  /* first query does not require validation */
+  sr->setDNSSECValidationRequested(false);
+  int res = sr->beginResolve(target, QType(QType::DS), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4);
+  BOOST_CHECK_EQUAL(queriesCount, 1);
+
+  ret.clear();
+  /* second one _does_ require validation */
+  sr->setDNSSECValidationRequested(true);
+  res = sr->beginResolve(target, QType(QType::DS), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), Secure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 4);
+  BOOST_CHECK_EQUAL(queriesCount, 4);
+}
+
 BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_insecure) {
   /*
     Validation is optional, and the first query does not ask for it,
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 5c026511af..40c69b7770 100644
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1030,7 +1030,7 @@ void SyncRes::computeNegCacheValidationStatus(NegCache::NegCacheEntry& ne, const
 
   if (state == Secure) {
     dState expectedState = res == RCode::NXDomain ? NXDOMAIN : NXQTYPE;
-    dState denialState = getDenialValidationState(ne, state, expectedState, qtype == QType::DS);
+    dState denialState = getDenialValidationState(ne, state, expectedState, false);
     updateDenialValidationState(ne, state, denialState, expectedState, qtype == QType::DS);
   }
   if (state != Indeterminate) {
diff --git a/pdns/validate.cc b/pdns/validate.cc
index 66b7d13eaf..f7f37e6fa6 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -298,7 +298,7 @@ static bool provesNSEC3NoWildCard(DNSName wildcard, uint16_t const qtype, const
 /*
   This function checks whether the existence of qname|qtype is denied by the NSEC and NSEC3
   in validrrsets.
-  - If `referralToUnsigned` is true and qtype is QType::DS, this functions returns Insecure
+  - If `referralToUnsigned` is true and qtype is QType::DS, this functions returns NODATA
   if a NSEC or NSEC3 proves that the name exists but no NS type exists, as specified in RFC 5155 section 8.9.
   - If `wantsNoDataProof` is set but a NSEC proves that the whole name does not exist, the function will return
   NXQTYPE is the name is proven to be ENT and NXDOMAIN otherwise.