From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 3 Jul 2023 12:06:05 +0000 (+0200)
Subject: dnsdist: Fix a crash when X-Forwarded-For overrides the initial source IP
X-Git-Tag: rec-5.0.0-alpha1~120^2~1
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9b703b51ca25838eeec19449a1c49cb926aef52a;p=thirdparty%2Fpdns.git

dnsdist: Fix a crash when X-Forwarded-For overrides the initial source IP

When both the processing of X-Forwarded-For DNS-over-https headers
(`trustForwardedForHeader=true`) and a maximum number of concurrent
TCP connections per client (`setMaxTCPConnectionsPerClient()`) are
enabled, dnsdist could crash because of an uncaught exception:
```
dnsdist[X]: terminate called after throwing an instance of 'std::runtime_error'
dnsdist[X]:   what():  DOH thread failed to launch: map::at
```
This was caused by the TCP connection being first accounted for with the
initial source IP (from the upstream HTTP proxy) but later released using
the IP extracted from the X-Forwarded-For header, leading to an unexpected
failure to locate the corresponding entry in the map.

We might not actually want to enforce the maximum number of concurrent
TCP connections per client when X-Forwarded-For processing is enabled,
though, because we usually want to rate limit the actual client and
not the HTTP proxy, but X-Forwarded-For being set per HTTP query, instead
of per-connection, makes that pretty much impossible at our level since
the same connection from the HTTP proxy can be reused for several clients.
The proxy protocol would be a better option to enforce that limit.
---

diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc
index 4959039ffe..c88cb11130 100644
--- a/pdns/dnsdistdist/doh.cc
+++ b/pdns/dnsdistdist/doh.cc
@@ -909,7 +909,7 @@ static bool getHTTPHeaderValue(const h2o_req_t* req, const std::string& headerNa
 }
 
 /* can only be called from the main DoH thread */
-static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote)
+static std::optional<ComboAddress> processForwardedForHeader(const h2o_req_t* req, const ComboAddress& remote)
 {
   static const std::string headerName = "x-forwarded-for";
   std::string_view value;
@@ -927,8 +927,7 @@ static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote
           value = value.substr(pos);
         }
       }
-      auto newRemote = ComboAddress(std::string(value));
-      remote = newRemote;
+      return ComboAddress(std::string(value));
     }
     catch (const std::exception& e) {
       vinfolog("Invalid X-Forwarded-For header ('%s') received from %s : %s", std::string(value), remote.toStringWithPort(), e.what());
@@ -937,6 +936,8 @@ static void processForwardedForHeader(const h2o_req_t* req, ComboAddress& remote
       vinfolog("Invalid X-Forwarded-For header ('%s') received from %s : %s", std::string(value), remote.toStringWithPort(), e.reason);
     }
   }
+
+  return std::nullopt;
 }
 
 /*
@@ -971,14 +972,18 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
       h2o_socket_getsockname(sock, reinterpret_cast<struct sockaddr*>(&conn.d_local));
     }
 
+    auto remote = conn.d_remote;
     if (dsc->df->d_trustForwardedForHeader) {
-      processForwardedForHeader(req, conn.d_remote);
+      auto newRemote = processForwardedForHeader(req, remote);
+      if (newRemote) {
+        remote = std::move(*newRemote);
+      }
     }
 
     auto& holders = dsc->holders;
-    if (!holders.acl->match(conn.d_remote)) {
+    if (!holders.acl->match(remote)) {
       ++dnsdist::metrics::g_stats.aclDrops;
-      vinfolog("Query from %s (DoH) dropped because of ACL", conn.d_remote.toStringWithPort());
+      vinfolog("Query from %s (DoH) dropped because of ACL", remote.toStringWithPort());
       h2o_send_error_403(req, "Forbidden", "dns query not allowed because of ACL", 0);
       return 0;
     }
@@ -1034,7 +1039,7 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
       query.reserve(req->entity.len + maxAdditionalSizeForEDNS);
       query.resize(req->entity.len);
       memcpy(query.data(), req->entity.base, req->entity.len);
-      doh_dispatch_query(dsc, self, req, std::move(query), conn.d_local, conn.d_remote, std::move(path));
+      doh_dispatch_query(dsc, self, req, std::move(query), conn.d_local, remote, std::move(path));
     }
     else if(req->query_at != SIZE_MAX && (req->path.len - req->query_at > 5)) {
       auto pos = path.find("?dns=");
@@ -1073,7 +1078,7 @@ static int doh_handler(h2o_handler_t *self, h2o_req_t *req)
           else
             ++dsc->df->d_http1Stats.d_nbQueries;
 
-          doh_dispatch_query(dsc, self, req, std::move(decoded), conn.d_local, conn.d_remote, std::move(path));
+          doh_dispatch_query(dsc, self, req, std::move(decoded), conn.d_local, remote, std::move(path));
         }
       }
       else
diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py
index 61936de441..2dd016fd1b 100644
--- a/regression-tests.dnsdist/test_DOH.py
+++ b/regression-tests.dnsdist/test_DOH.py
@@ -1064,6 +1064,9 @@ class TestDOHForwardedFor(DNSDistDOHTest):
 
     setACL('192.0.2.1/32')
     addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/" }, {trustForwardedForHeader=true})
+    -- Set a maximum number of TCP connections per client, to exercise
+    -- that code along with X-Forwarded-For support
+    setMaxTCPConnectionsPerClient(2)
     """
     _config_params = ['_testServerPort', '_dohServerPort', '_serverCert', '_serverKey']