From 1e60f00d0a81e78021cb5a7491142547544a4164 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 28 Jan 2022 14:26:36 +0100 Subject: [PATCH 1/2] If we get a CNAME when asking for a DS, we should give up and return vState::BogusUnableToGetDSs (cherry picked from commit 271ae639803453a6193b6c3c2baf034446a9965a) --- pdns/syncres.cc | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 92344dd9314b..2868c342675c 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -2860,12 +2860,18 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname state = vState::BogusSelfSignedDS; dsFailed = true; } - else if (qtype == QType::DS && signer == qname && !signer.isRoot() && (type == QType::SOA || type == QType::NSEC || type == QType::NSEC3)) { + else if (qtype == QType::DS && signer == qname && !signer.isRoot()) { + if (type == QType::SOA || type == QType::NSEC || type == QType::NSEC3) { /* if we are trying to validate the DS or more likely NSEC(3)s proving that it does not exist, we have a problem. In that case let's go Bogus (we will check later if we missed a cut) */ - state = vState::BogusSelfSignedDS; - dsFailed = true; + state = vState::BogusSelfSignedDS; + dsFailed = true; + } + else if (type == QType::CNAME) { + state = vState::BogusUnableToGetDSs; + dsFailed = true; + } } else if (qtype == QType::DNSKEY && signer == qname) { /* that actually does happen when a server returns NS records in authority From 7e1ecb3a9a5385f94aa8e6ab5ebb58e0626ebda3 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 28 Jan 2022 14:27:35 +0100 Subject: [PATCH 2/2] Add a test for the case where an (Insecure) domain fails to get a DS record because of a CNAME loop, avoiding a SERVFAIL. (cherry picked from commit c10acee8f241f7fa038fb5904d4bf71124093ac4) --- pdns/recursordist/test-syncres_cc6.cc | 112 ++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/pdns/recursordist/test-syncres_cc6.cc b/pdns/recursordist/test-syncres_cc6.cc index af2d528ca9f0..c2c5d16a1d97 100644 --- a/pdns/recursordist/test-syncres_cc6.cc +++ b/pdns/recursordist/test-syncres_cc6.cc @@ -1767,4 +1767,116 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_broken_without_ds) BOOST_CHECK_EQUAL(queriesCount, 6U); } +BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_broken_cname_ds) +{ + /* Test an Insecure domain that responds with a CNAME on a DS query */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::ValidateAll); + + primeHints(); + const DNSName target("www.sub.powerdns.com."); + const ComboAddress targetAddr("192.0.2.42"); + testkeysset_t keys, pdnskeys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + + // We have two set of keys as powerdns.com and sub.powerdns.com are Insecure but still have RRSIGS + 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, pdnskeys); + generateKeyMaterial(DNSName("sub.powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, pdnskeys); + + g_luaconfs.setState(luaconfsCopy); + + size_t queriesCount = 0; + + sr->setAsyncCallback([target, targetAddr, &queriesCount, keys, pdnskeys](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) { + // Return a signed CNAME on a DS query for powerdns.com and sub.powerdns.com + if (domain == DNSName("powerdns.com.") || domain == DNSName("sub.powerdns.com.")) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, domain, QType::CNAME, "some.name", DNSResourceRecord::ANSWER, 300); + addRRSIG(pdnskeys, res->d_records, domain, 300); + return LWResult::Result::Success; + } + else { + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys); + } + } + else if (type == QType::DNSKEY) { + if (domain == DNSName("powerdns.com.") || domain == DNSName("sub.powerdns.com.")) { + return genericDSAndDNSKEYHandler(res, domain, domain, type, pdnskeys); + } + 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, DNSName("com."), QType::NS, "a.gtld-servers.com."); + addRRSIG(keys, res->d_records, DNSName("com."), 300); + addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600); + } + else { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, DNSName("powerdns.com."), QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600); + addNSECRecordToLW(DNSName("powerdns.com."), DNSName("z.powerdns.com."), {QType::NS, QType::RRSIG}, 600, res->d_records); + 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, false, false, true); + addRecordToLW(res, DNSName("sub.powerdns.com."), QType::NS, "ns1.sub.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600); + addRRSIG(pdnskeys, res->d_records, DNSName("powerdns.com."), 300); + + addRecordToLW(res, "ns1.sub.powerdns.com.", QType::A, "192.0.2.3", DNSResourceRecord::ADDITIONAL, 3600); + return LWResult::Result::Success; + } + else if (ip == ComboAddress("192.0.2.3:53")) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, domain, QType::A, targetAddr.toString(), DNSResourceRecord::ANSWER, 3600); + addRRSIG(pdnskeys, res->d_records, DNSName("sub.powerdns.com."), 300); + 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::Insecure); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::A); + BOOST_CHECK_EQUAL(queriesCount, 8U); + + /* 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::Insecure); + BOOST_REQUIRE_EQUAL(ret.size(), 2U); + BOOST_CHECK(ret[0].d_type == QType::A); + BOOST_CHECK_EQUAL(queriesCount, 8U); +} + BOOST_AUTO_TEST_SUITE_END()