From 4b3aa4d8b301538ae102b4f816c03a1f6a7307ea Mon Sep 17 00:00:00 2001 From: Pieter Lexis Date: Mon, 5 Jul 2021 14:19:12 +0200 Subject: [PATCH] backport 10555 to 4.4 --- pdns/recursordist/test-syncres_cc3.cc | 10 ++-- pdns/recursordist/test-syncres_cc4.cc | 76 +++++++++++++++++++++++++++ pdns/syncres.cc | 41 +++++++++++++++ pdns/syncres.hh | 3 ++ 4 files changed, 125 insertions(+), 5 deletions(-) diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc index 4fbc6d4aec..7512194b9f 100644 --- a/pdns/recursordist/test-syncres_cc3.cc +++ b/pdns/recursordist/test-syncres_cc3.cc @@ -106,8 +106,8 @@ BOOST_AUTO_TEST_CASE(test_unauth_any) vector ret; int res = sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret); - BOOST_CHECK_EQUAL(res, RCode::ServFail); - BOOST_CHECK_EQUAL(ret.size(), 0U); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 1U); } static void test_no_data_f(bool qmin) @@ -310,14 +310,14 @@ BOOST_AUTO_TEST_CASE(test_answer_no_aa) vector ret; int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); - BOOST_CHECK_EQUAL(res, RCode::ServFail); - BOOST_CHECK_EQUAL(ret.size(), 0U); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 1U); /* check that the record in the answer section has not been cached */ const ComboAddress who; vector cached; vector> signatures; - BOOST_REQUIRE_EQUAL(s_RC->get(now, target, QType(QType::A), false, &cached, who, boost::none, &signatures), -1); + BOOST_REQUIRE_GT(s_RC->get(now, target, QType(QType::A), false, &cached, who), 0); } BOOST_AUTO_TEST_CASE(test_special_types) diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc index 908c8b6304..32ded0f2fd 100644 --- a/pdns/recursordist/test-syncres_cc4.cc +++ b/pdns/recursordist/test-syncres_cc4.cc @@ -1111,6 +1111,82 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_no_rrsig) BOOST_CHECK_EQUAL(queriesCount, 1U); } +BOOST_AUTO_TEST_CASE(test_dnssec_bogus_no_rrsig_noaa) +{ + 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(target, DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_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, LWResult* res, bool* chained) { + queriesCount++; + + if (domain == target && type == QType::NS) { + + /* We are not setting AA! */ + 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, 86400); + } + + /* No RRSIG */ + + 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; + }); + + SyncRes::s_maxcachettl = 86400; + SyncRes::s_maxbogusttl = 3600; + + vector ret; + int res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoRRSIG); + /* 13 NS + 0 RRSIG */ + BOOST_REQUIRE_EQUAL(ret.size(), 13U); + /* no RRSIG so no query for DNSKEYs */ + BOOST_CHECK_EQUAL(queriesCount, 1U); + + /* again, to test the cache */ + ret.clear(); + res = sr->beginResolve(target, QType(QType::NS), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), vState::BogusNoRRSIG); + BOOST_REQUIRE_EQUAL(ret.size(), 13U); + /* check that we capped the TTL to max-cache-bogus-ttl */ + for (const auto& record : ret) { + BOOST_CHECK_LE(record.d_ttl, SyncRes::s_maxbogusttl); + } + BOOST_CHECK_EQUAL(queriesCount, 1U); +} + BOOST_AUTO_TEST_CASE(test_dnssec_insecure_unknown_ds_algorithm) { std::unique_ptr sr; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 625478a474..0761de6d80 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -2789,6 +2789,46 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname return state; } +/* This function will check whether the answer should have the AA bit set, and will set if it should be set and isn't. + This is unfortunately needed to deal with very crappy so-called DNS servers */ +void SyncRes::fixupAnswer(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery) +{ + const bool wasForwardRecurse = wasForwarded && rdQuery; + + if (wasForwardRecurse || lwr.d_aabit) { + /* easy */ + return; + } + + for (const auto& rec : lwr.d_records) { + + if (rec.d_type == QType::OPT) { + continue; + } + + if (rec.d_class != QClass::IN) { + continue; + } + + if (rec.d_type == QType::ANY) { + continue; + } + + if (rec.d_place == DNSResourceRecord::ANSWER && (QType(rec.d_type) == qtype || rec.d_type == QType::CNAME || qtype == QType::ANY) && rec.d_name == qname && rec.d_name.isPartOf(auth)) { + /* This is clearly an answer to the question we were asking, from an authoritative server that is allowed to send it. + We are going to assume this server is broken and does not know it should set the AA bit, even though it is DNS 101 */ + LOG(prefix<<"Received a record for "<& allowedAdditionals, const DNSRecord& rec) { switch(rec.d_type) { @@ -2956,6 +2996,7 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr prefix.append(depth, ' '); } + fixupAnswer(prefix, lwr, qname, qtype, auth, wasForwarded, rdQuery); sanitizeRecords(prefix, lwr, qname, qtype, auth, wasForwarded, rdQuery); std::vector> authorityRecs; diff --git a/pdns/syncres.hh b/pdns/syncres.hh index 547946832a..3ec00af666 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -850,6 +850,9 @@ private: 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, unsigned int& addressQueriesForNS); +/* This function will check whether the answer should have the AA bit set, and will set if it should be set and isn't. + This is unfortunately needed to deal with very crappy so-called DNS servers */ + void fixupAnswer(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType qtype, const DNSName& auth, bool wasForwarded, bool rdQuery); void sanitizeRecords(const std::string& prefix, LWResult& lwr, const DNSName& qname, const QType& qtype, const DNSName& auth, bool wasForwarded, bool rdQuery); 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, bool& gatherWildcardProof, unsigned int& wildcardLabelsCount, bool sendRDQuery, const ComboAddress& remoteIP); 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 bool gatherwildcardProof, const unsigned int wildcardLabelsCount, int& rcode, unsigned int depth); -- 2.47.2