From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 21 Jan 2022 12:15:54 +0000 (+0100)
Subject: rec: Reject non-apex NSEC(3)s that have both the NS and SOA bits set
X-Git-Tag: auth-4.7.0-alpha1~38^2~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be5d851dbedeecdeef39a583f0e2ac50d786b806;p=thirdparty%2Fpdns.git

rec: Reject non-apex NSEC(3)s that have both the NS and SOA bits set

Ancestor NSEC(3)s have the SOA bit clear (delegation), and the remaining
non-apex ones should not have the NS set.
---

diff --git a/pdns/recursordist/test-syncres_cc8.cc b/pdns/recursordist/test-syncres_cc8.cc
index 45341da566..96a7330c59 100644
--- a/pdns/recursordist/test-syncres_cc8.cc
+++ b/pdns/recursordist/test-syncres_cc8.cc
@@ -332,6 +332,46 @@ BOOST_AUTO_TEST_CASE(test_nsec_insecure_delegation_denial)
   BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
 }
 
+BOOST_AUTO_TEST_CASE(test_nsec_insecure_delegation_denial_soa)
+{
+  initSR();
+
+  testkeysset_t keys;
+  generateKeyMaterial(DNSName("."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+
+  vector<DNSRecord> records;
+
+  sortedRecords_t recordContents;
+  vector<shared_ptr<RRSIGRecordContent>> signatureContents;
+
+  /*
+   * RFC 5155 section 8.9:
+   * If there is an NSEC3 RR present in the response that matches the
+   * delegation name, then the validator MUST ensure that the NS bit is
+   * set and that the DS bit is not set in the Type Bit Maps field of the
+   * NSEC3 RR.
+   */
+  /*
+    The RRSIG from "." denies the existence of any type at a except NS and SOA.
+    NS has to be set since it is proving an insecure delegation, but SOA should NOT!
+  */
+  addNSECRecordToLW(DNSName("a."), DNSName("b."), {QType::NS, QType::SOA}, 600, records);
+  recordContents.insert(records.at(0).d_content);
+  addRRSIG(keys, records, DNSName("."), 300);
+  signatureContents.push_back(getRR<RRSIGRecordContent>(records.at(1)));
+  records.clear();
+
+  ContentSigPair pair;
+  pair.records = recordContents;
+  pair.signatures = signatureContents;
+  cspmap_t denialMap;
+  denialMap[std::pair(DNSName("a."), QType::NSEC)] = pair;
+
+  /* Insecure because both NS and SOA are set, so this is not a proper delegation */
+  dState denialState = getDenial(denialMap, DNSName("a."), QType::DS, true, true);
+  BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
+}
+
 BOOST_AUTO_TEST_CASE(test_nsec_nxqtype_cname)
 {
   initSR();
@@ -874,6 +914,46 @@ BOOST_AUTO_TEST_CASE(test_nsec3_insecure_delegation_denial)
   BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
 }
 
+BOOST_AUTO_TEST_CASE(test_nsec3_insecure_delegation_denial_soa)
+{
+  initSR();
+
+  testkeysset_t keys;
+  generateKeyMaterial(DNSName("."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+
+  vector<DNSRecord> records;
+
+  sortedRecords_t recordContents;
+  vector<shared_ptr<RRSIGRecordContent>> signatureContents;
+
+  /*
+   * RFC 5155 section 8.9:
+   * If there is an NSEC3 RR present in the response that matches the
+   * delegation name, then the validator MUST ensure that the NS bit is
+   * set and that the DS bit is not set in the Type Bit Maps field of the
+   * NSEC3 RR.
+   */
+  /*
+    The RRSIG from "." denies the existence of any type at a except NS and SOA.
+    NS has to be set since it is proving an insecure delegation, but SOA should NOT!
+  */
+  addNSEC3UnhashedRecordToLW(DNSName("a."), DNSName("."), "whatever", {QType::NS, QType::SOA}, 600, records);
+  recordContents.insert(records.at(0).d_content);
+  addRRSIG(keys, records, DNSName("."), 300);
+  signatureContents.push_back(getRR<RRSIGRecordContent>(records.at(1)));
+
+  ContentSigPair pair;
+  pair.records = recordContents;
+  pair.signatures = signatureContents;
+  cspmap_t denialMap;
+  denialMap[std::pair(records.at(0).d_name, records.at(0).d_type)] = pair;
+  records.clear();
+
+  /* Insecure because both NS and SOA are set, so it is not a proper delegation */
+  dState denialState = getDenial(denialMap, DNSName("a."), QType::DS, true, true);
+  BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
+}
+
 BOOST_AUTO_TEST_CASE(test_nsec3_ent_opt_out)
 {
   initSR();
diff --git a/pdns/validate.cc b/pdns/validate.cc
index 1417e5ab49..a35e1148c2 100644
--- a/pdns/validate.cc
+++ b/pdns/validate.cc
@@ -512,6 +512,16 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
            continue;
         }
 
+        /* The NSEC is either a delegation one, from the parent zone, and
+         * must have the NS bit set but not the SOA one, or a regular NSEC
+         * either at apex (signer == owner) or with the SOA or NS bits clear.
+         */
+        const bool notApex = signer.countLabels() < owner.countLabels();
+        if (notApex && nsec->isSet(QType::NS) && nsec->isSet(QType::SOA)) {
+          LOG("However, that NSEC is not at the apex and has both the NS and the SOA bits set!"<<endl);
+          continue;
+        }
+
         /* RFC 6840 section 4.1 "Clarifications on Nonexistence Proofs":
            Ancestor delegation NSEC or NSEC3 RRs MUST NOT be used to assume
            nonexistence of any RRs below that zone cut, which include all RRs at
@@ -546,9 +556,11 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
            * attention.  Bits corresponding to the delegation NS RRset and any
            * RRsets for which the parent zone has authoritative data MUST be set
            */
-          if (referralToUnsigned && qtype == QType::DS && !nsec->isSet(QType::NS)) {
-            LOG("However, no NS record exists at this level!"<<endl);
-            return dState::NODENIAL;
+          if (referralToUnsigned && qtype == QType::DS) {
+            if (!nsec->isSet(QType::NS)) {
+              LOG("However, no NS record exists at this level!"<<endl);
+              return dState::NODENIAL;
+            }
           }
 
           /* we know that the name exists (but this qtype doesn't) so except
@@ -641,9 +653,10 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
           continue;
         }
 
+        const DNSName& owner = v.first.first;
         const DNSName signer = getSigner(v.second.signatures);
-        if (!v.first.first.isPartOf(signer)) {
-          LOG("Owner "<<v.first.first<<" is not part of the signer "<<signer<<", ignoring"<<endl);
+        if (!owner.isPartOf(signer)) {
+          LOG("Owner "<<owner<<" is not part of the signer "<<signer<<", ignoring"<<endl);
           continue;
         }
 
@@ -652,6 +665,16 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
           continue;
         }
 
+        /* The NSEC is either a delegation one, from the parent zone, and
+         * must have the NS bit set but not the SOA one, or a regular NSEC
+         * either at apex (signer == owner) or with the SOA or NS bits clear.
+         */
+        const bool notApex = signer.countLabels() < owner.countLabels();
+        if (notApex && nsec3->isSet(QType::NS) && nsec3->isSet(QType::SOA)) {
+          LOG("However, that NSEC3 is not at the apex and has both the NS and the SOA bits set!"<<endl);
+          continue;
+        }
+
         string h = getHashFromNSEC3(qname, nsec3, cache);
         if (h.empty()) {
           LOG("Unsupported hash, ignoring"<<endl);
@@ -661,7 +684,7 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
         nsec3Seen = true;
 
         LOG("\tquery hash: "<<toBase32Hex(h)<<endl);
-        string beginHash=fromBase32Hex(v.first.first.getRawLabels()[0]);
+        string beginHash=fromBase32Hex(owner.getRawLabels()[0]);
 
         // If the name exists, check if the qtype is denied
         if (beginHash == h) {
@@ -672,7 +695,7 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
              that (original) owner name other than DS RRs, and all RRs below that
              owner name regardless of type.
           */
-          if (qtype != QType::DS && isNSEC3AncestorDelegation(signer, v.first.first, nsec3)) {
+          if (qtype != QType::DS && isNSEC3AncestorDelegation(signer, owner, nsec3)) {
             /* this is an "ancestor delegation" NSEC3 RR */
             LOG("An ancestor delegation NSEC3 RR can only deny the existence of a DS"<<endl);
             return dState::NODENIAL;
@@ -692,9 +715,11 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
            * set and that the DS bit is not set in the Type Bit Maps field of the
            * NSEC3 RR.
            */
-          if (referralToUnsigned && qtype == QType::DS && !nsec3->isSet(QType::NS)) {
-            LOG("However, no NS record exists at this level!"<<endl);
-            return dState::NODENIAL;
+          if (referralToUnsigned && qtype == QType::DS) {
+            if (!nsec3->isSet(QType::NS)) {
+              LOG("However, no NS record exists at this level!"<<endl);
+              return dState::NODENIAL;
+            }
           }
 
           return dState::NXQTYPE;