File bsc1217654.patch of Package squid.34512

Overview Repositories Revisions Requests Users Attributes Meta

File bsc1217654.patch of Package squid.34512

commit 45b6522eb80a6d12f75630fe1c132b52fc3f1624
Author: Thomas Leroy <32497783+p4zuu@users.noreply.github.com>
Date:   Tue Nov 28 07:35:46 2023 +0000

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

Index: squid-5.7/src/ClientRequestContext.h
===================================================================
--- squid-5.7.orig/src/ClientRequestContext.h
+++ squid-5.7/src/ClientRequestContext.h
@@ -80,6 +80,10 @@ public:
 #endif
     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
 };
 
 #endif /* SQUID_CLIENTREQUESTCONTEXT_H */
Index: squid-5.7/src/client_side_request.cc
===================================================================
--- squid-5.7.orig/src/client_side_request.cc
+++ squid-5.7/src/client_side_request.cc
@@ -80,6 +80,11 @@
 static const char *const crlf = "\r\n";
 
 #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 */
 
@@ -485,8 +490,16 @@ clientFollowXForwardedForCheck(Acl::Answ
                 /* 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
         }
     }

Places

File bsc1217654.patch of Package squid.34512

Places