From 3996e9fdddaa14e40092f1fdb4caf531c2fe9da8 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Wed, 9 Jan 2019 17:08:38 +0100 Subject: [PATCH] rec: Always check signature for records in ANSWER, even with AA=0 Except for a small exception with chains of CNAMEs. --- pdns/recursordist/test-syncres_cc.cc | 68 ++++++++++++++++++++++++++++ pdns/syncres.cc | 13 ++++-- pdns/syncres.hh | 2 +- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index 0f140bcb27..3a3284dc40 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -4270,6 +4270,74 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_bad_sig) { BOOST_CHECK_EQUAL(queriesCount, 2); } +BOOST_AUTO_TEST_CASE(test_dnssec_bogus_bad_sig_no_aa) { + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::ValidateAll); + + primeHints(); + const DNSName target("."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::RSASHA512, DNSSECKeeper::SHA384, 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, bool* chained) { + queriesCount++; + + if (domain == target && type == QType::NS) { + + /* set AA=0 */ + setLWResult(res, 0, false, false, true); + char addr[] = "a.root-servers.net."; + for (char idx = 'a'; idx <= 'm'; idx++) { + addr[0] = idx; + addRecordToLW(res, domain, QType::NS, std::string(addr), DNSResourceRecord::ANSWER, 3600); + } + + addRRSIG(keys, res->d_records, domain, 300, true); + + addRecordToLW(res, "a.root-servers.net.", QType::A, "198.41.0.4", DNSResourceRecord::ADDITIONAL, 3600); + addRecordToLW(res, "a.root-servers.net.", QType::AAAA, "2001:503:ba3e::2:30", DNSResourceRecord::ADDITIONAL, 3600); + + return 1; + } else if (domain == target && type == QType::DNSKEY) { + + setLWResult(res, 0, true, false, true); + + addDNSKEY(keys, domain, 300, res->d_records); + addRRSIG(keys, res->d_records, domain, 300); + + return 1; + } + + return 0; + }); + + vector ret; + int res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); + /* 13 NS + 1 RRSIG */ + BOOST_REQUIRE_EQUAL(ret.size(), 14); + BOOST_CHECK_EQUAL(queriesCount, 2); + + /* again, to test the cache, but we will trigger a new outgoing query since + the answer did not have the AA bit set */ + ret.clear(); + res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); + BOOST_REQUIRE_EQUAL(ret.size(), 14); + BOOST_CHECK_EQUAL(queriesCount, 3); +} + BOOST_AUTO_TEST_CASE(test_dnssec_bogus_bad_algo) { std::unique_ptr sr; initSR(sr, true); diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 5fa46e17cb..3840e31109 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -1916,8 +1916,9 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname return Bogus; } -RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional ednsmask, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount) +RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional ednsmask, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount, bool rdQuery) { + bool wasForwardRecurse = wasForwarded && rdQuery; tcache_t tcache; string prefix; @@ -2068,7 +2069,8 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr set the TC bit solely because these RRSIG RRs didn't fit." */ bool isAA = lwr.d_aabit && i->first.place != DNSResourceRecord::ADDITIONAL; - if (isAA && isCNAMEAnswer && (i->first.place != DNSResourceRecord::ANSWER || i->first.type != QType::CNAME || i->first.name != qname)) { + bool expectSignature = i->first.place == DNSResourceRecord::ANSWER || ((lwr.d_aabit || wasForwardRecurse) && i->first.place != DNSResourceRecord::ADDITIONAL); + 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 @@ -2080,6 +2082,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); @@ -2088,7 +2091,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr if (shouldValidate() && recordState == Secure) { vState initialState = recordState; - if (isAA) { + if (expectSignature) { if (i->first.place != DNSResourceRecord::ADDITIONAL) { /* the additional entries can be insecure, like glue: @@ -2118,7 +2121,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr } } - if (initialState == Secure && state != recordState && isAA) { + if (initialState == Secure && state != recordState && expectSignature) { updateValidationState(state, recordState); } } @@ -2508,7 +2511,7 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn bool needWildcardProof = false; unsigned int wildcardLabelsCount; - *rcode = updateCacheFromRecords(depth, lwr, qname, qtype, auth, wasForwarded, ednsmask, state, needWildcardProof, wildcardLabelsCount); + *rcode = updateCacheFromRecords(depth, lwr, qname, qtype, auth, wasForwarded, ednsmask, state, needWildcardProof, wildcardLabelsCount, sendRDQuery); if (*rcode != RCode::NoError) { return true; } diff --git a/pdns/syncres.hh b/pdns/syncres.hh index f7de35a786..70ef510bbe 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -750,7 +750,7 @@ private: bool throttledOrBlocked(const std::string& prefix, const ComboAddress& remoteIP, const DNSName& qname, const QType& qtype, bool pierceDontQuery); vector retrieveAddressesForNS(const std::string& prefix, const DNSName& qname, vector::const_iterator& tns, const unsigned int depth, set& beenthere, const vector& rnameservers, NsSet& nameservers, bool& sendRDQuery, bool& pierceDontQuery, bool& flawedNSSet, bool cacheOnly); - RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount); + RCode::rcodes_ updateCacheFromRecords(unsigned int depth, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, const boost::optional, vState& state, bool& needWildcardProof, unsigned int& wildcardLabelsCount, bool sendRDQuery); bool processRecords(const std::string& prefix, const DNSName& qname, const QType& qtype, const DNSName& auth, LWResult& lwr, const bool sendRDQuery, vector& ret, set& nsset, DNSName& newtarget, DNSName& newauth, bool& realreferral, bool& negindic, vState& state, const bool needWildcardProof, const unsigned int wildcardLabelsCount); bool doSpecialNamesResolve(const DNSName &qname, const QType &qtype, const uint16_t qclass, vector &ret); -- 2.47.2