From: Remi Gacogne Date: Fri, 31 May 2019 16:24:27 +0000 (+0200) Subject: rec: Better detection of Bogus zone cuts for DNSSEC validation X-Git-Tag: dnsdist-1.4.0-rc1~120^2~1 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=dd359a9fc01c318ef2fc1753d3e8170be5977c05;p=pdns rec: Better detection of Bogus zone cuts for DNSSEC validation --- diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc index 37c63940e..2d094bce5 100644 --- a/pdns/recursordist/test-syncres_cc1.cc +++ b/pdns/recursordist/test-syncres_cc1.cc @@ -1363,7 +1363,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) { } if (domain.countLabels() == 1 && type == QType::DS) { // powerdns|DS or example|DS setLWResult(res, 0, true, false, true); - addDS(domain, 300, res->d_records, keys); + addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); addRRSIG(keys, res->d_records, DNSName("."), 300); return 1; } @@ -1392,7 +1392,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) { return 1; } if (domain == target && type == QType::DS) { // dname.powerdns|DS - return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys); + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); } if (domain == target) { setLWResult(res, 0, true, false, false); @@ -1408,8 +1408,8 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_secure) { addRRSIG(keys, res->d_records, domain, 300); return 1; } - if (domain == target && type == QType::DS) { // dname.example|DS - return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys); + if (domain == cnameTarget && type == QType::DS) { // dname.example|DS + return genericDSAndDNSKEYHandler(res, domain, domain, type, keys, false); } if (domain == cnameTarget) { setLWResult(res, 0, true, false, false); @@ -1512,7 +1512,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) { } if (domain == dnameOwner && type == QType::DS) { // powerdns|DS setLWResult(res, 0, true, false, true); - addDS(domain, 300, res->d_records, keys); + addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); addRRSIG(keys, res->d_records, DNSName("."), 300); return 1; } @@ -1544,7 +1544,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) { return 1; } if (domain == target && type == QType::DS) { // dname.powerdns|DS - return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys); + return genericDSAndDNSKEYHandler(res, domain, dnameOwner, type, keys, false); } if (domain == target) { setLWResult(res, 0, true, false, false); @@ -1555,7 +1555,7 @@ BOOST_AUTO_TEST_CASE(test_dname_dnssec_insecure) { } } else if (ip == ComboAddress("192.0.2.2:53")) { if (domain == target && type == QType::DS) { // dname.example|DS - return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys); + return genericDSAndDNSKEYHandler(res, domain, dnameTarget, type, keys, false); } if (domain == cnameTarget) { setLWResult(res, 0, true, false, false); diff --git a/pdns/recursordist/test-syncres_cc4.cc b/pdns/recursordist/test-syncres_cc4.cc index a515f9362..aceff1ff1 100644 --- a/pdns/recursordist/test-syncres_cc4.cc +++ b/pdns/recursordist/test-syncres_cc4.cc @@ -1202,7 +1202,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 4); + BOOST_CHECK_EQUAL(queriesCount, 3); /* again, to test the cache */ ret.clear(); @@ -1210,7 +1210,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 4); + BOOST_CHECK_EQUAL(queriesCount, 3); /* now we ask directly for the DS */ ret.clear(); @@ -1218,7 +1218,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 1); - BOOST_CHECK_EQUAL(queriesCount, 4); + BOOST_CHECK_EQUAL(queriesCount, 3); } BOOST_AUTO_TEST_CASE(test_dnssec_bogus_unsigned_ds_direct) { diff --git a/pdns/recursordist/test-syncres_cc6.cc b/pdns/recursordist/test-syncres_cc6.cc index 7bb466afb..5b0a1e3f3 100644 --- a/pdns/recursordist/test-syncres_cc6.cc +++ b/pdns/recursordist/test-syncres_cc6.cc @@ -34,6 +34,13 @@ BOOST_AUTO_TEST_CASE(test_dnssec_no_ds_on_referral_secure) { auth.chopOff(); dsQueriesCount++; + if (domain == target) { + if (genericDSAndDNSKEYHandler(res, domain, auth, type, keys, false) == 0) { + return 0; + } + return 1; + } + setLWResult(res, 0, true, false, true); addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); addRRSIG(keys, res->d_records, auth, 300); @@ -145,8 +152,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) { setLWResult(res, 0, true, false, true); if (domain == target) { - addRecordToLW(res, domain, QType::SOA, "ns1.powerdns.com. blah. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400); - addRRSIG(keys, res->d_records, target, 300); + addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); + addRRSIG(keys, res->d_records, domain, 300); } else { addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); @@ -218,7 +225,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 9); + BOOST_CHECK_EQUAL(queriesCount, 8); /* again, to test the cache */ ret.clear(); @@ -226,7 +233,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_ds_sign_loop) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 9); + BOOST_CHECK_EQUAL(queriesCount, 8); } BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) { @@ -260,14 +267,8 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) { auth.chopOff(); setLWResult(res, 0, true, false, true); - if (domain == target) { - addRecordToLW(res, domain, QType::SOA, "ns1.powerdns.com. blah. 2017032800 1800 900 604800 86400", DNSResourceRecord::AUTHORITY, 86400); - addRRSIG(keys, res->d_records, target, 300); - } - else { - addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); - addRRSIG(keys, res->d_records, auth, 300); - } + addDS(domain, 300, res->d_records, keys, DNSResourceRecord::ANSWER); + addRRSIG(keys, res->d_records, auth, 300); return 1; } else if (type == QType::DNSKEY) { @@ -333,7 +334,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 9); + BOOST_CHECK_EQUAL(queriesCount, 10); /* again, to test the cache */ ret.clear(); @@ -341,7 +342,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_dnskey_signed_child) { BOOST_CHECK_EQUAL(res, RCode::NoError); BOOST_CHECK_EQUAL(sr->getValidationState(), Bogus); BOOST_REQUIRE_EQUAL(ret.size(), 2); - BOOST_CHECK_EQUAL(queriesCount, 9); + BOOST_CHECK_EQUAL(queriesCount, 10); } BOOST_AUTO_TEST_CASE(test_dnssec_no_ds_on_referral_insecure) { diff --git a/pdns/recursordist/test-syncres_cc7.cc b/pdns/recursordist/test-syncres_cc7.cc index 59550b928..ae16ec444 100644 --- a/pdns/recursordist/test-syncres_cc7.cc +++ b/pdns/recursordist/test-syncres_cc7.cc @@ -43,6 +43,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) { if (domain == DNSName("com.")) { addRecordToLW(res, domain, QType::SOA, "pdns-public-ns1.powerdns.com. pieter\\.lexis.powerdns.com. 2017032301 10800 3600 604800 3600", DNSResourceRecord::AUTHORITY, 3600); + addRRSIG(keys, res->d_records, DNSName("."), 300); /* no DS */ addNSECRecordToLW(DNSName("com."), DNSName("dom."), { QType::NS }, 600, res->d_records); addRRSIG(keys, res->d_records, DNSName("."), 300); @@ -119,7 +120,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) { BOOST_CHECK_EQUAL(sr->getValidationState(), Secure); BOOST_REQUIRE_EQUAL(ret.size(), 2); BOOST_CHECK(ret[0].d_type == QType::A); - BOOST_CHECK_EQUAL(queriesCount, 8); + BOOST_CHECK_EQUAL(queriesCount, 7); /* again, to test the cache */ ret.clear(); @@ -128,7 +129,7 @@ BOOST_AUTO_TEST_CASE(test_dnssec_insecure_to_ta_skipped_cut) { BOOST_CHECK_EQUAL(sr->getValidationState(), Secure); BOOST_REQUIRE_EQUAL(ret.size(), 2); BOOST_CHECK(ret[0].d_type == QType::A); - BOOST_CHECK_EQUAL(queriesCount, 8); + BOOST_CHECK_EQUAL(queriesCount, 7); } BOOST_AUTO_TEST_CASE(test_dnssec_secure_to_insecure_nodata) { diff --git a/pdns/syncres.cc b/pdns/syncres.cc index 2dd483253..fa73da994 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -1747,58 +1747,63 @@ vState SyncRes::getDSRecords(const DNSName& zone, dsmap_t& ds, bool taOnly, unsi d_skipCNAMECheck = oldSkipCNAME; if (rcode == RCode::NoError || (rcode == RCode::NXDomain && !bogusOnNXD)) { - uint8_t bestDigestType = 0; - if (state == Secure) { - bool gotCNAME = false; - for (const auto& record : dsrecords) { - if (record.d_type == QType::DS) { - const auto dscontent = getRR(record); - if (dscontent && isSupportedDS(*dscontent)) { - // Make GOST a lower prio than SHA256 - if (dscontent->d_digesttype == DNSSECKeeper::GOST && bestDigestType == DNSSECKeeper::SHA256) { - continue; - } - if (dscontent->d_digesttype > bestDigestType || (bestDigestType == DNSSECKeeper::GOST && dscontent->d_digesttype == DNSSECKeeper::SHA256)) { - bestDigestType = dscontent->d_digesttype; - } - ds.insert(*dscontent); + bool gotCNAME = false; + for (const auto& record : dsrecords) { + if (record.d_type == QType::DS) { + const auto dscontent = getRR(record); + if (dscontent && isSupportedDS(*dscontent)) { + // Make GOST a lower prio than SHA256 + if (dscontent->d_digesttype == DNSSECKeeper::GOST && bestDigestType == DNSSECKeeper::SHA256) { + continue; } + if (dscontent->d_digesttype > bestDigestType || (bestDigestType == DNSSECKeeper::GOST && dscontent->d_digesttype == DNSSECKeeper::SHA256)) { + bestDigestType = dscontent->d_digesttype; + } + ds.insert(*dscontent); } - else if (record.d_type == QType::CNAME && record.d_name == zone) { - gotCNAME = true; - } } + else if (record.d_type == QType::CNAME && record.d_name == zone) { + gotCNAME = true; + } + } - /* RFC 4509 section 3: "Validator implementations SHOULD ignore DS RRs containing SHA-1 - * digests if DS RRs with SHA-256 digests are present in the DS RRset." - * As SHA348 is specified as well, the spirit of the this line is "use the best algorithm". - */ - for (auto dsrec = ds.begin(); dsrec != ds.end(); ) { - if (dsrec->d_digesttype != bestDigestType) { - dsrec = ds.erase(dsrec); - } - else { - ++dsrec; - } + /* RFC 4509 section 3: "Validator implementations SHOULD ignore DS RRs containing SHA-1 + * digests if DS RRs with SHA-256 digests are present in the DS RRset." + * As SHA348 is specified as well, the spirit of the this line is "use the best algorithm". + */ + for (auto dsrec = ds.begin(); dsrec != ds.end(); ) { + if (dsrec->d_digesttype != bestDigestType) { + dsrec = ds.erase(dsrec); + } + else { + ++dsrec; } + } - if (rcode == RCode::NoError && ds.empty()) { - if (foundCut) { - if (gotCNAME || denialProvesNoDelegation(zone, dsrecords)) { - /* we are still inside the same Secure zone */ + if (rcode == RCode::NoError) { + if (ds.empty()) { + if (gotCNAME || denialProvesNoDelegation(zone, dsrecords)) { + /* we are still inside the same zone */ + if (foundCut) { *foundCut = false; - return Secure; } + return state; + } + /* delegation with no DS, might be Secure -> Insecure */ + if (foundCut) { *foundCut = true; } - return Insecure; - } else if (foundCut && rcode == RCode::NoError && !ds.empty()) { - *foundCut = true; + return state == Secure ? Insecure : state; + } else { + /* we have a DS */ + if (foundCut) { + *foundCut = true; + } } } @@ -2033,6 +2038,11 @@ vState SyncRes::validateRecordsWithSigs(unsigned int depth, const DNSName& qname if (!signer.empty() && name.isPartOf(signer)) { if ((qtype == QType::DNSKEY || qtype == QType::DS) && signer == qname) { /* we are already retrieving those keys, sorry */ + if (qtype == QType::DS) { + /* something is very wrong */ + LOG(d_prefix<<"The DS for "<