Hello,
Should the attached patch go in? It prevents assertions (or worse)
during forwarding loops or carefully crafted messages. Production-tested
in Squid 3.0, although I do not know whether the code was ever triggered
outside the lab.
This change also prevents most cases of pointless computation of the
original X-Forwarded-For value list in Squid 3.1. That computation can
be quite expensive.
Thank you,
Alex.
Limit X-Forwarded-For growth.
X-Forwarded-For growth leads to String size limit assertions and probably
other problems.
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 may also grow too large for reasons unrelated
to forwarding loops.
Ported from 3p0-plus (r8940..8941).
This change also prevents most cases of pointless computation of the original
X-Forwarded-For value list. That computation can be quite expensive.
=== modified file 'src/http.cc'
--- src/http.cc 2009-07-11 00:16:42 +0000
+++ src/http.cc 2009-07-11 01:07:15 +0000
@@ -1464,7 +1464,6 @@
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);
@@ -1514,24 +1513,37 @@
}
#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.
+
+ // XXX: the Right Thing is to catch this at a higher level and
+ // terminate this looping transaction or it may loop forever.
+ strFwd = "unknowns";
+
+ static int warnedCount = 0;
+ if (warnedCount++ < 100) {
+ const char *url = entry ? entry->url() : urlCanonical(orig_request);
+ debugs(11, 1, "Warning: Undetected forwarding loop for " << 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() )
@@ -1543,7 +1555,6 @@
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)) {
