On 07/02/2009 09:11 AM, Alex Rousskov wrote:
> On 06/26/2009 11:00 PM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> Hello,
>>>
>>> Squid detects forwarding loops in most configurations, but breaks
>>> them (using a customizable HTTP_FORBIDDEN response) only when working as
>>> an accelerator. Squid does not break loops when working as a transparent
>>> proxy. Interestingly enough, the breaking code comment (in the patch
>>> below) says that both cases are covered. Perhaps the exclusion of
>>> transparent mode was not done on purpose.
>>>
>>> I understand that forwarding loops in transparent environment are
>>> usually caused by misconfiguration. However, when an admin is unable to
>>> fix the problem promptly, should not we help her by breaking the loop?
>>>
>>> Please note that a persistent loop is going to be broken anyway, when
>>> the Via and X-Forwarded-For headers exceed header size limit, but that
>>> wastes a lot of resources and may also crash Squid.
>>>
>>> Should we break forwarding loops in transparent mode?
>>
>> Yes.
>
> Committed to Squid3 trunk (r9782). Please commit to v3.1.
>> Keep in mind the fact that any crash during header processing opens the
>> doorway for DDoS sooner rather than later. I would go so far as to
>> suggest making it abort cleanly on mode. Not just these two.
>
> I agree, but wonder what the original rationale was behind the decision
> to limit loop breaking to accelerator setups? Does anybody know?
We hit another flavor of the same bug with Squid 3.2: either the complex
network setup allows loops for forwarding ports or the flags.transparent
test is failing because we do not set that request flag for transparent
ports if certain transparent redirection actions fail.
Does anybody know why the original code did not break all forwarding
loops, including loops for regular forwarding proxies?
Does anybody object to breaking all loops as the attached patch does?
Thank you,
Alex.
>> Might be time for a configuration error page to accompany this change as
>> well. Not just access denied. The wiki search queries are growing for
>> people looking for "access denied" message then some peer or
>> interception config option.
>
> Yeah. It would be nice to have a generic mechanism where admins can
> specify an error page to use for virtually all error contexts. The core
> code will only provide a unique context ID, and there will be default
> context_ID:error_page mapping that can be customized in squid.conf. This
> way, it would be much easier to customize error pages without the code
> bloat.
>
> Thank you,
>
> Alex.
>
>>> === modified file 'src/client_side_reply.cc'
>>> --- src/client_side_reply.cc 2009-04-10 09:23:50 +0000
>>> +++ src/client_side_reply.cc 2009-06-24 20:33:37 +0000
>>> @@ -640,7 +640,8 @@
>>> /*
>>> * Deny loops when running in accelerator/transproxy mode.
>>> */
>>> - if (http->flags.accel && r->flags.loopdetect) {
>>> + if (r->flags.loopdetect &&
>>> + (http->flags.accel || http->flags.transparent)) {
>>> http->al.http.code = HTTP_FORBIDDEN;
>>> err =
>>> clientBuildError(ERR_ACCESS_DENIED, HTTP_FORBIDDEN, NULL,
>>>
>>
>> Amos
Break all detected forwarding loops.
The old code tried to avoid breaking loops for non-intercepted/accelerated
requests (nobody on squid-dev could explain why). Unfortunately, the "is this
an intercepted request?" question cannot be reliably answered by examining
http->flags. We would have to check the port settings because sometimes the
port is intercepting, but we do not flip the corresponding flag in the request
(I do not know why).
If breaking all loops does not cause problems in valid setups, then it is a
better option that trying to figure out exactly what loops are safe to break.
This change will break setups that use non-unique proxy names in Via headers.
On the other hand, such setups can be considered invalid anyway, and the issue
should not be tied to whether this Squid uses interception or direct
forwarding. In other words, they were just lucky to work before this change.
=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc 2010-11-03 15:48:09 +0000
+++ src/client_side_reply.cc 2011-01-07 18:36:43 +0000
@@ -620,9 +620,8 @@
return;
}
- /// Deny loops for accelerator and interceptor. TODO: deny in all modes?
- if (r->flags.loopdetect &&
- (http->flags.accel || http->flags.intercepted)) {
+ // Deny all detected loops because we cannot reliably identify benign ones.
+ if (r->flags.loopdetect) {
http->al.http.code = HTTP_FORBIDDEN;
err = clientBuildError(ERR_ACCESS_DENIED, HTTP_FORBIDDEN, NULL, http->getConn()->peer, http->request);
createStoreEntry(r->method, request_flags());
