rec: Better detection of Bogus zone cuts for DNSSEC validation

diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc
index 37c63940e24d2877b32267ef0c4af446b4b2fee4..2d094bce5f69c8b12d48e22a34aaea1af44b0d08 100644 (file)
--- a/pdns/recursordist/test-syncres_cc1.cc
+++ b/pdns/recursordist/test-syncres_cc1.cc
@@ -1363,7 +1363,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) {
         }
         if (domain.countLabels() == 1 && type == QType::DS) { // powerdns|DS or example|DS
           setLWResult(res, 0, true, false, true);
-          addDS(domain, 300, res->d_records, keys);
+          addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
           addRRSIG(keys, res->d_records, DNSName("."), 300);
           return 1;
         }
@@ -1392,7 +1392,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) {
           return 1;
         }
         if (domain == target && type == QType::DS) { // dname.powerdns|DS
-          return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys);
+          return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false);
         }
         if (domain == target) {
           setLWResult(res, 0, true, false, false);
@@ -1408,8 +1408,8 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) {
           addRRSIG(keys, res->d_records, domain, 300);
           return 1;
         }
-        if (domain == target && type == QType::DS) { // dname.example|DS
-          return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys);
+        if (domain == cnameTarget && type == QType::DS) { // dname.example|DS
+          return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false);
         }
         if (domain == cnameTarget) {
           setLWResult(res, 0, true, false, false);
@@ -1512,7 +1512,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) {
         }
         if (domain == dnameOwner && type == QType::DS) { // powerdns|DS
           setLWResult(res, 0, true, false, true);
-          addDS(domain, 300, res->d_records, keys);
+          addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
           addRRSIG(keys, res->d_records, DNSName("."), 300);
           return 1;
         }
@@ -1544,7 +1544,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) {
           return 1;
         }
         if (domain == target && type == QType::DS) { // dname.powerdns|DS
-          return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys);
+          return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys, false);
         }
         if (domain == target) {
           setLWResult(res, 0, true, false, false);
@@ -1555,7 +1555,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) {
         }
       } else if (ip == ComboAddress("192.0.2.2:53")) {
         if (domain == target && type == QType::DS) { // dname.example|DS
-          return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys);
+          return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys, false);
         }
         if (domain == cnameTarget) {
           setLWResult(res, 0, true, false, false);

diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc
index a515f936250e5e5aea7a895a482f731af5f68446..aceff1ff1f8a86b0bf72ba929e7c717e3b8aee44 100644 (file)
--- a/pdns/recursordist/test-syncres_cc4.cc
+++ b/pdns/recursordist/test-syncres_cc4.cc
@@ -1202,7 +1202,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_CHECK_EQUAL(queriesCount, 3);
 
   /* again, to test the cache */
   ret.clear();
@@ -1210,7 +1210,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_CHECK_EQUAL(queriesCount, 3);
 
   /* now we ask directly for the DS */
   ret.clear();
@@ -1218,7 +1218,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 1);
-  BOOST_CHECK_EQUAL(queriesCount, 4);
+  BOOST_CHECK_EQUAL(queriesCount, 3);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds_direct) {

diff --git a/pdns/recursordist/test-syncres_cc6.cc b/pdns/recursordist/test-syncres_cc6.cc
index 7bb466afbbb72fce3d3aa4b51666c7938bdcddcf..5b0a1e3f3215621add7b186be7239979fb5b7bdb 100644 (file)
--- a/pdns/recursordist/test-syncres_cc6.cc
+++ b/pdns/recursordist/test-syncres_cc6.cc
@@ -34,6 +34,13 @@ BOOST_AUTO_TEST_CASE(test_dnssec_no_ds_on_referral_secure) {
         auth.chopOff();
         dsQueriesCount++;
 
+        if (domain == target) {
+          if (genericDSAndDNSKEYHandler(res, domain, auth, type, keys, false) == 0) {
+            return 0;
+          }
+          return 1;
+        }
+
         setLWResult(res, 0, true, false, true);
         addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
         addRRSIG(keys, res->d_records, auth, 300);
@@ -145,8 +152,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) {
 
         setLWResult(res, 0, true, false, true);
         if (domain == target) {
-          addRecordToLW(res, domain, QType::SOA, "ns1.powerdns.com. blah. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400);
-          addRRSIG(keys, res->d_records, target, 300);
+          addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
+          addRRSIG(keys, res->d_records, domain, 300);
         }
         else {
           addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
@@ -218,7 +225,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 8);
 
   /* again, to test the cache */
   ret.clear();
@@ -226,7 +233,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 8);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) {
@@ -260,14 +267,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) {
         auth.chopOff();
 
         setLWResult(res, 0, true, false, true);
-        if (domain == target) {
-          addRecordToLW(res, domain, QType::SOA, "ns1.powerdns.com. blah. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400);
-          addRRSIG(keys, res->d_records, target, 300);
-        }
-        else {
-          addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
-          addRRSIG(keys, res->d_records, auth, 300);
-        }
+        addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER);
+        addRRSIG(keys, res->d_records, auth, 300);
         return 1;
       }
       else if (type == QType::DNSKEY) {
@@ -333,7 +334,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 10);
 
   /* again, to test the cache */
   ret.clear();
@@ -341,7 +342,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) {
   BOOST_CHECK_EQUAL(res, RCode::NoError);
   BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
-  BOOST_CHECK_EQUAL(queriesCount, 9);
+  BOOST_CHECK_EQUAL(queriesCount, 10);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_no_ds_on_referral_insecure) {

diff --git a/pdns/recursordist/test-syncres_cc7.cc b/pdns/recursordist/test-syncres_cc7.cc
index 59550b9280e987f6b297fb4fe03db8f50aa3034a..ae16ec444bf8e777bfcd291da61c0342bec2a126 100644 (file)
--- a/pdns/recursordist/test-syncres_cc7.cc
+++ b/pdns/recursordist/test-syncres_cc7.cc
@@ -43,6 +43,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) {
 
           if (domain == DNSName("com.")) {
             addRecordToLW(res, domain, QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600);
+            addRRSIG(keys, res->d_records, DNSName("."), 300);
             /* no DS */
             addNSECRecordToLW(DNSName("com."), DNSName("dom."), { QType::NS }, 600, res->d_records);
             addRRSIG(keys, res->d_records, DNSName("."), 300);
@@ -119,7 +120,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Secure);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  BOOST_CHECK_EQUAL(queriesCount, 8);
+  BOOST_CHECK_EQUAL(queriesCount, 7);
 
   /* again, to test the cache */
   ret.clear();
@@ -128,7 +129,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) {
   BOOST_CHECK_EQUAL(sr->getValidationState(), Secure);
   BOOST_REQUIRE_EQUAL(ret.size(), 2);
   BOOST_CHECK(ret[0].d_type == QType::A);
-  BOOST_CHECK_EQUAL(queriesCount, 8);
+  BOOST_CHECK_EQUAL(queriesCount, 7);
 }
 
 BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_nodata) {

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 2dd483253c9060875ac93c8fa04bd148e14b2926..fa73da9945682f1d3158dfbec25b7033dfc85b97 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -1747,58 +1747,63 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi
   d_skipCNAMECheck = oldSkipCNAME;
 
   if (rcode == RCode::NoError || (rcode == RCode::NXDomain && !bogusOnNXD)) {
-
     uint8_t bestDigestType = 0;
 
-    if (state == Secure) {
-      bool gotCNAME = false;
-      for (const auto& record : dsrecords) {
-        if (record.d_type == QType::DS) {
-          const auto dscontent = getRR<DSRecordContent>(record);
-          if (dscontent && isSupportedDS(*dscontent)) {
-            // Make GOST a lower prio than SHA256
-            if (dscontent->d_digesttype == DNSSECKeeper::GOST && bestDigestType == DNSSECKeeper::SHA256) {
-              continue;
-            }
-            if (dscontent->d_digesttype > bestDigestType || (bestDigestType == DNSSECKeeper::GOST && dscontent->d_digesttype == DNSSECKeeper::SHA256)) {
-              bestDigestType = dscontent->d_digesttype;
-            }
-            ds.insert(*dscontent);
+    bool gotCNAME = false;
+    for (const auto& record : dsrecords) {
+      if (record.d_type == QType::DS) {
+        const auto dscontent = getRR<DSRecordContent>(record);
+        if (dscontent && isSupportedDS(*dscontent)) {
+          // Make GOST a lower prio than SHA256
+          if (dscontent->d_digesttype == DNSSECKeeper::GOST && bestDigestType == DNSSECKeeper::SHA256) {
+            continue;
           }
+          if (dscontent->d_digesttype > bestDigestType || (bestDigestType == DNSSECKeeper::GOST && dscontent->d_digesttype == DNSSECKeeper::SHA256)) {
+            bestDigestType = dscontent->d_digesttype;
+          }
+          ds.insert(*dscontent);
         }
-        else if (record.d_type == QType::CNAME && record.d_name == zone) {
-          gotCNAME = true;
-        }
       }
+      else if (record.d_type == QType::CNAME && record.d_name == zone) {
+        gotCNAME = true;
+      }
+    }
 
-      /* RFC 4509 section 3: "Validator implementations SHOULD ignore DS RRs containing SHA-1
-       * digests if DS RRs with SHA-256 digests are present in the DS RRset."
-       * As SHA348 is specified as well, the spirit of the this line is "use the best algorithm".
-       */
-      for (auto dsrec = ds.begin(); dsrec != ds.end(); ) {
-        if (dsrec->d_digesttype != bestDigestType) {
-          dsrec = ds.erase(dsrec);
-        }
-        else {
-          ++dsrec;
-        }
+    /* RFC 4509 section 3: "Validator implementations SHOULD ignore DS RRs containing SHA-1
+     * digests if DS RRs with SHA-256 digests are present in the DS RRset."
+     * As SHA348 is specified as well, the spirit of the this line is "use the best algorithm".
+     */
+    for (auto dsrec = ds.begin(); dsrec != ds.end(); ) {
+      if (dsrec->d_digesttype != bestDigestType) {
+        dsrec = ds.erase(dsrec);
+      }
+      else {
+        ++dsrec;
       }
+    }
 
-      if (rcode == RCode::NoError && ds.empty()) {
-        if (foundCut) {
-          if (gotCNAME || denialProvesNoDelegation(zone, dsrecords)) {
-            /* we are still inside the same Secure zone */
+    if (rcode == RCode::NoError) {
+      if (ds.empty()) {
+        if (gotCNAME || denialProvesNoDelegation(zone, dsrecords)) {
+          /* we are still inside the same zone */
 
+          if (foundCut) {
             *foundCut = false;
-            return Secure;
           }
+          return state;
+        }
 
+        /* delegation with no DS, might be Secure -> Insecure */
+        if (foundCut) {
           *foundCut = true;
         }
 
-        return Insecure;
-      } else if (foundCut && rcode == RCode::NoError && !ds.empty()) {
-        *foundCut = true;
+        return state == Secure ? Insecure : state;
+      } else {
+        /* we have a DS */
+        if (foundCut) {
+          *foundCut = true;
+        }
       }
     }
 
@@ -2033,6 +2038,11 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname
     if (!signer.empty() && name.isPartOf(signer)) {
       if ((qtype == QType::DNSKEY || qtype == QType::DS) && signer == qname) {
         /* we are already retrieving those keys, sorry */
+        if (qtype == QType::DS) {
+          /* something is very wrong */
+          LOG(d_prefix<<"The DS for "<<qname<<" is signed by itself, going Bogus"<<endl);
+          return Bogus;
+        }
         return Indeterminate;
       }
       vState state = getDNSKeys(signer, keys, depth);