dnsdist: Fix handling of XFR requests over DoH

We did not properly handle incoming XFR requests received over DoH
When a TCP-only or DoT backend was configured, and the nghttp2 provider
used.
This commits fixes the assertion failure and makes sure that XFR
requests are denied with `NOTIMP` when received over DNS over HTTPS,
including DNS over HTTP/3. It also denies them when received over
DNS over QUIC as this is not properly handled at the moment, although
it does not cause a crash.

diff --git a/pdns/dnsdistdist/dnsdist-idstate.cc b/pdns/dnsdistdist/dnsdist-idstate.cc
index 395ce3ccbae6b394033623c7c15028ac042a5b59..3217720a58b0028dc79f401c721b94fbb1a8cdf9 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-idstate.cc
+++ b/pdns/dnsdistdist/dnsdist-idstate.cc
@@ -50,5 +50,7 @@ InternalQueryState InternalQueryState::partialCloneForXFR() const
     ids.d_protoBufData = std::make_unique<InternalQueryState::ProtoBufData>(*d_protoBufData);
   }
   ids.cs = cs;
+  /* in case we want to support XFR over DoH, or the stream ID becomes used for QUIC */
+  ids.d_streamID = d_streamID;
   return ids;
 }

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
index 0e4e602a360c72bd8cdc45da5ff3a100ae1bac83..797def327e56104f3febed829418aa9703dbcc88 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
@@ -222,8 +222,10 @@ void IncomingHTTP2Connection::handleResponse(const struct timeval& now, TCPRespo
 
 std::unique_ptr<DOHUnitInterface> IncomingHTTP2Connection::getDOHUnit(uint32_t streamID)
 {
-  // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay): clang-tidy is getting confused by assert()
-  assert(streamID <= std::numeric_limits<IncomingHTTP2Connection::StreamID>::max());
+  if (streamID > std::numeric_limits<IncomingHTTP2Connection::StreamID>::max()) {
+    throw std::runtime_error("Invalid stream ID while retrieving DoH unit");
+  }
+
   // NOLINTNEXTLINE(*-narrowing-conversions): generic interface between DNS and DoH with different types
   auto query = std::move(d_currentStreams.at(static_cast<IncomingHTTP2Connection::StreamID>(streamID)));
   return std::make_unique<IncomingDoHCrossProtocolContext>(std::move(query), std::dynamic_pointer_cast<IncomingHTTP2Connection>(shared_from_this()), streamID);
@@ -551,8 +553,9 @@ void NGHTTP2Headers::addDynamicHeader(std::vector<nghttp2_nv>& headers, NGHTTP2H
 
 IOState IncomingHTTP2Connection::sendResponse(const struct timeval& now, TCPResponse&& response)
 {
-  // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay): clang-tidy is getting confused by assert()
-  assert(response.d_idstate.d_streamID != -1);
+  if (response.d_idstate.d_streamID == -1) {
+    throw std::runtime_error("Invalid DoH stream ID while sending response");
+  }
   auto& context = d_currentStreams.at(response.d_idstate.d_streamID);
 
   uint32_t statusCode = 200U;
@@ -584,8 +587,10 @@ void IncomingHTTP2Connection::notifyIOError(const struct timeval& now, TCPRespon
     return;
   }
 
-  // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay): clang-tidy is getting confused by assert()
-  assert(response.d_idstate.d_streamID != -1);
+  if (response.d_idstate.d_streamID == -1) {
+    throw std::runtime_error("Invalid DoH stream ID while handling I/O error notification");
+  }
+
   auto& context = d_currentStreams.at(response.d_idstate.d_streamID);
   context.d_buffer = std::move(response.d_buffer);
   sendResponse(response.d_idstate.d_streamID, context, 502, d_ci.cs->dohFrontend->d_customResponseHeaders);

diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc
index 954450202fe052a1767df2946205b23c325e9313..d411b0b66ce63360570a2827882564f1e24a12d3 100644 (file)
--- a/pdns/dnsdistdist/dnsdist.cc
+++ b/pdns/dnsdistdist/dnsdist.cc
@@ -1685,6 +1685,15 @@ ProcessQueryResult processQuery(DNSQuestion& dnsQuestion, LocalHolders& holders,
     timespec now{};
     gettime(&now);
 
+    if ((dnsQuestion.ids.qtype == QType::AXFR || dnsQuestion.ids.qtype == QType::IXFR) && (dnsQuestion.getProtocol() == dnsdist::Protocol::DoH || dnsQuestion.getProtocol() == dnsdist::Protocol::DoQ || dnsQuestion.getProtocol() == dnsdist::Protocol::DoH3)) {
+      dnsQuestion.editHeader([](dnsheader& header) {
+        header.rcode = RCode::NotImp;
+        header.qr = true;
+        return true;
+      });
+      return processQueryAfterRules(dnsQuestion, holders, selectedBackend);
+    }
+
     if (!applyRulesToQuery(holders, dnsQuestion, now)) {
       return ProcessQueryResult::Drop;
     }

diff --git a/regression-tests.dnsdist/quictests.py b/regression-tests.dnsdist/quictests.py
index 743de28db561eab5374d23f7ff6dd08af8e3ebb5..b4ec180722869ed750c2d9f5c6a3e7533b06f409 100644 (file)
--- a/regression-tests.dnsdist/quictests.py
+++ b/regression-tests.dnsdist/quictests.py
@@ -191,3 +191,18 @@ class QUICGetLocalAddressOnAnyBindTests(object):
 
         (_, receivedResponse) = self.sendQUICQuery(query, response=None, useQueue=False)
         self.assertEqual(receivedResponse, response)
+
+class QUICXFRTests(object):
+
+    def testXFR(self):
+        """
+        QUIC: XFR
+        """
+        name = 'xfr.doq.tests.powerdns.com.'
+        for xfrType in [dns.rdatatype.AXFR, dns.rdatatype.IXFR]:
+            query = dns.message.make_query(name, xfrType, 'IN')
+            expectedResponse = dns.message.make_response(query)
+            expectedResponse.set_rcode(dns.rcode.NOTIMP)
+
+            (_, receivedResponse) = self.sendQUICQuery(query, response=None, useQueue=False)
+            self.assertEqual(receivedResponse, expectedResponse)

diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py
index f1d7912e31ffcfd279ed0c67af3f337223b0a4e9..562795402d40e5da7c70fa881fa54df0a05090eb 100644 (file)
--- a/regression-tests.dnsdist/test_DOH.py
+++ b/regression-tests.dnsdist/test_DOH.py
@@ -1712,3 +1712,39 @@ class TestDOHLimitsNGHTTP2(DOHLimits, DNSDistDOHTest):
 
 class TestDOHLimitsH2O(DOHLimits, DNSDistDOHTest):
     _dohLibrary = 'h2o'
+
+class DOHXFR(object):
+    _serverName = 'tls.tests.dnsdist.org'
+    _caCert = 'ca.pem'
+    _dohServerPort = pickAvailablePort()
+    _dohBaseURL = ("https://%s:%d/" % (_serverName, _dohServerPort))
+    _serverKey = 'server.key'
+    _serverCert = 'server.chain'
+    _maxTCPConnsPerClient = 3
+    _config_template = """
+    newServer{address="127.0.0.1:%d", tcpOnly=true}
+    addDOHLocal("127.0.0.1:%d", "%s", "%s", { "/" }, {library='%s'})
+    """
+    _config_params = ['_testServerPort', '_dohServerPort', '_serverCert', '_serverKey', '_dohLibrary']
+
+    def testXFR(self):
+        """
+        DoH XFR: Check that XFR requests over DoH are refused with NotImp
+        """
+        name = 'xfr.doh.tests.powerdns.com.'
+        for xfrType in [dns.rdatatype.AXFR, dns.rdatatype.IXFR]:
+            query = dns.message.make_query(name, xfrType, 'IN')
+            url = self.getDOHGetURL(self._dohBaseURL, query)
+
+            expectedResponse = dns.message.make_response(query)
+            expectedResponse.set_rcode(dns.rcode.NOTIMP)
+
+            (_, receivedResponse) = self.sendDOHQuery(self._dohServerPort, self._serverName, self._dohBaseURL, query, caFile=self._caCert, useQueue=False)
+
+            self.assertEqual(receivedResponse, expectedResponse)
+
+class TestDOHXFRNGHTTP2(DOHXFR, DNSDistDOHTest):
+    _dohLibrary = 'nghttp2'
+
+class TestDOHXFRH2O(DOHXFR, DNSDistDOHTest):
+    _dohLibrary = 'h2o'

diff --git a/regression-tests.dnsdist/test_DOH3.py b/regression-tests.dnsdist/test_DOH3.py
index 4a91c433f25ea962a6e451a3e998b92415681cd2..9634c914c7d8d2acecd7523abb15ad0aed9b3943 100644 (file)
--- a/regression-tests.dnsdist/test_DOH3.py
+++ b/regression-tests.dnsdist/test_DOH3.py
@@ -4,7 +4,7 @@ import clientsubnetoption
 
 from dnsdisttests import DNSDistTest
 from dnsdisttests import pickAvailablePort
-from quictests import QUICTests, QUICWithCacheTests, QUICACLTests, QUICGetLocalAddressOnAnyBindTests
+from quictests import QUICTests, QUICWithCacheTests, QUICACLTests, QUICGetLocalAddressOnAnyBindTests, QUICXFRTests
 import doh3client
 
 class TestDOH3(QUICTests, DNSDistTest):
@@ -122,3 +122,24 @@ class TestDOH3GetLocalAddressOnAnyBind(QUICGetLocalAddressOnAnyBindTests, DNSDis
 
     def sendQUICQuery(self, query, response=None, useQueue=True, connection=None):
         return self.sendDOH3Query(self._doqServerPort, self._dohBaseURL, query, response=response, caFile=self._caCert, useQueue=useQueue, serverName=self._serverName, connection=connection)
+
+class TestDOH3XFR(QUICXFRTests, DNSDistTest):
+    _serverKey = 'server.key'
+    _serverCert = 'server.chain'
+    _serverName = 'tls.tests.dnsdist.org'
+    _caCert = 'ca.pem'
+    _doqServerPort = pickAvailablePort()
+    _dohBaseURL = ("https://%s:%d/" % (_serverName, _doqServerPort))
+    _config_template = """
+    newServer{address="127.0.0.1:%d", tcpOnly=true}
+
+    addDOH3Local("127.0.0.1:%d", "%s", "%s")
+    """
+    _config_params = ['_testServerPort', '_doqServerPort','_serverCert', '_serverKey']
+    _verboseMode = True
+
+    def getQUICConnection(self):
+        return self.getDOQConnection(self._doqServerPort, self._caCert)
+
+    def sendQUICQuery(self, query, response=None, useQueue=True, connection=None):
+        return self.sendDOH3Query(self._doqServerPort, self._dohBaseURL, query, response=response, caFile=self._caCert, useQueue=useQueue, serverName=self._serverName, connection=connection)

diff --git a/regression-tests.dnsdist/test_DOQ.py b/regression-tests.dnsdist/test_DOQ.py
index 657df001a9931a9fa1b13e678364b5de9bf6c0f5..5a817747e04c2fef38cf41da730dda93b3ed8b6f 100644 (file)
--- a/regression-tests.dnsdist/test_DOQ.py
+++ b/regression-tests.dnsdist/test_DOQ.py
@@ -6,7 +6,7 @@ import clientsubnetoption
 from dnsdisttests import DNSDistTest
 from dnsdisttests import pickAvailablePort
 from doqclient import quic_bogus_query
-from quictests import QUICTests, QUICWithCacheTests, QUICACLTests, QUICGetLocalAddressOnAnyBindTests
+from quictests import QUICTests, QUICWithCacheTests, QUICACLTests, QUICGetLocalAddressOnAnyBindTests, QUICXFRTests
 import doqclient
 from doqclient import quic_query
 
@@ -105,6 +105,26 @@ class TestDOQWithACL(QUICACLTests, DNSDistTest):
     def sendQUICQuery(self, query, response=None, useQueue=True, connection=None):
         return self.sendDOQQuery(self._doqServerPort, query, response=response, caFile=self._caCert, useQueue=useQueue, serverName=self._serverName, connection=connection)
 
+class TestDOQXFR(QUICXFRTests, DNSDistTest):
+    _serverKey = 'server.key'
+    _serverCert = 'server.chain'
+    _serverName = 'tls.tests.dnsdist.org'
+    _caCert = 'ca.pem'
+    _doqServerPort = pickAvailablePort()
+    _config_template = """
+    newServer{address="127.0.0.1:%d", tcpOnly=True}
+
+    addDOQLocal("127.0.0.1:%d", "%s", "%s")
+    """
+    _config_params = ['_testServerPort', '_doqServerPort','_serverCert', '_serverKey']
+    _verboseMode = True
+
+    def getQUICConnection(self):
+        return self.getDOQConnection(self._doqServerPort, self._caCert)
+
+    def sendQUICQuery(self, query, response=None, useQueue=True, connection=None):
+        return self.sendDOQQuery(self._doqServerPort, query, response=response, caFile=self._caCert, useQueue=useQueue, serverName=self._serverName, connection=connection)
+
 class TestDOQCertificateReloading(DNSDistTest):
     _consoleKey = DNSDistTest.generateConsoleKey()
     _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii')