From f4de85a30424a60c006d7413c8526f6901d36846 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 3 Nov 2017 17:20:57 +0100 Subject: [PATCH] rec: Fix incomplete validation of cached entries When an entry retrieved from the cache or the negative cache has not been previously validated because the initial query did not ask for validation, we only validate answers if the current zone state was Secure. This is fine, but we also need to update the state of the current query if the current zone is Insecure or Bogus, even though we don't need to validate the records. --- pdns/recursordist/test-syncres_cc.cc | 394 ++++++++++++++++++++++++++- pdns/syncres.cc | 44 ++- 2 files changed, 423 insertions(+), 15 deletions(-) diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index c29cff5fd..a827d6474 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -399,7 +399,7 @@ static void generateKeyMaterial(const DNSName& name, unsigned int algo, uint8_t dsAnchors[name].insert(keys[name].second); } -static int genericDSAndDNSKEYHandler(LWResult* res, const DNSName& domain, DNSName auth, int type, const testkeysset_t& keys) +static int genericDSAndDNSKEYHandler(LWResult* res, const DNSName& domain, DNSName auth, int type, const testkeysset_t& keys, bool proveCut=true) { if (type == QType::DS) { auth.chopOff(); @@ -418,7 +418,12 @@ static int genericDSAndDNSKEYHandler(LWResult* res, const DNSName& domain, DNSNa /* sign the SOA */ addRRSIG(keys, res->d_records, auth, 300); /* add a NSEC denying the DS */ - addNSECRecordToLW(domain, DNSName("z") + domain, { QType::NS, QType::NSEC }, 600, res->d_records); + std::set types = { QType::NSEC }; + if (proveCut) { + types.insert(QType::NS); + } + + addNSECRecordToLW(domain, DNSName("z") + domain, types, 600, res->d_records); addRRSIG(keys, res->d_records, auth, 300); } } @@ -8076,6 +8081,391 @@ BOOST_AUTO_TEST_CASE(test_dnssec_rrsig_cache_validity) { BOOST_CHECK_EQUAL(queriesCount, 4); } +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cache_secure) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is cached as Indeterminate. + The second query asks for validation, answer should be marked as + Secure. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + 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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); + } + else { + if (domain == target && type == QType::A) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.1"); + addRRSIG(keys, res->d_records, DNSName("."), 300); + return 1; + } + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 2); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 1); + + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Secure); + BOOST_REQUIRE_EQUAL(ret.size(), 2); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 3); +} + +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cache_insecure) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is cached as Indeterminate. + The second query asks for validation, answer should be marked as + Insecure. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + 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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); + } + else { + if (domain == target && type == QType::A) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.1"); + return 1; + } + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 1); + + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 1); +} + +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cache_bogus) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is cached as Indeterminate. + The second query asks for validation, answer should be marked as + Bogus. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + 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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); + } + else { + if (domain == target && type == QType::A) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.1"); + /* no RRSIG */ + return 1; + } + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 1); + + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::RRSIG); + } + BOOST_CHECK_EQUAL(queriesCount, 3); +} + +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is negatively cached as Indeterminate. + The second query asks for validation, answer should be marked as + Secure. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + DNSName auth = domain; + auth.chopOff(); + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, auth, type, keys); + } + else { + setLWResult(res, RCode::NoError, true, false, true); + 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, domain, 300); + addNSECRecordToLW(domain, DNSName("z."), { QType::NSEC, QType::RRSIG }, 600, res->d_records); + addRRSIG(keys, res->d_records, domain, 1); + return 1; + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 4); + BOOST_CHECK_EQUAL(queriesCount, 1); + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Secure); + BOOST_REQUIRE_EQUAL(ret.size(), 4); + BOOST_CHECK_EQUAL(queriesCount, 4); +} + +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_insecure) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is negatively cached as Indeterminate. + The second query asks for validation, answer should be marked as + Insecure. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + 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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + DNSName auth = domain; + auth.chopOff(); + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, auth, type, keys); + } + else { + setLWResult(res, RCode::NoError, true, false, true); + addRecordToLW(res, domain, QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600); + return 1; + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + BOOST_CHECK_EQUAL(queriesCount, 1); + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Insecure); + BOOST_REQUIRE_EQUAL(ret.size(), 1); + BOOST_CHECK_EQUAL(queriesCount, 1); +} + +BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_bogus) { + /* + Validation is optional, and the first query does not ask for it, + so the answer is negatively cached as Indeterminate. + The second query asks for validation, answer should be marked as + Bogus. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::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, std::shared_ptr outgoingLogger, LWResult* res) { + queriesCount++; + + DNSName auth = domain; + auth.chopOff(); + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, auth, type, keys); + } + else { + setLWResult(res, RCode::NoError, true, false, true); + 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, domain, 300); + /* no denial */ + return 1; + } + + return 0; + }); + + vector ret; + /* first query does not require validation */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_REQUIRE_EQUAL(ret.size(), 2); + BOOST_CHECK_EQUAL(queriesCount, 1); + + ret.clear(); + /* second one _does_ require validation */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); + BOOST_REQUIRE_EQUAL(ret.size(), 2); + BOOST_CHECK_EQUAL(queriesCount, 4); +} + BOOST_AUTO_TEST_CASE(test_lowercase_outgoing) { g_lowercaseOutgoing = true; std::unique_ptr sr; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 504aa61ce..b946fb15d 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -874,7 +874,12 @@ bool SyncRes::doCNAMECacheCheck(const DNSName &qname, const QType &qtype, vector /* This means we couldn't figure out the state when this entry was cached, most likely because we hadn't computed the zone cuts yet. */ /* make sure they are computed before validating */ - computeZoneCuts(qname, g_rootdnsname, depth); + DNSName subdomain(qname); + /* if we are retrieving a DS, we only care about the state of the parent zone */ + if(qtype == QType::DS) + subdomain.chopOff(); + + computeZoneCuts(subdomain, g_rootdnsname, depth); vState recordState = getValidationStatus(qname, false); if (recordState == Secure) { @@ -979,7 +984,12 @@ static void addTTLModifiedRecords(const vector& records, const uint32 void SyncRes::computeNegCacheValidationStatus(NegCache::NegCacheEntry& ne, const DNSName& qname, const QType& qtype, const int res, vState& state, unsigned int depth) { - computeZoneCuts(qname, g_rootdnsname, depth); + DNSName subdomain(qname); + /* if we are retrieving a DS, we only care about the state of the parent zone */ + if(qtype == QType::DS) + subdomain.chopOff(); + + computeZoneCuts(subdomain, g_rootdnsname, depth); tcache_t tcache; reapRecordsFromNegCacheEntryForValidation(tcache, ne.authoritySOA.records); @@ -1001,13 +1011,13 @@ void SyncRes::computeNegCacheValidationStatus(NegCache::NegCacheEntry& ne, const } if (recordState == Secure) { - vState cachedState = SyncRes::validateRecordsWithSigs(depth, qname, qtype, owner, entry.second.records, entry.second.signatures); + recordState = SyncRes::validateRecordsWithSigs(depth, qname, qtype, owner, entry.second.records, entry.second.signatures); + } - if (state == Secure && cachedState != recordState) { - updateValidationState(state, cachedState); - if (state != Secure) { - break; - } + if (recordState != Indeterminate && recordState != state) { + updateValidationState(state, recordState); + if (state != Secure) { + break; } } } @@ -1122,17 +1132,25 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const QType &qtype, vectorupdateValidationStatus(d_now.tv_sec, sqname, sqt, d_incomingECSFound ? d_incomingECSNetwork : d_requestor, d_requireAuthData, cachedState); - } + if (cachedState != Indeterminate) { + LOG(prefix<updateValidationStatus(d_now.tv_sec, sqname, sqt, d_incomingECSFound ? d_incomingECSNetwork : d_requestor, d_requireAuthData, cachedState); } } -- 2.40.0