Merge pull request #11358 from omoerbeek/backport-11245-to-rec-4.6.x

rec: Backport 11245 to rec 4.6.x: a CNAME answer on DS query should abort DS retrieval
PowerDNS · Feb 28, 2022 · ad1a9b2 · ad1a9b2
2 parents 1a33160 + 7e1ecb3
commit ad1a9b2
Show file tree

Hide file tree

Showing 2 changed files with 121 additions and 3 deletions.
diff --git a/pdns/recursordist/test-syncres_cc6.cc b/pdns/recursordist/test-syncres_cc6.cc
@@ -1767,4 +1767,116 @@ BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_broken_without_ds)
   BOOST_CHECK_EQUAL(queriesCount, 6U);
 }
 
+BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_broken_cname_ds)
+{
+  /* Test an Insecure domain that responds with a CNAME on a DS query */
+  std::unique_ptr<SyncRes> sr;
+  initSR(sr, true);
+
+  setDNSSECValidation(sr, DNSSECMode::ValidateAll);
+
+  primeHints();
+  const DNSName target("www.sub.powerdns.com.");
+  const ComboAddress targetAddr("192.0.2.42");
+  testkeysset_t keys, pdnskeys;
+
+  auto luaconfsCopy = g_luaconfs.getCopy();
+  luaconfsCopy.dsAnchors.clear();
+
+  // We have two set of keys as powerdns.com and sub.powerdns.com are Insecure but still have RRSIGS
+  generateKeyMaterial(g_rootdnsname, DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys, luaconfsCopy.dsAnchors);
+  generateKeyMaterial(DNSName("com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, keys);
+
+  generateKeyMaterial(DNSName("powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, pdnskeys);
+  generateKeyMaterial(DNSName("sub.powerdns.com."), DNSSECKeeper::ECDSA256, DNSSECKeeper::DIGEST_SHA256, pdnskeys);
+
+  g_luaconfs.setState(luaconfsCopy);
+
+  size_t queriesCount = 0;
+
+  sr->setAsyncCallback([target, targetAddr, &queriesCount, keys, pdnskeys](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, LWResult* res, bool* chained) {
+    queriesCount++;
+
+    if (type == QType::DS) {
+      // Return a signed CNAME on a DS query for powerdns.com and sub.powerdns.com
+      if (domain == DNSName("powerdns.com.") || domain == DNSName("sub.powerdns.com.")) {
+        setLWResult(res, 0, true, false, true);
+        addRecordToLW(res, domain, QType::CNAME, "some.name", DNSResourceRecord::ANSWER, 300);
+        addRRSIG(pdnskeys, res->d_records, domain, 300);
+        return LWResult::Result::Success;
+      }
+      else {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+      }
+    }
+    else if (type == QType::DNSKEY) {
+      if (domain == DNSName("powerdns.com.") || domain == DNSName("sub.powerdns.com.")) {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, pdnskeys);
+      }
+      else {
+        return genericDSAndDNSKEYHandler(res, domain, domain, type, keys);
+      }
+    }
+    else {
+      if (isRootServer(ip)) {
+        setLWResult(res, 0, false, false, true);
+        addRecordToLW(res, "com.", QType::NS, "a.gtld-servers.com.", DNSResourceRecord::AUTHORITY, 3600);
+        addDS(DNSName("com."), 300, res->d_records, keys);
+        addRRSIG(keys, res->d_records, DNSName("."), 300);
+        addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+        return LWResult::Result::Success;
+      }
+      else if (ip == ComboAddress("192.0.2.1:53")) {
+        if (domain == DNSName("com.")) {
+          setLWResult(res, 0, true, false, true);
+          addRecordToLW(res, DNSName("com."), QType::NS, "a.gtld-servers.com.");
+          addRRSIG(keys, res->d_records, DNSName("com."), 300);
+          addRecordToLW(res, "a.gtld-servers.com.", QType::A, "192.0.2.1", DNSResourceRecord::ADDITIONAL, 3600);
+        }
+        else {
+          setLWResult(res, 0, false, false, true);
+          addRecordToLW(res, DNSName("powerdns.com."), QType::NS, "ns1.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600);
+          addNSECRecordToLW(DNSName("powerdns.com."), DNSName("z.powerdns.com."), {QType::NS, QType::RRSIG}, 600, res->d_records);
+          addRRSIG(keys, res->d_records, DNSName("com."), 300);
+          addRecordToLW(res, "ns1.powerdns.com.", QType::A, "192.0.2.2", DNSResourceRecord::ADDITIONAL, 3600);
+        }
+        return LWResult::Result::Success;
+      }
+      else if (ip == ComboAddress("192.0.2.2:53")) {
+        setLWResult(res, 0, false, false, true);
+        addRecordToLW(res, DNSName("sub.powerdns.com."), QType::NS, "ns1.sub.powerdns.com.", DNSResourceRecord::AUTHORITY, 3600);
+        addRRSIG(pdnskeys, res->d_records, DNSName("powerdns.com."), 300);
+
+        addRecordToLW(res, "ns1.sub.powerdns.com.", QType::A, "192.0.2.3", DNSResourceRecord::ADDITIONAL, 3600);
+        return LWResult::Result::Success;
+      }
+      else if (ip == ComboAddress("192.0.2.3:53")) {
+        setLWResult(res, 0, true, false, true);
+        addRecordToLW(res, domain, QType::A, targetAddr.toString(), DNSResourceRecord::ANSWER, 3600);
+        addRRSIG(pdnskeys, res->d_records, DNSName("sub.powerdns.com."), 300);
+        return LWResult::Result::Success;
+      }
+    }
+
+    return LWResult::Result::Timeout;
+  });
+
+  vector<DNSRecord> ret;
+  int res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Insecure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+  BOOST_CHECK(ret[0].d_type == QType::A);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
+
+  /* again, to test the cache */
+  ret.clear();
+  res = sr->beginResolve(target, QType(QType::A), QClass::IN, ret);
+  BOOST_CHECK_EQUAL(res, RCode::NoError);
+  BOOST_CHECK_EQUAL(sr->getValidationState(), vState::Insecure);
+  BOOST_REQUIRE_EQUAL(ret.size(), 2U);
+  BOOST_CHECK(ret[0].d_type == QType::A);
+  BOOST_CHECK_EQUAL(queriesCount, 8U);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/pdns/syncres.cc b/pdns/syncres.cc
@@ -2860,12 +2860,18 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname
         state = vState::BogusSelfSignedDS;
         dsFailed = true;
       }
-      else if (qtype == QType::DS && signer == qname && !signer.isRoot() && (type == QType::SOA || type == QType::NSEC || type == QType::NSEC3)) {
+      else if (qtype == QType::DS && signer == qname && !signer.isRoot()) {
+        if (type == QType::SOA || type == QType::NSEC || type == QType::NSEC3) {
         /* if we are trying to validate the DS or more likely NSEC(3)s proving that it does not exist, we have a problem.
            In that case let's go Bogus (we will check later if we missed a cut)
         */
-        state = vState::BogusSelfSignedDS;
-        dsFailed = true;
+          state = vState::BogusSelfSignedDS;
+          dsFailed = true;
+        }
+        else if (type == QType::CNAME) {
+          state = vState::BogusUnableToGetDSs;
+          dsFailed = true;
+        }
       }
       else if (qtype == QType::DNSKEY && signer == qname) {
         /* that actually does happen when a server returns NS records in authority