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 341e4020435e75deb4b76aace0fd80c4778d7208..f88294168e72d8c58c686793df9d8a0be4360682 100644 (file)
--- 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 a2e58a45a919bd6bc69569e8974024c2f7249dba..e63077882c39288d856aea4c02c65025e1f45aeb 100644 (file)
--- 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