Skip to content

Commit

Permalink
rec: Be more strict when validating DS wrt parent/child NSEC(3)s
Browse files Browse the repository at this point in the history 
The existing code would not have accepted that proof anyway since
the NSEC(3) are signed by a DNSKEY that would require fetching the
corresponding DS, which causes a loop that would be detected, but
it is cleaner to actually check this.
  • Loading branch information
@rgacogne
rgacogne committed Jul 20, 2021
1 parent 6682b9c commit 0a9dcd1
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 5 deletions.
4 changes: 2 additions & 2 deletions pdns/recursordist/test-syncres_cc6.cc
View file
Expand Up @@ -1036,7 +1036,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_optout)
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("com."), 300);
/* closest encloser */
addNSEC3UnhashedRecordToLW(DNSName("com."), DNSName("com."), DNSName("a.com.").toStringNoDot(), {QType::NS}, 600, res->d_records, 10, true);
addNSEC3UnhashedRecordToLW(DNSName("com."), DNSName("com."), DNSName("a.com.").toStringNoDot(), {QType::NS,QType::SOA}, 600, res->d_records, 10, true);
addRRSIG(keys, res->d_records, DNSName("com."), 300);
/* next closer */
addNSEC3UnhashedRecordToLW(DNSName("oowerdns.com."), DNSName("com."), DNSName("qowerdns.com.").toStringNoDot(), {QType::NS}, 600, res->d_records, 10, true);
Expand Down Expand Up @@ -1081,7 +1081,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_optout)
addRecordToLW(res, domain, QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600);
/* no DS */
/* closest encloser */
addNSEC3UnhashedRecordToLW(DNSName("com."), DNSName("com."), DNSName("a.com.").toStringNoDot(), {QType::NS}, 600, res->d_records, 10, true);
addNSEC3UnhashedRecordToLW(DNSName("com."), DNSName("com."), DNSName("a.com.").toStringNoDot(), {QType::NS,QType::SOA}, 600, res->d_records, 10, true);
addRRSIG(keys, res->d_records, DNSName("com."), 300);
/* next closer */
addNSEC3UnhashedRecordToLW(DNSName("oowerdns.com."), DNSName("com."), DNSName("qowerdns.com.").toStringNoDot(), {QType::NS}, 600, res->d_records, 10, true);
Expand Down
71 changes: 69 additions & 2 deletions pdns/recursordist/test-syncres_cc8.cc
View file
Expand Up @@ -223,7 +223,8 @@ BOOST_AUTO_TEST_CASE(test_nsec_ancestor_nxqtype_denial)
The RRSIG from "." denies the existence of any type except NS at a.
However since it's an ancestor delegation NSEC (NS bit set, SOA bit clear,
signer field that is shorter than the owner name of the NSEC RR) it can't
be used to deny anything except the whole name or a DS.
be used to deny anything except the whole name (which does not make sense here)
or a DS.
*/
addNSECRecordToLW(DNSName("a."), DNSName("b."), {QType::NS}, 600, records);
recordContents.insert(records.at(0).d_content);
Expand All @@ -244,7 +245,7 @@ BOOST_AUTO_TEST_CASE(test_nsec_ancestor_nxqtype_denial)
owner name regardless of type.
*/

dState denialState = getDenial(denialMap, DNSName("a."), QType::A, false, false);
dState denialState = getDenial(denialMap, DNSName("a."), QType::A, false, true);
/* no data means the qname/qtype is not denied, because an ancestor
delegation NSEC can only deny the DS */
BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
Expand All @@ -257,6 +258,38 @@ BOOST_AUTO_TEST_CASE(test_nsec_ancestor_nxqtype_denial)
BOOST_CHECK_EQUAL(denialState, dState::NXQTYPE);
}

BOOST_AUTO_TEST_CASE(test_nsec_ds_denial_from_child)
{
initSR();

testkeysset_t keys;
generateKeyMaterial(DNSName("org."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
generateKeyMaterial(DNSName("example.org."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);

vector<DNSRecord> records;

sortedRecords_t recordContents;
vector<shared_ptr<RRSIGRecordContent>> signatureContents;

addNSECRecordToLW(DNSName("example.org."), DNSName("a.example.org"), {QType::A, QType::TXT, QType::RRSIG, QType::NSEC}, 600, records);
recordContents.insert(records.at(0).d_content);
addRRSIG(keys, records, DNSName("example.org."), 300);
signatureContents.push_back(getRR<RRSIGRecordContent>(records.at(1)));
records.clear();

ContentSigPair pair;
pair.records = recordContents;
pair.signatures = signatureContents;
cspmap_t denialMap;
denialMap[std::make_pair(DNSName("example.org."), QType::NSEC)] = pair;

/* check that this NSEC from the child zone can deny a AAAA at the apex */
BOOST_CHECK_EQUAL(getDenial(denialMap, DNSName("example.org."), QType::AAAA, false, true, true), dState::NXQTYPE);

/* but not that the DS does not exist, since we need the parent for that */
BOOST_CHECK_EQUAL(getDenial(denialMap, DNSName("example.org."), QType::DS, false, true, true), dState::NODENIAL);
}

BOOST_AUTO_TEST_CASE(test_nsec_insecure_delegation_denial)
{
initSR();
Expand Down Expand Up @@ -328,6 +361,36 @@ BOOST_AUTO_TEST_CASE(test_nsec_nxqtype_cname)
BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
}

BOOST_AUTO_TEST_CASE(test_nsec3_nxqtype_ds)
{
initSR();

testkeysset_t keys;
generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);

vector<DNSRecord> records;

sortedRecords_t recordContents;
vector<shared_ptr<RRSIGRecordContent>> signatureContents;

addNSEC3UnhashedRecordToLW(DNSName("powerdns.com."), DNSName("powerdns.com."), "whatever", {QType::A}, 600, records);
recordContents.insert(records.at(0).d_content);
addRRSIG(keys, records, DNSName("powerdns.com."), 300);
signatureContents.push_back(getRR<RRSIGRecordContent>(records.at(1)));

ContentSigPair pair;
pair.records = recordContents;
pair.signatures = signatureContents;
cspmap_t denialMap;
denialMap[std::make_pair(records.at(0).d_name, records.at(0).d_type)] = pair;
records.clear();

/* this NSEC3 is not valid to deny the DS since it is from the child zone */
BOOST_CHECK_EQUAL(getDenial(denialMap, DNSName("powerdns.com."), QType::DS, false, true), dState::NODENIAL);
/* AAAA should be fine, though */
BOOST_CHECK_EQUAL(getDenial(denialMap, DNSName("powerdns.com."), QType::AAAA, false, true), dState::NXQTYPE);
}

BOOST_AUTO_TEST_CASE(test_nsec3_nxqtype_cname)
{
initSR();
Expand Down Expand Up @@ -733,6 +796,10 @@ BOOST_AUTO_TEST_CASE(test_nsec3_ancestor_nxqtype_denial)

denialState = getDenial(denialMap, DNSName("sub.a."), QType::A, false, true);
BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);

/* not even the DS! */
denialState = getDenial(denialMap, DNSName("sub.a."), QType::DS, false, true);
BOOST_CHECK_EQUAL(denialState, dState::NODENIAL);
}

BOOST_AUTO_TEST_CASE(test_nsec3_denial_too_many_iterations)
Expand Down
13 changes: 12 additions & 1 deletion pdns/validate.cc
View file
Expand Up @@ -521,6 +521,11 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
}
}

if (qtype == QType::DS && !qname.isRoot() && signer == qname) {
LOG("A NSEC RR from the child zone cannot deny the existence of a DS"<<endl);
continue;
}

/* check if the type is denied */
if (qname == owner) {
if (!isTypeDenied(nsec, QType(qtype))) {
Expand Down Expand Up @@ -637,6 +642,11 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16
continue;
}

if (qtype == QType::DS && !qname.isRoot() && signer == qname) {
LOG("A NSEC3 RR from the child zone cannot deny the existence of a DS"<<endl);
continue;
}

string h = getHashFromNSEC3(qname, nsec3, cache);
if (h.empty()) {
LOG("Unsupported hash, ignoring"<<endl);
Expand Down Expand Up @@ -729,7 +739,8 @@ dState getDenial(const cspmap_t &validrrsets, const DNSName& qname, const uint16

LOG("Comparing "<<toBase32Hex(h)<<" ("<<closestEncloser<<") against "<<toBase32Hex(beginHash)<<endl);
if (beginHash == h) {
if (qtype != QType::DS && isNSEC3AncestorDelegation(signer, v.first.first, nsec3)) {
/* If the closest encloser is a delegation NS we know nothing about the names in the child zone. */
if (isNSEC3AncestorDelegation(signer, v.first.first, nsec3)) {
LOG("An ancestor delegation NSEC3 RR can only deny the existence of a DS"<<endl);
continue;
}
Expand Down

0 comments on commit 0a9dcd1

Please sign in to comment.