From: Remi Gacogne Date: Wed, 15 Jan 2020 13:28:25 +0000 (+0100) Subject: rec: Only the first filtering policy should match X-Git-Tag: auth-4.3.0-beta1~30^2~4 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=124dd1d4124c52c56a93d6e765f091c88f7bc88a;p=thirdparty%2Fpdns.git rec: Only the first filtering policy should match Subsequent ones should not be applied. Also make sure that NSDNAME and NSIP triggers really stop the processing of the query, instead of just causing the current NS to be skipped. --- diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index fb7d8eb35c..9dd73d3b36 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -1324,7 +1324,7 @@ static void startDoResolve(void *p) } // Check if the query has a policy attached to it - if (wantsRPZ) { + if (wantsRPZ && appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) { appliedPolicy = luaconfsLocal->dfe.getQueryPolicy(dc->d_mdp.d_qname, dc->d_source, sr.d_discardedPolicies); } @@ -1370,15 +1370,19 @@ static void startDoResolve(void *p) // Query got not handled for QNAME Policy reasons, now actually go out to find an answer try { + sr.d_appliedPolicy = appliedPolicy; res = sr.beginResolve(dc->d_mdp.d_qname, QType(dc->d_mdp.d_qtype), dc->d_mdp.d_qclass, ret); shouldNotValidate = sr.wasOutOfBand(); } - catch(ImmediateServFailException &e) { - if(g_logCommonErrors) + catch(const ImmediateServFailException &e) { + if(g_logCommonErrors) { g_log<getRemote()<<" during resolve of '"<d_mdp.d_qname<<"' because: "<dfe.getPostPolicy(ret, sr.d_discardedPolicies); } @@ -1557,7 +1561,7 @@ static void startDoResolve(void *p) } } } - catch(ImmediateServFailException &e) { + catch(const ImmediateServFailException &e) { if(g_logCommonErrors) g_log<getRemote()<<" during validation of '"<d_mdp.d_qname<<"|"<d_mdp.d_qtype).getName()<<"' because: "<rcode=RCode::ServFail; @@ -2936,18 +2940,21 @@ static void houseKeeping(void *) try { doSecPoll(&last_secpoll); } - catch(std::exception& e) + catch(const std::exception& e) { g_log< ret; - int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); - BOOST_CHECK_EQUAL(res, -2); - BOOST_CHECK_EQUAL(ret.size(), 0U); + BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException); } BOOST_AUTO_TEST_CASE(test_nameserver_ipv6_rpz) @@ -564,9 +562,7 @@ BOOST_AUTO_TEST_CASE(test_nameserver_ipv6_rpz) g_luaconfs.setState(luaconfsCopy); vector ret; - int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); - BOOST_CHECK_EQUAL(res, -2); - BOOST_CHECK_EQUAL(ret.size(), 0U); + BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException); } BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz) @@ -608,9 +604,7 @@ BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz) g_luaconfs.setState(luaconfsCopy); vector ret; - int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); - BOOST_CHECK_EQUAL(res, -2); - BOOST_CHECK_EQUAL(ret.size(), 0U); + BOOST_CHECK_THROW(sr->beginResolve(target, QType(QType::A), QClass::IN, ret), PolicyHitException); } BOOST_AUTO_TEST_CASE(test_nameserver_name_rpz_disabled) diff --git a/pdns/syncres.cc b/pdns/syncres.cc index e4a3b962ee..486bd2bee5 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -733,6 +733,7 @@ int SyncRes::doResolve(const DNSName &qname, const QType &qtype, vector SyncRes::getAddrs(const DNSName &qname, unsigned int depth, d_requireAuthData = false; d_DNSSECValidationRequested = false; - vState newState = Indeterminate; - res_t resv4; - // If IPv4 ever becomes second class, we should revisit this - if (doResolve(qname, QType::A, resv4, depth+1, beenthere, newState) == 0) { // this consults cache, OR goes out - for (auto const &i : resv4) { - if (i.d_type == QType::A) { - if (auto rec = getRR(i)) { - ret.push_back(rec->getCA(53)); + try { + vState newState = Indeterminate; + res_t resv4; + // If IPv4 ever becomes second class, we should revisit this + if (doResolve(qname, QType::A, resv4, depth+1, beenthere, newState) == 0) { // this consults cache, OR goes out + for (auto const &i : resv4) { + if (i.d_type == QType::A) { + if (auto rec = getRR(i)) { + ret.push_back(rec->getCA(53)); + } } } } - } - if (s_doIPv6) { - if (ret.empty()) { - // We did not find IPv4 addresses, try to get IPv6 ones - newState = Indeterminate; - res_t resv6; - if (doResolve(qname, QType::AAAA, resv6, depth+1, beenthere, newState) == 0) { // this consults cache, OR goes out - for (const auto &i : resv6) { - if (i.d_type == QType::AAAA) { - if (auto rec = getRR(i)) - ret.push_back(rec->getCA(53)); + if (s_doIPv6) { + if (ret.empty()) { + // We did not find IPv4 addresses, try to get IPv6 ones + newState = Indeterminate; + res_t resv6; + if (doResolve(qname, QType::AAAA, resv6, depth+1, beenthere, newState) == 0) { // this consults cache, OR goes out + for (const auto &i : resv6) { + if (i.d_type == QType::AAAA) { + if (auto rec = getRR(i)) + ret.push_back(rec->getCA(53)); + } } } - } - } else { - // We have some IPv4 records, don't bother with going out to get IPv6, but do consult the cache - // Once IPv6 adoption matters, this needs to be revisited - res_t cset; - if (t_RC->get(d_now.tv_sec, qname, QType(QType::AAAA), false, &cset, d_cacheRemote) > 0) { - for (const auto &i : cset) { - if (i.d_ttl > (unsigned int)d_now.tv_sec ) { - if (auto rec = getRR(i)) { - ret.push_back(rec->getCA(53)); + } else { + // We have some IPv4 records, don't bother with going out to get IPv6, but do consult the cache + // Once IPv6 adoption matters, this needs to be revisited + res_t cset; + if (t_RC->get(d_now.tv_sec, qname, QType(QType::AAAA), false, &cset, d_cacheRemote) > 0) { + for (const auto &i : cset) { + if (i.d_ttl > (unsigned int)d_now.tv_sec ) { + if (auto rec = getRR(i)) { + ret.push_back(rec->getCA(53)); + } } } } } } } + catch (const PolicyHitException& e) { + /* we ignore a policy hit while trying to retrieve the addresses + of a NS and keep processing the current query */ + } d_requireAuthData = oldRequireAuthData; d_DNSSECValidationRequested = oldValidationRequested; @@ -1790,7 +1797,13 @@ static void addNXNSECS(vector&ret, const vector& records) bool SyncRes::nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& nameservers) { - if(d_wantsRPZ) { + /* we skip RPZ processing if: + - it was disabled (d_wantsRPZ is false) ; + - we already got a RPZ hit (d_appliedPolicy.d_type != DNSFilterEngine::PolicyType::None) since + the only way we can get back here is that it was a 'pass-thru' (NoAction) meaning that we should not + process any further RPZ rules. + */ + if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) { for (auto const &ns : nameservers) { d_appliedPolicy = dfe.getProcessingPolicy(ns.first, d_discardedPolicies); if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response @@ -1813,7 +1826,13 @@ bool SyncRes::nameserversBlockedByRPZ(const DNSFilterEngine& dfe, const NsSet& n bool SyncRes::nameserverIPBlockedByRPZ(const DNSFilterEngine& dfe, const ComboAddress& remoteIP) { - if (d_wantsRPZ) { + /* we skip RPZ processing if: + - it was disabled (d_wantsRPZ is false) ; + - we already got a RPZ hit (d_appliedPolicy.d_type != DNSFilterEngine::PolicyType::None) since + the only way we can get back here is that it was a 'pass-thru' (NoAction) meaning that we should not + process any further RPZ rules. + */ + if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) { d_appliedPolicy = dfe.getProcessingPolicy(remoteIP, d_discardedPolicies); if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { LOG(" (blocked by RPZ policy '"+(d_appliedPolicy.d_name ? *d_appliedPolicy.d_name : "")+"')"); @@ -3385,12 +3404,11 @@ bool SyncRes::processAnswer(unsigned int depth, LWResult& lwr, const DNSName& qn nameservers.clear(); for (auto const &nameserver : nsset) { - if (d_wantsRPZ) { + if (d_wantsRPZ && d_appliedPolicy.d_type == DNSFilterEngine::PolicyType::None) { d_appliedPolicy = dfe.getProcessingPolicy(nameserver, d_discardedPolicies); if (d_appliedPolicy.d_kind != DNSFilterEngine::PolicyKind::NoAction) { // client query needs an RPZ response LOG("however "<dfe, nameservers)) { - return -2; + /* RPZ hit */ + throw PolicyHitException(); } LOG(endl); @@ -3508,8 +3527,10 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con } } LOG(endl); - if (hitPolicy) //implies d_wantsRPZ - return -2; + if (hitPolicy) { //implies d_wantsRPZ + /* RPZ hit */ + throw PolicyHitException(); + } } for(remoteIP = remoteIPs.cbegin(); remoteIP != remoteIPs.cend(); ++remoteIP) { @@ -3663,6 +3684,10 @@ int directResolve(const DNSName& qname, const QType& qtype, int qclass, vector addrringbuf_t; extern thread_local std::unique_ptr t_servfailremotes, t_largeanswerremotes, t_remotes, t_bogusremotes, t_timeouts;