From 405a26bd69cdc8e64e5ef11ba5e0a1bb0a04a459 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 27 Nov 2017 11:21:21 +0100 Subject: [PATCH] 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. --- pdns/recursordist/test-syncres_cc.cc | 80 ++++++++++++++++++++++++++++ pdns/syncres.cc | 14 ++++- 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index bd0cb1a88..14e4eaf1d 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 91211cf0f..8e9678c60 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: -- 2.40.0