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".

diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h
index b4ad460dbfb82fd204e6f18ccf9f4926f00c85dc..4b2261352794f4ed0eeff05c964774c67c599df4 100644 (file)
--- a/src/ClientRequestContext.h
+++ b/src/ClientRequestContext.h
@@ -78,8 +78,13 @@ public:
 #if USE_OPENSSL
     bool sslBumpCheckDone = false;
 #endif
-    ErrorState *error = nullptr; ///< saved error page for centralized/delayed processing
+
     bool readNextRequest = false; ///< whether Squid should read after error handling
+    ErrorState *error = nullptr; ///< saved error page for centralized/delayed processing
+
+#if FOLLOW_X_FORWARDED_FOR
+    size_t currentXffHopNumber = 0; ///< number of X-Forwarded-For header values processed so far
+#endif
 };
 
 #endif /* SQUID_CLIENTREQUESTCONTEXT_H */

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 235dfad2d6df4a60808b47e5ed6d1740812b4fa7..15e556296e7a0ac0addc07bd6d501ffe2f0784a3 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -74,6 +74,11 @@
 #endif
 
 #if FOLLOW_X_FORWARDED_FOR
+
+#if !defined(SQUID_X_FORWARDED_FOR_HOP_MAX)
+#define SQUID_X_FORWARDED_FOR_HOP_MAX 64
+#endif
+
 static void clientFollowXForwardedForCheck(Acl::Answer answer, void *data);
 #endif /* FOLLOW_X_FORWARDED_FOR */
 
@@ -438,8 +443,16 @@ clientFollowXForwardedForCheck(Acl::Answer 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;
+            }
+            const auto headerName = Http::HeaderLookupTable.lookup(Http::HdrType::X_FORWARDED_FOR).name;
+            debugs(28, DBG_CRITICAL, "ERROR: Ignoring trailing " << headerName << " 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
         }
     }