From 0e809f7ec9796cae0e3cc0b6e7407083a22cc157 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Fri, 29 Mar 2024 14:12:29 +0100
Subject: [PATCH] dnsdist: Fix a null-deref in incoming DoH w/ nghttp2

When an incoming DoH connection using the `nghttp2` provider is waiting
for a response from a backend that results in a I/O error or timeout,
and the incoming connection also fails due to a I/O error or timeout,
dnsdist could in some cases try to dereference a null pointer, leading
to a crash.
---
 pdns/dnsdistdist/dnsdist-nghttp2-in.cc        | 40 ++++++++++---------
 pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc | 31 ++++++++++++++
 2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
index f3f622b152..263631b065 100644
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
@@ -1099,7 +1099,9 @@ void IncomingHTTP2Connection::handleWritableIOCallback([[maybe_unused]] int desc
 
 void IncomingHTTP2Connection::stopIO()
 {
-  d_ioState->reset();
+  if (d_ioState) {
+    d_ioState->reset();
+  }
 }
 
 uint32_t IncomingHTTP2Connection::getConcurrentStreamsCount() const
@@ -1135,27 +1137,27 @@ void IncomingHTTP2Connection::updateIO(IOState newState, const FDMultiplexer::ca
   boost::optional<struct timeval> ttd{boost::none};
 
   auto shared = std::dynamic_pointer_cast<IncomingHTTP2Connection>(shared_from_this());
-  if (shared) {
-    struct timeval now
-    {
-    };
-    gettimeofday(&now, nullptr);
+  if (!shared || !d_ioState) {
+    return;
+  }
 
-    if (newState == IOState::NeedRead) {
-      /* use the idle TTL if the handshake has been completed (and proxy protocol payload received, if any),
-         and we have processed at least one query, otherwise we use the shorter read TTL  */
-      if ((d_state == State::waitingForQuery || d_state == State::idle) && (d_queriesCount > 0 || d_currentQueriesCount > 0)) {
-        ttd = getIdleClientReadTTD(now);
-      }
-      else {
-        ttd = getClientReadTTD(now);
-      }
-      d_ioState->update(newState, callback, shared, ttd);
+  timeval now{};
+  gettimeofday(&now, nullptr);
+
+  if (newState == IOState::NeedRead) {
+    /* use the idle TTL if the handshake has been completed (and proxy protocol payload received, if any),
+       and we have processed at least one query, otherwise we use the shorter read TTL  */
+    if ((d_state == State::waitingForQuery || d_state == State::idle) && (d_queriesCount > 0 || d_currentQueriesCount > 0)) {
+      ttd = getIdleClientReadTTD(now);
     }
-    else if (newState == IOState::NeedWrite) {
-      ttd = getClientWriteTTD(now);
-      d_ioState->update(newState, callback, shared, ttd);
+    else {
+      ttd = getClientReadTTD(now);
     }
+    d_ioState->update(newState, callback, shared, ttd);
+  }
+  else if (newState == IOState::NeedWrite) {
+    ttd = getClientWriteTTD(now);
+    d_ioState->update(newState, callback, shared, ttd);
   }
 }
 
diff --git a/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc b/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc
index 968e6c2d7e..c3f555a225 100644
--- a/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc
+++ b/pdns/dnsdistdist/test-dnsdistnghttp2-in_cc.cc
@@ -738,5 +738,36 @@ BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_BackendTimeout, TestFixture)
   }
 }
 
+BOOST_FIXTURE_TEST_CASE(test_IncomingConnection_ClientTimeout_BackendTimeout, TestFixture)
+{
+  auto local = getBackendAddress("1", 80);
+  ClientState localCS(local, true, false, 0, "", {}, true);
+  localCS.dohFrontend = std::make_shared<DOHFrontend>(std::make_shared<MockupTLSCtx>());
+  localCS.dohFrontend->d_urls.insert("/dns-query");
+
+  TCPClientThreadData threadData;
+  threadData.mplexer = std::make_unique<MockupFDMultiplexer>();
+
+  auto backend = std::make_shared<DownstreamState>(getBackendAddress("42", 53));
+
+  timeval now{};
+  gettimeofday(&now, nullptr);
+
+  size_t counter = 0;
+  s_connectionContexts[counter++] = ExpectedData{{}, {}, {}, {}};
+  s_steps = {
+    {ExpectedStep::ExpectedRequest::handshakeClient, IOState::Done},
+    /* write to client, but the client closes the connection */
+    {ExpectedStep::ExpectedRequest::writeToClient, IOState::Done, 0},
+    /* server close */
+    {ExpectedStep::ExpectedRequest::closeClient, IOState::Done},
+  };
+
+  auto state = std::make_shared<IncomingHTTP2Connection>(ConnectionInfo(&localCS, getBackendAddress("84", 4242)), threadData, now);
+  auto base = std::static_pointer_cast<IncomingTCPConnectionState>(state);
+  IncomingHTTP2Connection::handleTimeout(base, true);
+  state->handleIO();
+}
+
 BOOST_AUTO_TEST_SUITE_END();
 #endif /* HAVE_DNS_OVER_HTTPS && HAVE_NGHTTP2 */
-- 
2.47.2