From: Otto Moerbeek Date: Wed, 21 Aug 2024 09:09:42 +0000 (+0200) Subject: rec: refuse to store very large RRSets in the cache and ServFail on retrieval of... X-Git-Tag: rec-5.2.0-alpha1~53^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=60aee317a54aa80cec6c4574d40b3632cf6c0546;p=thirdparty%2Fpdns.git rec: refuse to store very large RRSets in the cache and ServFail on retrieval of those --- diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 6a1eb68358..49c1154fc8 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -1791,6 +1791,8 @@ static int initSyncRes(Logr::log_t log) MemRecursorCache::s_maxServedStaleExtensions = sse; NegCache::s_maxServedStaleExtensions = sse; } + MemRecursorCache::s_maxRRSetSize = ::arg().asNum("max-rrset-size"); + MemRecursorCache::s_limitQTypeAny = ::arg().mustDo("limit-qtype-any"); if (SyncRes::s_tcp_fast_open_connect) { checkFastOpenSysctl(true, log); diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc index b888bf2ef1..b487ffc337 100644 --- a/pdns/recursordist/recursor_cache.cc +++ b/pdns/recursordist/recursor_cache.cc @@ -52,6 +52,8 @@ */ uint16_t MemRecursorCache::s_maxServedStaleExtensions; +uint16_t MemRecursorCache::s_maxRRSetSize = 256; +bool MemRecursorCache::s_limitQTypeAny = true; void MemRecursorCache::resetStaticsForTests() { @@ -150,6 +152,9 @@ static void ptrAssign(T* ptr, const T& value) time_t MemRecursorCache::handleHit(time_t now, MapCombo::LockedContent& content, MemRecursorCache::OrderedTagIterator_t& entry, const DNSName& qname, uint32_t& origTTL, vector* res, vector>* signatures, std::vector>* authorityRecs, bool* variable, boost::optional& state, bool* wasAuth, DNSName* fromAuthZone, ComboAddress* fromAuthIP) { // MUTEX SHOULD BE ACQUIRED (as indicated by the reference to the content which is protected by a lock) + if (entry->d_tooBig) { + throw ImmediateServFailException("too many records in RRSet"); + } time_t ttd = entry->d_ttd; if (ttd <= now) { // Expired, don't bother returning contents. Callers *MUST* check return value of get(), and only look at the entry @@ -163,6 +168,10 @@ time_t MemRecursorCache::handleHit(time_t now, MapCombo::LockedContent& content, } if (res != nullptr) { + if (s_limitQTypeAny && res->size() + entry->d_records.size() > s_maxRRSetSize) { + throw ImmediateServFailException("too many records in result"); + } + res->reserve(res->size() + entry->d_records.size()); for (const auto& record : entry->d_records) { @@ -350,7 +359,7 @@ time_t MemRecursorCache::fakeTTD(MemRecursorCache::OrderedTagIterator_t& entry, } // returns -1 for no hits -time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype, Flags flags, vector* res, const ComboAddress& who, const OptTag& routingTag, vector>* signatures, std::vector>* authorityRecs, bool* variable, vState* state, bool* wasAuth, DNSName* fromAuthZone, ComboAddress* fromAuthIP) +time_t MemRecursorCache::get(time_t now, const DNSName& qname, const QType qtype, Flags flags, vector* res, const ComboAddress& who, const OptTag& routingTag, vector>* signatures, std::vector>* authorityRecs, bool* variable, vState* state, bool* wasAuth, DNSName* fromAuthZone, ComboAddress* fromAuthIP) // NOLINT(readability-function-cognitive-complexity) { bool requireAuth = (flags & RequireAuth) != 0; bool refresh = (flags & Refresh) != 0; @@ -592,7 +601,6 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qty cacheEntry.d_signatures = signatures; cacheEntry.d_authorityRecs = authorityRecs; cacheEntry.d_records.clear(); - cacheEntry.d_records.reserve(content.size()); cacheEntry.d_authZone = authZone; if (from) { cacheEntry.d_from = *from; @@ -601,6 +609,15 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qty cacheEntry.d_from = ComboAddress(); } + size_t toStore = content.size(); + if (toStore <= s_maxRRSetSize) { + cacheEntry.d_tooBig = false; + } + else { + toStore = 1; // record cache does not like empty RRSets + cacheEntry.d_tooBig = true; + } + cacheEntry.d_records.reserve(toStore); for (const auto& record : content) { /* Yes, we have altered the d_ttl value by adding time(nullptr) to it prior to calling this function, so the TTL actually holds a TTD. */ @@ -615,8 +632,10 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qty cacheEntry.d_orig_ttl = SyncRes::s_minimumTTL; } cacheEntry.d_records.push_back(record.getContent()); + if (--toStore == 0) { + break; + } } - if (!isNew) { moveCacheItemToBack(lockedShard->d_map, stored); } @@ -801,7 +820,7 @@ uint64_t MemRecursorCache::doDump(int fileDesc, size_t maxCacheEntries) for (const auto& record : recordSet.d_records) { count++; try { - fprintf(filePtr.get(), "%s %" PRIu32 " %" PRId64 " IN %s %s ; (%s) auth=%i zone=%s from=%s nm=%s rtag=%s ss=%hd\n", recordSet.d_qname.toString().c_str(), recordSet.d_orig_ttl, static_cast(recordSet.d_ttd - now), recordSet.d_qtype.toString().c_str(), record->getZoneRepresentation().c_str(), vStateToString(recordSet.d_state).c_str(), static_cast(recordSet.d_auth), recordSet.d_authZone.toLogString().c_str(), recordSet.d_from.toString().c_str(), recordSet.d_netmask.empty() ? "" : recordSet.d_netmask.toString().c_str(), !recordSet.d_rtag ? "" : recordSet.d_rtag.get().c_str(), recordSet.d_servedStale); + fprintf(filePtr.get(), "%s %" PRIu32 " %" PRId64 " IN %s %s ; (%s) auth=%i zone=%s from=%s nm=%s rtag=%s ss=%hd%s\n", recordSet.d_qname.toString().c_str(), recordSet.d_orig_ttl, static_cast(recordSet.d_ttd - now), recordSet.d_qtype.toString().c_str(), record->getZoneRepresentation().c_str(), vStateToString(recordSet.d_state).c_str(), static_cast(recordSet.d_auth), recordSet.d_authZone.toLogString().c_str(), recordSet.d_from.toString().c_str(), recordSet.d_netmask.empty() ? "" : recordSet.d_netmask.toString().c_str(), !recordSet.d_rtag ? "" : recordSet.d_rtag.get().c_str(), recordSet.d_servedStale, recordSet.d_tooBig ? " (too big!)" : ""); } catch (...) { fprintf(filePtr.get(), "; error printing '%s'\n", recordSet.d_qname.empty() ? "EMPTY" : recordSet.d_qname.toString().c_str()); diff --git a/pdns/recursordist/recursor_cache.hh b/pdns/recursordist/recursor_cache.hh index 360e97958c..adf65ec6a8 100644 --- a/pdns/recursordist/recursor_cache.hh +++ b/pdns/recursordist/recursor_cache.hh @@ -54,6 +54,11 @@ public: // The time a stale cache entry is extended static constexpr uint32_t s_serveStaleExtensionPeriod = 30; + // Maximum size of RRSet we are willing to cache. If the RRSet is larger, we do create an entry, + // but mark it as too big. Subsequent gets will cause an ImmediateServFailException to be thrown. + static uint16_t s_maxRRSetSize; + static bool s_limitQTypeAny; + [[nodiscard]] size_t size() const; [[nodiscard]] size_t bytes(); [[nodiscard]] pair stats(); @@ -142,6 +147,7 @@ private: QType d_qtype; bool d_auth; mutable bool d_submitted{false}; // whether this entry has been queued for refetch + bool d_tooBig{false}; }; /* The ECS Index (d_ecsIndex) keeps track of whether there is any ECS-specific diff --git a/pdns/recursordist/settings/table.py b/pdns/recursordist/settings/table.py index 92caff19dc..d06bf885e7 100644 --- a/pdns/recursordist/settings/table.py +++ b/pdns/recursordist/settings/table.py @@ -1555,7 +1555,31 @@ Maximum length of a CNAME chain. If a CNAME chain exceeds this length, a ``ServF Previously, this limit was fixed at 10. ''', 'versionadded': '5.1.0' - }, + }, + { + 'name' : 'limit_qtype_any', + 'section' : 'recordcache', + 'type' : LType.Bool, + 'default' : 'true', + 'help' : 'Limit answers to ANY queries in size', + 'doc' : ''' +Limit answers to ANY queries constructed from the record cache in size. +Trying to retrieve more than `:xref:setting-max-rrset-size` records will result in a ``ServFail``', + ''', + 'versionadded': ['4.9.9', '5.0.9', '5.1.2'] + }, + { + 'name' : 'max_rrset_size', + 'section' : 'recordcache', + 'type' : LType.Uint64, + 'default' : '256', + 'help' : 'Maximum size of RRSet in cache', + 'doc' : ''' +Maximum size of RRSets in cache. +Trying to retrieve larger RRSets will result in a ``ServFail``.', + ''', + 'versionadded': ['4.9.x', '5.0.y', '5.1.x'] + }, { 'name' : 'max_ns_address_qperq', 'section' : 'outgoing', diff --git a/pdns/recursordist/test-recursorcache_cc.cc b/pdns/recursordist/test-recursorcache_cc.cc index 27b6e90738..b2f99e2b9c 100644 --- a/pdns/recursordist/test-recursorcache_cc.cc +++ b/pdns/recursordist/test-recursorcache_cc.cc @@ -11,6 +11,7 @@ #include "iputils.hh" #include "recursor_cache.hh" +#include "syncres.hh" BOOST_AUTO_TEST_SUITE(recursorcache_cc) @@ -162,6 +163,7 @@ static void simple(time_t now) BOOST_CHECK_EQUAL(retrieved.size(), 0U); // QType::ANY should return any qtype, so from the right subnet we should get all of them + MemRecursorCache::s_limitQTypeAny = false; BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::ANY), MemRecursorCache::None, &retrieved, ComboAddress("192.0.2.3")), (ttd - now)); BOOST_CHECK_EQUAL(retrieved.size(), 3U); for (const auto& rec : retrieved) { @@ -389,6 +391,53 @@ BOOST_AUTO_TEST_CASE(test_RecursorCacheSimpleDistantFuture) } #endif +BOOST_AUTO_TEST_CASE(test_RecursorCacheBig) +{ + MemRecursorCache::resetStaticsForTests(); + MemRecursorCache MRC; + + std::vector records; + std::vector retrieved; + const DNSName authZone("."); + + time_t now = time(nullptr); + time_t ttd = now + 30; + DNSName power("powerdns.com."); + DNSRecord dr0; + string dr0Content("2001:DB8::"); + dr0.d_name = power; + dr0.d_type = QType::AAAA; + dr0.d_class = QClass::IN; + dr0.d_ttl = static_cast(ttd); // XXX truncation + dr0.d_place = DNSResourceRecord::ANSWER; + for (int i = 0; i < MemRecursorCache::s_maxRRSetSize; i++) { + dr0.setContent(std::make_shared(dr0Content + std::to_string(i))); + records.push_back(dr0); + } + + // This one should fit + MRC.replace(now, power, QType::AAAA, records, {}, {}, true, authZone, boost::none); + BOOST_CHECK_EQUAL(MRC.size(), 1U); + BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::AAAA), MemRecursorCache::None, &retrieved, ComboAddress()), (ttd - now)); + BOOST_CHECK_EQUAL(retrieved.size(), MemRecursorCache::s_maxRRSetSize); + + dr0.setContent(std::make_shared(dr0Content + std::to_string(MemRecursorCache::s_maxRRSetSize))); + records.push_back(dr0); + // This one is too large and should throw exception + MRC.replace(now, power, QType::AAAA, records, {}, {}, true, authZone, boost::none); + BOOST_CHECK_EQUAL(MRC.size(), 1U); + + BOOST_CHECK_THROW((void)MRC.get(now, power, QType(QType::AAAA), MemRecursorCache::None, &retrieved, ComboAddress()), + ImmediateServFailException); + + records.resize(1); + // This one should fit again + MRC.replace(now, power, QType::AAAA, records, {}, {}, true, authZone, boost::none); + BOOST_CHECK_EQUAL(MRC.size(), 1U); + BOOST_CHECK_EQUAL(MRC.get(now, power, QType(QType::AAAA), MemRecursorCache::None, &retrieved, ComboAddress()), (ttd - now)); + BOOST_CHECK_EQUAL(retrieved.size(), 1U); +} + BOOST_AUTO_TEST_CASE(test_RecursorCacheGhost) { MemRecursorCache::resetStaticsForTests(); diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index 7f253647d1..96aa8e164a 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -143,6 +143,8 @@ void initSR(bool debug) RecursorPacketCache::s_refresh_ttlperc = 0; MemRecursorCache::s_maxServedStaleExtensions = 0; + MemRecursorCache::s_maxRRSetSize = 100; + MemRecursorCache::s_limitQTypeAny = true; NegCache::s_maxServedStaleExtensions = 0; g_recCache = std::make_unique(); g_negCache = std::make_unique(); diff --git a/pdns/recursordist/test-syncres_cc3.cc b/pdns/recursordist/test-syncres_cc3.cc index 303bf97c7f..8f683ac388 100644 --- a/pdns/recursordist/test-syncres_cc3.cc +++ b/pdns/recursordist/test-syncres_cc3.cc @@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(test_unauth_any) const DNSName target("powerdns.com."); - sr->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int /* type */, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) { + sr->setAsyncCallback([&](const ComboAddress& address, const DNSName& domain, int type, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, LWResult* res, bool* /* chained */) { if (isRootServer(address)) { setLWResult(res, 0, false, false, true); addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.net.", DNSResourceRecord::AUTHORITY, 172800); @@ -62,19 +62,41 @@ BOOST_AUTO_TEST_CASE(test_unauth_any) return LWResult::Result::Success; } if (address == ComboAddress("192.0.2.1:53")) { - - setLWResult(res, 0, false, false, true); - addRecordToLW(res, domain, QType::A, "192.0.2.42"); - return LWResult::Result::Success; + if (type == QType::A) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::A, "192.0.2.42"); + addRecordToLW(res, domain, QType::A, "192.0.2.43"); + return LWResult::Result::Success; + } + if (type == QType::AAAA) { + setLWResult(res, 0, false, false, true); + addRecordToLW(res, domain, QType::AAAA, "::1"); + return LWResult::Result::Success; + } } return LWResult::Result::Timeout; }); vector ret; - int res = sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 2U); + + ret.clear(); + res = sr->beginResolve(target, QType(QType::AAAA), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(ret.size(), 1U); + + ret.clear(); + MemRecursorCache::s_maxRRSetSize = 2; + BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret), ImmediateServFailException); + + MemRecursorCache::s_limitQTypeAny = false; + ret.clear(); + res = sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(ret.size(), 3U); } static void test_no_data_f(bool qmin) diff --git a/pdns/recursordist/test-syncres_cc8.cc b/pdns/recursordist/test-syncres_cc8.cc index de9f89529d..d167fa4342 100644 --- a/pdns/recursordist/test-syncres_cc8.cc +++ b/pdns/recursordist/test-syncres_cc8.cc @@ -1677,6 +1677,20 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cache_secure_any) ret.clear(); /* third one _does_ require validation */ sr->setDNSSECValidationRequested(true); + MemRecursorCache::s_maxRRSetSize = 1; + BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret), ImmediateServFailException); + // BOOST_CHECK_EQUAL(res, RCode::NoError); + // BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure); + // BOOST_REQUIRE_EQUAL(ret.size(), 2U); + // for (const auto& record : ret) { + // BOOST_CHECK(record.d_type == QType::A || record.d_type == QType::AAAA || record.d_type == QType::RRSIG); + // } + BOOST_CHECK_EQUAL(queriesCount, 2U); + + ret.clear(); + /* next one _does_ require validation */ + MemRecursorCache::s_limitQTypeAny = false; + sr->setDNSSECValidationRequested(true); res = sr->beginResolve(target, QType(QType::ANY), QClass::IN, ret); BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Secure);