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 bd0cb1a8876feba3854549b6d9f4705a4d4bf7ad..14e4eaf1df6cc5c77354b83c411fecc339ffa1e4 100644 (file)
--- 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<SyncRes> 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<Netmask>& srcmask, boost::optional<const ResolveContext&> context, std::shared_ptr<RemoteLogger> 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<DNSRecord> 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 91211cf0fea980ecd86dffb3f0b471c1ddef9670..8e9678c60a554c15074d38a769f38061cb1d3ffe 100644 (file)
--- 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: