From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Thu, 18 Apr 2024 08:49:10 +0000 (+0200)
Subject: dnsdist: Fix a crash in incoming DoH with nghttp2
X-Git-Tag: rec-5.1.0-alpha1~28^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a4d82c53f714211462ae4da2e16fb13dec51ba05;p=thirdparty%2Fpdns.git

dnsdist: Fix a crash in incoming DoH with nghttp2

This fixes an issue in the code dealing with incoming DNS over HTTPS
queries with the nghttp2 provider. In some rare cases, if the incoming
query is forwarded to the backend over TCP and the response comes back
immediately (the `read()` call done just after the `write()` call sending
the query must succeed and yield a complete response), the processing
of the response might end up calling `IncomingHTTP2Connection::readHTTPData()`
down the line, via the `nghttp2` callbacks, while we were already
inside this function. This does not actually work because
`nghttp2_session_mem_recv` is not reentrant, so the internal state of
the `nghttp2_session` object might become inconsistent and trigger
an assertion, for example:
```
nghttp2_session.c:6854: nghttp2_session_mem_recv2: Assertion `iframe->state == NGHTTP2_IB_IGN_ALL' failed.
```

This results in a call to `abort()` and very unlikely to be exploitable,
because there is no memory corruption occurring. It would also be quite
difficult for an attacker to trigger the conditions leading to this event
remotely.

Reported by Daniel Stirnimann from Switch and Stephane Bortzmeyer, many thanks to them.
---

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
index 341e402043..f88294168e 100644
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
@@ -330,6 +330,27 @@ IOState IncomingHTTP2Connection::handleHandshake(const struct timeval& now)
   return iostate;
 }
 
+class ReadFunctionGuard
+{
+public:
+  ReadFunctionGuard(bool& inReadFunction) :
+    d_inReadFunctionRef(inReadFunction)
+  {
+    d_inReadFunctionRef = true;
+  }
+  ReadFunctionGuard(ReadFunctionGuard&&) = delete;
+  ReadFunctionGuard(const ReadFunctionGuard&) = delete;
+  ReadFunctionGuard& operator=(ReadFunctionGuard&&) = delete;
+  ReadFunctionGuard& operator=(const ReadFunctionGuard&) = delete;
+  ~ReadFunctionGuard()
+  {
+    d_inReadFunctionRef = false;
+  }
+
+private:
+  bool& d_inReadFunctionRef;
+};
+
 void IncomingHTTP2Connection::handleIO()
 {
   IOState iostate = IOState::Done;
@@ -392,10 +413,10 @@ void IncomingHTTP2Connection::handleIO()
       }
     }
 
-    if (active() && !d_connectionClosing && (d_state == State::waitingForQuery || d_state == State::idle)) {
+    if (!d_inReadFunction && active() && !d_connectionClosing && (d_state == State::waitingForQuery || d_state == State::idle)) {
       do {
         iostate = readHTTPData();
-      } while (active() && !d_connectionClosing && iostate == IOState::Done);
+      } while (!d_inReadFunction && active() && !d_connectionClosing && iostate == IOState::Done);
     }
 
     if (!active()) {
@@ -1064,6 +1085,11 @@ int IncomingHTTP2Connection::on_error_callback(nghttp2_session* session, int lib
 
 IOState IncomingHTTP2Connection::readHTTPData()
 {
+  if (d_inReadFunction) {
+    return IOState::Done;
+  }
+  ReadFunctionGuard readGuard(d_inReadFunction);
+
   IOState newState = IOState::Done;
   size_t got = 0;
   if (d_in.size() < s_initialReceiveBufferSize) {
diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.hh b/pdns/dnsdistdist/dnsdist-nghttp2-in.hh
index a2e58a45a9..e63077882c 100644
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.hh
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.hh
@@ -116,6 +116,10 @@ private:
   /* Whether we have data that we want to write to the socket,
      but the socket is full. */
   bool d_pendingWrite{false};
+  /* Whether we are currently inside the readHTTPData function,
+     which is not reentrant and could be called from itself via
+     the nghttp2 callbacks */
+  bool d_inReadFunction{false};
 };
 
 class NGHTTP2Headers