From: Remi Gacogne Date: Mon, 27 Nov 2017 10:21:21 +0000 (+0100) Subject: rec: Store additional records as non-auth, even on AA=1 answers X-Git-Tag: auth-4.1.0~9^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F5997%2Fhead;p=thirdparty%2Fpdns.git rec: Store additional records as non-auth, even on AA=1 answers We used to store additional records in AA=1 answers as auth. In addition to being wrong, it also broke DNSSEC validation if the record was stored as Indeterminate because while we take care of not validating additional records when processing an answer, we have no way of knowing in which section a record was originally located when we retrieve it from the cache. When an answer becomes too big to fit in the requester UDP payload, rfc4035 allows the sender to keep records in the additional section while omitting the corresponding RRSIGs, without setting the TC bit. --- diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index bd0cb1a887..14e4eaf1df 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -8793,6 +8793,86 @@ BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_cname_cache_bogus) { BOOST_CHECK_EQUAL(queriesCount, 5); } +BOOST_AUTO_TEST_CASE(test_dnssec_validation_additional_without_rrsig) { + /* + We get a record from a secure zone in the additional section, without + the corresponding RRSIG. The record should not be marked as authoritative + and should be correctly validated. + */ + std::unique_ptr sr; + initSR(sr, true); + + setDNSSECValidation(sr, DNSSECMode::Process); + + primeHints(); + const DNSName target("com."); + const DNSName addTarget("nsX.com."); + testkeysset_t keys; + + auto luaconfsCopy = g_luaconfs.getCopy(); + luaconfsCopy.dsAnchors.clear(); + generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::SHA256, keys, luaconfsCopy.dsAnchors); + g_luaconfs.setState(luaconfsCopy); + + size_t queriesCount = 0; + + sr->setAsyncCallback([target,addTarget,&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) { + queriesCount++; + + if (type == QType::DS || type == QType::DNSKEY) { + if (domain == addTarget) { + DNSName auth(domain); + /* no DS for com, auth will be . */ + auth.chopOff(); + return genericDSAndDNSKEYHandler(res, domain, auth, type, keys, false); + } + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); + } + else { + if (domain == target && type == QType::A) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, target, QType::A, "192.0.2.1"); + addRRSIG(keys, res->d_records, DNSName("."), 300); + addRecordToLW(res, addTarget, QType::A, "192.0.2.42", DNSResourceRecord::ADDITIONAL); + /* no RRSIG for the additional record */ + return 1; + } else if (domain == addTarget && type == QType::A) { + setLWResult(res, 0, true, false, true); + addRecordToLW(res, addTarget, QType::A, "192.0.2.42"); + addRRSIG(keys, res->d_records, DNSName("."), 300); + return 1; + } + } + + return 0; + }); + + vector ret; + /* first query for target/A, will pick up the additional record as non-auth / unvalidated */ + sr->setDNSSECValidationRequested(false); + int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Indeterminate); + BOOST_CHECK_EQUAL(ret.size(), 2); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::RRSIG || record.d_type == QType::A); + } + BOOST_CHECK_EQUAL(queriesCount, 1); + + ret.clear(); + /* ask for the additional record directly, we should not use + the non-auth one and issue a new query, properly validated */ + sr->setDNSSECValidationRequested(true); + res = sr->beginResolve(addTarget, QType(QType::A), QClass::IN, ret); + BOOST_CHECK_EQUAL(res, RCode::NoError); + BOOST_CHECK_EQUAL(sr->getValidationState(), Secure); + BOOST_CHECK_EQUAL(ret.size(), 2); + for (const auto& record : ret) { + BOOST_CHECK(record.d_type == QType::RRSIG || record.d_type == QType::A); + } + BOOST_CHECK_EQUAL(queriesCount, 5); +} + BOOST_AUTO_TEST_CASE(test_dnssec_validation_from_negcache_secure) { /* Validation is optional, and the first query does not ask for it, diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 91211cf0fe..8e9678c60a 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -2004,7 +2004,19 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr if(i->second.records.empty()) // this happens when we did store signatures, but passed on the records themselves continue; - bool isAA = lwr.d_aabit; + /* Even if the AA bit is set, additional data cannot be considered + as authoritative. This is especially important during validation + because keeping records in the additional section is allowed even + if the corresponding RRSIGs are not included, without setting the TC + bit, as stated in rfc4035's section 3.1.1. Including RRSIG RRs in a Response: + "When placing a signed RRset in the Additional section, the name + server MUST also place its RRSIG RRs in the Additional section. + If space does not permit inclusion of both the RRset and its + associated RRSIG RRs, the name server MAY retain the RRset while + dropping the RRSIG RRs. If this happens, the name server MUST NOT + 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)) { /* rfc2181 states: