From 9f7136105bff920413042a8806cc5de3f6086d6d Mon Sep 17 00:00:00 2001

From: Thomas Leroy <32497783+p4zuu@users.noreply.github.com>

Date: Tue, 28 Nov 2023 07:35:46 +0000

Subject: [PATCH] Limit the number of allowed X-Forwarded-For hops (#1589)

Squid will ignore all X-Forwarded-For elements listed after the first 64

addresses allowed by the follow_x_forwarded_for directive. A different

limit can be specified by defining a C++ SQUID_X_FORWARDED_FOR_HOP_MAX

macro, but that macro is not a supported Squid configuration interface

and may change or disappear at any time.

Squid will log a cache.log ERROR if the hop limit has been reached.

This change works around problematic ACLChecklist and/or slow ACLs

implementation that results in immediate nonBlockingCheck() callbacks.

Such callbacks have caused many bugs and development complications. In

clientFollowXForwardedForCheck() context, they lead to indirect

recursion that was bound only by the number of allowed XFF entries,

which could reach thousands and exhaust Squid process call stack.

This recursion bug was discovered and detailed by Joshua Rogers at

https://megamansec.github.io/Squid-Security-Audit/xff-stackoverflow.html

where it was filed as "X-Forwarded-For Stack Overflow".

---

 src/ClientRequestContext.h |  4 ++++

 src/client_side_request.cc | 17 +++++++++++++++--

 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h

index 7dde9bc..775e308 100644

--- a/src/ClientRequestContext.h

+++ b/src/ClientRequestContext.h

@@ -90,6 +90,10 @@ public:

     ErrorState *error; ///< saved error page for centralized/delayed processing

     bool readNextRequest; ///< whether Squid should read after error handling

+#if FOLLOW_X_FORWARDED_FOR

+    size_t currentXffHopNumber = 0; ///< number of X-Forwarded-For header values processed so far

+#endif

+

 private:

     CBDATA_CLASS2(ClientRequestContext);

 };

diff --git a/src/client_side_request.cc b/src/client_side_request.cc

index 57d8975..9b77271 100644

--- a/src/client_side_request.cc

+++ b/src/client_side_request.cc

@@ -362,6 +362,11 @@ clientBeginRequest(const HttpRequestMethod& method, char const *url, CSCB * stre

     request->client_addr.setNoAddr();

 #if FOLLOW_X_FORWARDED_FOR

+

+#if !defined(SQUID_X_FORWARDED_FOR_HOP_MAX)

+#define SQUID_X_FORWARDED_FOR_HOP_MAX 64

+#endif

+

     request->indirect_client_addr.setNoAddr();

 #endif /* FOLLOW_X_FORWARDED_FOR */

@@ -475,8 +480,15 @@ clientFollowXForwardedForCheck(allow_t answer, void *data)

                 /* override the default src_addr tested if we have to go deeper than one level into XFF */

                 Filled(calloutContext->acl_checklist)->src_addr = request->indirect_client_addr;

             }

-            calloutContext->acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, data);

-            return;

+            if (++calloutContext->currentXffHopNumber < SQUID_X_FORWARDED_FOR_HOP_MAX) {

+                calloutContext->acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, data);

+                return;

+            }

+            debugs(28, DBG_CRITICAL, "ERROR: Ignoring trailing X-Forwarded-For addresses" <<

+                   Debug::Extra << "addresses allowed by follow_x_forwarded_for: " << calloutContext->currentXffHopNumber <<

+                   Debug::Extra << "last/accepted address: " << request->indirect_client_addr <<

+                   Debug::Extra << "ignored trailing addresses: " << request->x_forwarded_for_iterator);

+            // fall through to resume clientAccessCheck() processing

         }

     } /*if (answer == ACCESS_ALLOWED &&

         request->x_forwarded_for_iterator.size () != 0)*/

-- 

2.43.0