From dff3ef07c6048cea28195f1db138f0fc6d8aae2b Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Tue, 18 Dec 2018 17:17:39 +0100 Subject: [PATCH] rec: Correctly handle the non-AA parts of a RD forwarded CNAME answer --- pdns/recursordist/test-syncres_cc.cc | 138 +++++++++++++++++++++++++++ pdns/syncres.cc | 15 ++- 2 files changed, 148 insertions(+), 5 deletions(-) diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index 22655c1fb..fe00fd7df 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -3095,6 +3095,144 @@ BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_rd) { BOOST_CHECK_EQUAL(ret.size(), 1); } +BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_rd_dnssec) { + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::ValidateAll); + + primeHints(); + /* signed */ + const DNSName target("test."); + /* unsigned */ + const DNSName cnameTarget("cname."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + generateKeyMaterial(target, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys); + g_luaconfs.setState(luaconfsCopy); + + const ComboAddress forwardedNS("192.0.2.42:53"); + size_t queriesCount = 0; + + SyncRes::AuthDomain ad; + ad.d_rdForward = true; + ad.d_servers.push_back(forwardedNS); + (*SyncRes::t_sstorage.domainmap)[g_rootdnsname] = ad; + + sr->setAsyncCallback([target,cnameTarget,keys,forwardedNS,&queriesCount](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++; + + BOOST_CHECK_EQUAL(sendRDQuery, true); + + if (ip != forwardedNS) { + return 0; + } + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, DNSName("."), type, keys); + } + + if (domain == target && type == QType::A) { + + setLWResult(res, 0, false, false, true); + addRecordToLW(res, target, QType::CNAME, cnameTarget.toString()); + addRRSIG(keys, res->d_records, domain, 300); + addRecordToLW(res, cnameTarget, QType::A, "192.0.2.1"); + + return 1; + } + return 0; + }); + + vector ret; + int 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(), 3); + BOOST_CHECK_EQUAL(queriesCount, 5); + + /* 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(), Insecure); + BOOST_REQUIRE_EQUAL(ret.size(), 3); + BOOST_CHECK_EQUAL(queriesCount, 5); +} + +BOOST_AUTO_TEST_CASE(test_forward_zone_recurse_rd_dnssec_bogus) { + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::ValidateAll); + + primeHints(); + /* signed */ + const DNSName target("test."); + /* signed */ + const DNSName cnameTarget("cname."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + generateKeyMaterial(target, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys); + generateKeyMaterial(cnameTarget, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys); + g_luaconfs.setState(luaconfsCopy); + + const ComboAddress forwardedNS("192.0.2.42:53"); + size_t queriesCount = 0; + + SyncRes::AuthDomain ad; + ad.d_rdForward = true; + ad.d_servers.push_back(forwardedNS); + (*SyncRes::t_sstorage.domainmap)[g_rootdnsname] = ad; + + sr->setAsyncCallback([target,cnameTarget,keys,forwardedNS,&queriesCount](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++; + + BOOST_CHECK_EQUAL(sendRDQuery, true); + + if (ip != forwardedNS) { + return 0; + } + + if (type == QType::DS || type == QType::DNSKEY) { + return genericDSAndDNSKEYHandler(res, domain, DNSName("."), type, keys); + } + + if (domain == target && type == QType::A) { + + setLWResult(res, 0, false, false, true); + addRecordToLW(res, target, QType::CNAME, cnameTarget.toString()); + addRRSIG(keys, res->d_records, domain, 300); + addRecordToLW(res, cnameTarget, QType::A, "192.0.2.1"); + /* no RRSIG in a signed zone, Bogus ! */ + + return 1; + } + return 0; + }); + + vector ret; + int 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(), 3); + BOOST_CHECK_EQUAL(queriesCount, 5); + + /* 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(), Bogus); + BOOST_REQUIRE_EQUAL(ret.size(), 3); + BOOST_CHECK_EQUAL(queriesCount, 5); +} + BOOST_AUTO_TEST_CASE(test_auth_zone_oob) { std::unique_ptr sr; initSR(sr, true); diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 7950d54bc..5eff0c398 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -941,7 +941,7 @@ bool SyncRes::doCNAMECacheCheck(const DNSName &qname, const QType &qtype, vector if(j->d_ttl>(unsigned int) d_now.tv_sec) { - if (!wasAuthZone && shouldValidate() && wasAuth && state == Indeterminate && d_requireAuthData) { + if (!wasAuthZone && shouldValidate() && (wasAuth || wasForwardRecurse) && state == Indeterminate && d_requireAuthData) { /* 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 */ @@ -1189,7 +1189,7 @@ bool SyncRes::doCacheCheck(const DNSName &qname, const DNSName& authname, bool w LOG(prefix<first.place != DNSResourceRecord::ADDITIONAL; - if (isAA && isCNAMEAnswer && (i->first.place != DNSResourceRecord::ANSWER || i->first.type != QType::CNAME || i->first.name != qname)) { + /* if we forwarded the query to a recursor, we can expect the answer to be signed, + even if the answer is not AA. Of course that's not only true inside a Secure + zone, but we check that below. */ + bool expectSignature = isAA || wasForwardRecurse; + if (isCNAMEAnswer && (i->first.place != DNSResourceRecord::ANSWER || i->first.type != QType::CNAME || i->first.name != qname)) { /* rfc2181 states: Note that the answer section of an authoritative answer normally @@ -2134,6 +2138,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr associated with the alias. */ isAA = false; + expectSignature = false; } vState recordState = getValidationStatus(i->first.name, false); @@ -2142,7 +2147,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr if (shouldValidate() && recordState == Secure) { vState initialState = recordState; - if (isAA || wasForwardRecurse) { + if (expectSignature) { if (i->first.place != DNSResourceRecord::ADDITIONAL) { /* the additional entries can be insecure, like glue: @@ -2172,7 +2177,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr } } - if (initialState == Secure && state != recordState && (isAA || wasForwardRecurse)) { + if (initialState == Secure && state != recordState && expectSignature) { updateValidationState(state, recordState); } } -- 2.40.0