From 0626e855ad5f944f8b357d729dc42e001f57820a Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Tue, 1 Dec 2020 17:20:22 +0100 Subject: [PATCH] rec: Fix the gathering of denial proof for wildcard-expanded answers If somehow the RRSIG indicating that the answer is expanded from a wildcard (label count smaller than the number of labels in the name) went _after_ the NSEC we need, we forgot to gather that NSEC. It might have been an issue for downstream validation (we do gather them a second time later for our own validation) since the client would not have received them. --- pdns/recursordist/test-syncres_cc5.cc | 118 ++++++++++++++++++++++++++ pdns/syncres.cc | 37 ++++---- 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/pdns/recursordist/test-syncres_cc5.cc b/pdns/recursordist/test-syncres_cc5.cc index de48487d39..b267b6b21e 100644 --- a/pdns/recursordist/test-syncres_cc5.cc +++ b/pdns/recursordist/test-syncres_cc5.cc @@ -817,6 +817,124 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_wildcard) BOOST_CHECK_EQUAL(queriesCount, 9U); } +BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_wildcard_proof_before_rrsig) +{ + /* this tests makes sure that we correctly detect that we need to gather + wildcard proof (since the answer is expanded from a wildcard, we need + to prove that the target name does not exist) even though the RRSIG which + allows us to detect that the answer is an expanded wildcard (from the label + count field of the RRSIG) comes _after_ the NSEC + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::ValidateAll); + + primeHints(); + const DNSName target("www.powerdns.com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors); + generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys); + generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_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& srcmask, boost::optional context, LWResult* res, bool* chained) { + queriesCount++; + + if (type == QType::DS || type == QType::DNSKEY) { + if (type == QType::DS && domain == target) { + setLWResult(res, RCode::NoError, true, false, true); + addRecordToLW(res, DNSName("powerdns.com."), 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("powerdns.com."), 300); + addNSECRecordToLW(DNSName("www.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300); + return LWResult::Result::Success; + } + else { + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys); + } + } + else { + if (isRootServer(ip)) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.com.", DNSResourceRecord::AUTHORITY, 3600); + addDS(DNSName("com."), 300, res->d_records, keys); + addRRSIG(keys, res->d_records, DNSName("."), 300); + addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.1:53")) { + if (domain == DNSName("com.")) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, domain, QType::NS, "a.gtld-servers.com."); + addRRSIG(keys, res->d_records, domain, 300); + addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + addRRSIG(keys, res->d_records, domain, 300); + } + else { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, "powerdns.com.", QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600); + addDS(DNSName("powerdns.com."), 300, res->d_records, keys); + addRRSIG(keys, res->d_records, DNSName("com."), 300); + addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600); + } + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.2:53")) { + setLWResult(res, 0, true, false, true); + if (type == QType::NS) { + if (domain == DNSName("powerdns.com.")) { + addRecordToLW(res, domain, QType::NS, "ns1.powerdns.com."); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300); + addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300); + } + else { + 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("powerdns.com"), 300); + addNSECRecordToLW(DNSName("www.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300); + } + } + else { + addRecordToLW(res, domain, QType::A, "192.0.2.42"); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300, false, boost::none, DNSName("*.powerdns.com")); + /* we need to add the proof that this name does not exist, so the wildcard may apply */ + addNSECRecordToLW(DNSName("a.powerdns.com."), DNSName("wwz.powerdns.com."), {QType::A, QType::NSEC, QType::RRSIG}, 600, res->d_records); + addRRSIG(keys, res->d_records, DNSName("powerdns.com"), 300); + /* now this is the important part! We are swapping the first RRSIG and the NSEC, to make sure we still gather the NSEC proof that the + exact name does not exist even though we have not seen the RRSIG whose label count is smaller than the target name yet */ + std::swap(res->d_records.at(1), res->d_records.at(3)); + } + return LWResult::Result::Success; + } + } + + return LWResult::Result::Timeout; + }); + + vector ret; + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure); + BOOST_REQUIRE_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(queriesCount, 9U); + + /* again, to test the cache */ + ret.clear(); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure); + BOOST_REQUIRE_EQUAL(ret.size(), 4U); + BOOST_CHECK_EQUAL(queriesCount, 9U); +} + BOOST_AUTO_TEST_CASE(test_dnssec_validation_nsec_nodata_nowildcard) { std::unique_ptr sr; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 92f674e4de..b00d15a928 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -2908,20 +2908,6 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr isCNAMEAnswer = false; } - /* if we have a positive answer synthesized from a wildcard, - we need to store the corresponding NSEC/NSEC3 records proving - that the exact name did not exist in the negative cache */ - if(gatherWildcardProof) { - if (nsecTypes.count(rec.d_type)) { - authorityRecs.push_back(std::make_shared(rec)); - } - else if (rec.d_type == QType::RRSIG) { - auto rrsig = getRR(rec); - if (rrsig && nsecTypes.count(rrsig->d_type)) { - authorityRecs.push_back(std::make_shared(rec)); - } - } - } if (rec.d_type == QType::RRSIG) { auto rrsig = getRR(rec); if (rrsig) { @@ -2953,8 +2939,29 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr } } + /* if we have a positive answer synthesized from a wildcard, + we need to store the corresponding NSEC/NSEC3 records proving + that the exact name did not exist in the negative cache */ + if (gatherWildcardProof) { + for (const auto& rec : lwr.d_records) { + if (rec.d_type == QType::OPT || rec.d_class != QClass::IN) { + continue; + } + + if (nsecTypes.count(rec.d_type)) { + authorityRecs.push_back(std::make_shared(rec)); + } + else if (rec.d_type == QType::RRSIG) { + auto rrsig = getRR(rec); + if (rrsig && nsecTypes.count(rrsig->d_type)) { + authorityRecs.push_back(std::make_shared(rec)); + } + } + } + } + // reap all answers from this packet that are acceptable - for(auto& rec : lwr.d_records) { + for (auto& rec : lwr.d_records) { if(rec.d_type == QType::OPT) { LOG(prefix<