Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rec: Fix validation of denial proofs #5868

Merged
merged 2 commits into from Oct 27, 2017
Merged

rec: Fix validation of denial proofs #5868

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b061cf
rec: Fix validation of denial proofs
rgacogne Oct 27, 2017
097f8a4
Fix case-sensitive comparison in DNSName::getCommonLabels()
rgacogne Oct 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions pdns/dnsname.cc
View file
Open in desktop
Expand Up @@ -253,6 +253,24 @@ void DNSName::makeUsRelative(const DNSName& zone)
clear();
}

DNSName DNSName::getCommonLabels(const DNSName& other) const
{
DNSName result;

const std::vector<std::string> ours = getRawLabels();
const std::vector<std::string> others = other.getRawLabels();

for (size_t pos = 0; ours.size() > pos && others.size() > pos; pos++) {
if (ours.at(ours.size() - pos - 1) != others.at(others.size() - pos - 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compare is case sensitive

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks! I just pushed a fix.

break;
}

result.prependRawLabel(ours.at(ours.size() - pos - 1));
}

return result;
}

DNSName DNSName::labelReverse() const
{
DNSName ret;
Expand Down
1 change: 1 addition & 0 deletions pdns/dnsname.hh
View file
Open in desktop
Expand Up @@ -94,6 +94,7 @@ public:
}
}
void makeUsRelative(const DNSName& zone);
DNSName getCommonLabels(const DNSName& other) const; //!< Return the list of common labels from the top, for example 'c.d' for 'a.b.c.d' and 'x.y.c.d'
DNSName labelReverse() const;
bool isWildcard() const;
bool isHostname() const;
Expand Down
635 changes: 620 additions & 15 deletions pdns/recursordist/test-syncres_cc.cc
View file
Open in desktop

Large diffs are not rendered by default.

43 changes: 39 additions & 4 deletions pdns/syncres.cc
View file
Open in desktop
Expand Up @@ -1821,6 +1821,9 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
isCNAMEAnswer = true;
}

/* if we have a positive answer synthetized from a wildcard,
we need to store the corresponding NSEC/NSEC3 records proving
that the exact name did not exist in the negative cache */
if(needWildcardProof) {
if (nsecTypes.count(rec.d_type)) {
authorityRecs.push_back(std::make_shared<DNSRecord>(rec));
Expand All @@ -1837,10 +1840,10 @@ RCode::rcodes_ SyncRes::updateCacheFromRecords(unsigned int depth, LWResult& lwr
if (rrsig) {
/* As illustrated in rfc4035's Appendix B.6, the RRSIG label
count can be lower than the name's label count if it was
synthesized from the wildcard. Note that the difference might
synthetized from the wildcard. Note that the difference might
be > 1. */
if (rec.d_name == qname && rrsig->d_labels < labelCount) {
LOG(prefix<<qname<<": RRSIG indicates the name was expanded from a wildcard, we need a wildcard proof"<<endl);
LOG(prefix<<qname<<": RRSIG indicates the name was synthetized from a wildcard, we need a wildcard proof"<<endl);
needWildcardProof = true;
}

Expand Down Expand Up @@ -2049,7 +2052,9 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
NegCache::NegCacheEntry ne;

uint32_t lowestTTL = rec.d_ttl;
ne.d_name = qname;
/* if we get an NXDomain answer with a CNAME, the name
does exist but the target does not */
ne.d_name = newtarget.empty() ? qname : newtarget;
ne.d_qtype = QType(0); // this encodes 'whole record'
ne.d_auth = rec.d_name;
harvestNXRecords(lwr.d_records, ne, d_now.tv_sec, &lowestTTL);
Expand All @@ -2063,7 +2068,12 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
ne.d_validationState = state;
}

if(!wasVariable()) {
/* if we get an NXDomain answer with a CNAME, let's not cache the
target, even the server was authoritative for it,
and do an additional query for the CNAME target.
We have a regression test making sure we do exactly that.
*/
if(!wasVariable() && newtarget.empty()) {
t_sstorage.negcache.add(ne);
if(s_rootNXTrust && ne.d_auth.isRoot() && auth.isRoot()) {
ne.d_name = ne.d_name.getLastLabel();
Expand All @@ -2079,6 +2089,9 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co
newtarget=content->getTarget();
}
}
/* if we have a positive answer synthetized from a wildcard, we need to
return the corresponding NSEC/NSEC3 records from the AUTHORITY section
proving that the exact name did not exist */
else if(needWildcardProof && (rec.d_type==QType::RRSIG || rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3) && rec.d_place==DNSResourceRecord::AUTHORITY) {
ret.push_back(rec); // enjoy your DNSSEC
}
Expand All @@ -2093,6 +2106,28 @@ bool SyncRes::processRecords(const std::string& prefix, const DNSName& qname, co

done=true;
ret.push_back(rec);

if (state == Secure && needWildcardProof) {
/* We have a positive answer synthetized from a wildcard, we need to check that we have
proof that the exact name doesn't exist so the wildcard can be used,
as described in section 5.3.4 of RFC 4035 and 5.3 of FRC 7129.
*/
NegCache::NegCacheEntry ne;

uint32_t lowestTTL = rec.d_ttl;
ne.d_name = qname;
ne.d_qtype = QType(0); // this encodes 'whole record'
harvestNXRecords(lwr.d_records, ne, d_now.tv_sec, &lowestTTL);

cspmap_t csp = harvestCSPFromNE(ne);
dState res = getDenial(csp, qname, ne.d_qtype.getCode(), false, false, false);
if (res != NXDOMAIN) {
LOG(d_prefix<<"Invalid denial in wildcard expanded positive response found for "<<qname<<", returning Bogus, res="<<res<<endl);
updateValidationState(state, Bogus);
/* we already store the record with a different validation status, let's fix it */
t_RC->updateValidationStatus(d_now.tv_sec, qname, qtype, d_incomingECSFound ? d_incomingECSNetwork : d_requestor, lwr.d_aabit, Bogus);
}
}
}
else if((rec.d_type==QType::RRSIG || rec.d_type==QType::NSEC || rec.d_type==QType::NSEC3) && rec.d_place==DNSResourceRecord::ANSWER) {
if(rec.d_type != QType::RRSIG || rec.d_name == qname)
Expand Down
18 changes: 18 additions & 0 deletions pdns/test-dnsname_cc.cc
View file
Open in desktop
Expand Up @@ -868,4 +868,22 @@ BOOST_AUTO_TEST_CASE(test_getlastlabel) {
// Check if the last label is indeed returned
BOOST_CHECK_EQUAL(ans, DNSName("com"));
}

BOOST_AUTO_TEST_CASE(test_getcommonlabels) {
const DNSName name1("www.powerdns.com");
const DNSName name2("a.long.list.of.labels.powerdns.com");

BOOST_CHECK_EQUAL(name1.getCommonLabels(name1), name1);
BOOST_CHECK_EQUAL(name2.getCommonLabels(name2), name2);

BOOST_CHECK_EQUAL(name1.getCommonLabels(name2), DNSName("powerdns.com"));
BOOST_CHECK_EQUAL(name2.getCommonLabels(name1), DNSName("powerdns.com"));

const DNSName name3("www.powerdns.org");
BOOST_CHECK_EQUAL(name1.getCommonLabels(name3), DNSName());
BOOST_CHECK_EQUAL(name2.getCommonLabels(name3), DNSName());
BOOST_CHECK_EQUAL(name3.getCommonLabels(name1), DNSName());
BOOST_CHECK_EQUAL(name3.getCommonLabels(name2), DNSName());
}

BOOST_AUTO_TEST_SUITE_END()