From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 23 Sep 2020 08:02:15 +0000 (+0200)
Subject: dnsdist: Fix getEDNSOptions() for {AN,NS}COUNT != 0 and ARCOUNT = 0
X-Git-Tag: auth-4.4.0-alpha1~14^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=38af359d79bccc500deaa598957a1b0d1ce11fd4;p=thirdparty%2Fpdns.git

dnsdist: Fix getEDNSOptions() for {AN,NS}COUNT != 0 and ARCOUNT = 0

Since 1.5.0, calling getEDNSOptions() from Lua would result in a
ServFail for queries that had no records in additional but at least
one record in either the answer or authority section, such as a
NOTIFY, because of a bug in parseEDNSOptions(). That last function
incorrectly called slowParseEDNSOptions() in that case, triggering
an exception to be raised because slowParseEDNSOptions() does not
expect to be called for a packet with no record in the additional
section.
parseEDNSOptions() now returns `false` for packets that have no
record in the additional section.
---

diff --git a/pdns/dnsdist-ecs.cc b/pdns/dnsdist-ecs.cc
index 6dfdbbc8f2..30659cd708 100644
--- a/pdns/dnsdist-ecs.cc
+++ b/pdns/dnsdist-ecs.cc
@@ -505,7 +505,12 @@ bool parseEDNSOptions(DNSQuestion& dq)
 
   dq.ednsOptions = std::make_shared<std::map<uint16_t, EDNSOptionView> >();
 
-  if (ntohs(dq.dh->ancount) != 0 || ntohs(dq.dh->nscount) != 0 || (ntohs(dq.dh->arcount) != 0 && ntohs(dq.dh->arcount) != 1)) {
+  if (ntohs(dq.dh->arcount) == 0) {
+    /* nothing in additional so no EDNS */
+    return false;
+  }
+
+  if (ntohs(dq.dh->ancount) != 0 || ntohs(dq.dh->nscount) != 0 || ntohs(dq.dh->arcount) > 1) {
     return slowParseEDNSOptions(reinterpret_cast<const char*>(dq.dh), dq.len, dq.ednsOptions);
   }
 
diff --git a/pdns/test-dnsdist_cc.cc b/pdns/test-dnsdist_cc.cc
index c54c1a365a..e32522433d 100644
--- a/pdns/test-dnsdist_cc.cc
+++ b/pdns/test-dnsdist_cc.cc
@@ -1985,4 +1985,75 @@ BOOST_AUTO_TEST_CASE(test_setNegativeAndAdditionalSOA) {
   }
 }
 
+BOOST_AUTO_TEST_CASE(getEDNSOptionsWithoutEDNS) {
+  const ComboAddress remote("192.168.1.25");
+  const DNSName name("www.powerdns.com.");
+  const ComboAddress origRemote("127.0.0.1");
+  const ComboAddress v4("192.0.2.1");
+
+  {
+    /* no EDNS and no other additional record */
+    vector<uint8_t> query;
+    DNSPacketWriter pw(query, name, QType::A, QClass::IN, 0);
+    pw.getHeader()->rd = 1;
+    pw.commit();
+
+    /* large enough packet */
+    char packet[1500];
+    memcpy(packet, query.data(), query.size());
+
+    unsigned int consumed = 0;
+    uint16_t qtype;
+    uint16_t qclass;
+    DNSName qname(packet, query.size(), sizeof(dnsheader), false, &qtype, &qclass, &consumed);
+    DNSQuestion dq(&qname, qtype, qclass, consumed, nullptr, &remote, reinterpret_cast<dnsheader*>(packet), sizeof(packet), query.size(), false, nullptr);
+
+    BOOST_CHECK(!parseEDNSOptions(dq));
+  }
+
+  {
+    /* nothing in additional (so no EDNS) but a record in ANSWER */
+    vector<uint8_t> query;
+    DNSPacketWriter pw(query, name, QType::A, QClass::IN, 0);
+    pw.getHeader()->rd = 1;
+    pw.startRecord(name, QType::A, 60, QClass::IN, DNSResourceRecord::ANSWER);
+    pw.xfrIP(v4.sin4.sin_addr.s_addr);
+    pw.commit();
+
+    /* large enough packet */
+    char packet[1500];
+    memcpy(packet, query.data(), query.size());
+
+    unsigned int consumed = 0;
+    uint16_t qtype;
+    uint16_t qclass;
+    DNSName qname(packet, query.size(), sizeof(dnsheader), false, &qtype, &qclass, &consumed);
+    DNSQuestion dq(&qname, qtype, qclass, consumed, nullptr, &remote, reinterpret_cast<dnsheader*>(packet), sizeof(packet), query.size(), false, nullptr);
+
+    BOOST_CHECK(!parseEDNSOptions(dq));
+  }
+
+  {
+    /* nothing in additional (so no EDNS) but a record in AUTHORITY */
+    vector<uint8_t> query;
+    DNSPacketWriter pw(query, name, QType::A, QClass::IN, 0);
+    pw.getHeader()->rd = 1;
+    pw.startRecord(name, QType::A, 60, QClass::IN, DNSResourceRecord::AUTHORITY);
+    pw.xfrIP(v4.sin4.sin_addr.s_addr);
+    pw.commit();
+
+    /* large enough packet */
+    char packet[1500];
+    memcpy(packet, query.data(), query.size());
+
+    unsigned int consumed = 0;
+    uint16_t qtype;
+    uint16_t qclass;
+    DNSName qname(packet, query.size(), sizeof(dnsheader), false, &qtype, &qclass, &consumed);
+    DNSQuestion dq(&qname, qtype, qclass, consumed, nullptr, &remote, reinterpret_cast<dnsheader*>(packet), sizeof(packet), query.size(), false, nullptr);
+
+    BOOST_CHECK(!parseEDNSOptions(dq));
+  }
+}
+
 BOOST_AUTO_TEST_SUITE_END();