Limit X-Forwarded-For growth.

X-Forwarded-For growth leads to String size limit assertions and probably
other problems.

We now replace huge XFF values with a string "error", warn the admin the
first 100 times, and hope that something will stop the loop (if it is a
loop). TODO: we should probably deny requests with huge XFF.

To make growth-associated problems visible during forwarding loops, the
loop breaking code must be disabled (no Via) or not applicable (direct
forwarding) and request_header_max_size has to be raised or disabled.
The X-Forwarded-For header value may also grow too large for reasons
unrelated to forwarding loops.

This change also prevents most cases of pointless computation of the
original X-Forwarded-For value list. That computation can be quite
expensive.

diff --git a/src/http.cc b/src/http.cc
index fa2ecd29e8c34390727dfc24c14a73a12f92c472..49d25fd350ed1e1d6db01f9f4313395d3d1fff6f 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1451,7 +1451,6 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
     LOCAL_ARRAY(char, ntoabuf, MAX_IPSTRLEN);
     const HttpHeader *hdr_in = &orig_request->header;
     const HttpHeaderEntry *e = NULL;
-    String strFwd;
     HttpHeaderPos pos = HttpHeaderInitPos;
     assert (hdr_out->owner == hoRequest);
 
@@ -1501,24 +1500,35 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
     }
 #endif
 
-    strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
-
     /** \pre Handle X-Forwarded-For */
     if (strcmp(opt_forwarded_for, "delete") != 0) {
+
+        String strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
+
+        if (strFwd.size() > 65536/2) {
+            // There is probably a forwarding loop with Via detection disabled.
+            // If we do nothing, String will assert on overflow soon.
+            // TODO: Terminate all transactions with huge XFF?
+            strFwd = "error";
+
+            static int warnedCount = 0;
+            if (warnedCount++ < 100) {
+                const char *url = entry ? entry->url() : urlCanonical(orig_request);
+                debugs(11, 1, "Warning: likely forwarding loop with " << url);
+            }
+        }
+
         if (strcmp(opt_forwarded_for, "on") == 0) {
             /** If set to ON - append client IP or 'unknown'. */
-            strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
             if ( orig_request->client_addr.IsNoAddr() )
                 strListAdd(&strFwd, "unknown", ',');
             else
                 strListAdd(&strFwd, orig_request->client_addr.NtoA(ntoabuf, MAX_IPSTRLEN), ',');
         } else if (strcmp(opt_forwarded_for, "off") == 0) {
             /** If set to OFF - append 'unknown'. */
-            strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
             strListAdd(&strFwd, "unknown", ',');
         } else if (strcmp(opt_forwarded_for, "transparent") == 0) {
             /** If set to TRANSPARENT - pass through unchanged. */
-            strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR);
         } else if (strcmp(opt_forwarded_for, "truncate") == 0) {
             /** If set to TRUNCATE - drop existing list and replace with client IP or 'unknown'. */
             if ( orig_request->client_addr.IsNoAddr() )
@@ -1530,7 +1540,6 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
             hdr_out->putStr(HDR_X_FORWARDED_FOR, strFwd.termedBuf());
     }
     /** If set to DELETE - do not copy through. */
-    strFwd.clean();
 
     /* append Host if not there already */
     if (!hdr_out->has(HDR_HOST)) {