Re: [RFC] Breaking forwarding loops

On 08/01/11 07:56, Alex Rousskov wrote:
> 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.

IFAIK this is a hangover of the old behaviour of allowing regular
requests to arrive and be handled by a transparent port or NAT lookup
failures to be ignored and treated as forward proxy.

To reduce CVE_2009-0801 effects it might be a good idea to prevent that
handling as a hard limit now in 3.2+. I've been pushing for the port
change at the user config end of things for over a year now, so its not
as big a backward compatibility issue.

>
> Does anybody know why the original code did not break all forwarding
> loops, including loops for regular forwarding proxies?

The unique_hostname / visible_hostname logics need a bit of
straightening to avoid the Via: breakage case. I have a patch somewhere
that attempted that. Will dig up for audit.

Note that a great many hostnames are "localhost" or
"localhost.localdomain" or "localhost.local" due to certain distros
hard-coding "localhost" into their packages.

We also use "localhost" as a backup when the gethostname() call fails to
provide anything with rDNS. (IMO that hard rDNS requirement is a bit naive)

>
> Does anybody object to breaking all loops as the attached patch does?
>

Sort of. The hostname mess needs to be resolved in any release using the
blanket loop detection. I'm flexible as to which fix goes into trunk
first though.

So +/-0 from me.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.10
   Beta testers wanted for 3.2.0.4