[PATCH]: Fix ipt_REJECT problem with nf_bridge

[PATCH]: Fix ipt_REJECT problem with nf_bridge

[Nir Tzachar]

Hello.

There is a problem with the current version of ipt_REJECT when
operating on a bridge interface.
To be more exact, an RST is never sent to connections over the bridge,
as the ip_route_me_harder function always fail.

The problem arises from the following code
(net/ipv4/netfilter/ipt_REJECT.c line 221:)

        if (hook != NF_INET_FORWARD
#ifdef CONFIG_BRIDGE_NETFILTER
            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
#endif
           )
                addr_type = RTN_LOCAL;

but, as nskb was newly allocated just a few line back, the
oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.
The following patch corrects the problem. Please review and apply.

Cheers, Nir.


Signed-off-by: Nir Tzachar <nir.tzachar@gmail.com>
Tested-by: Elad Kertis <elad.kertis@gmail.com>

--- linux-2.6.28.7.old/net/ipv4/netfilter/ipt_REJECT.c	2009-02-26
10:28:51.000000000 +0200
+++ linux-2.6.28.7/net/ipv4/netfilter/ipt_REJECT.c	2009-02-26
10:31:26.000000000 +0200
@@ -98,6 +98,12 @@
 				       niph->saddr, niph->daddr,
 				       csum_partial(tcph,
 						    sizeof(struct tcphdr), 0));
+#ifdef CONFIG_BRIDGE_NETFILTER
+	if (oldskb->nf_bridge){
+		nf_bridge_get(oldskb->nf_bridge);
+		nskb->nf_bridge = oldskb->nf_bridge;
+	}
+#endif

 	addr_type = RTN_UNSPEC;
 	if (hook != NF_INET_FORWARD

Re: [PATCH]: Fix ipt_REJECT problem with nf_bridge

[Nir Tzachar]

Good Morning.

I am resending this patch, as I got no feedback the previous time.

Cheers.

On Thu, Feb 26, 2009 at 10:48 AM, Nir Tzachar <nir.tzachar@gmail.com> wrote:
> Hello.
>
> There is a problem with the current version of ipt_REJECT when
> operating on a bridge interface.
> To be more exact, an RST is never sent to connections over the bridge,
> as the ip_route_me_harder function always fail.
>
> The problem arises from the following code
> (net/ipv4/netfilter/ipt_REJECT.c line 221:)
>
>        if (hook != NF_INET_FORWARD
> #ifdef CONFIG_BRIDGE_NETFILTER
>            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
> #endif
>           )
>                addr_type = RTN_LOCAL;
>
> but, as nskb was newly allocated just a few line back, the
> oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.
> The following patch corrects the problem. Please review and apply.
>
> Cheers, Nir.
>
>
> Signed-off-by: Nir Tzachar <nir.tzachar@gmail.com>
> Tested-by: Elad Kertis <elad.kertis@gmail.com>
>
> --- linux-2.6.28.7.old/net/ipv4/netfilter/ipt_REJECT.c  2009-02-26
> 10:28:51.000000000 +0200
> +++ linux-2.6.28.7/net/ipv4/netfilter/ipt_REJECT.c      2009-02-26
> 10:31:26.000000000 +0200
> @@ -98,6 +98,12 @@
>                                       niph->saddr, niph->daddr,
>                                       csum_partial(tcph,
>                                                    sizeof(struct tcphdr), 0));
> +#ifdef CONFIG_BRIDGE_NETFILTER
> +       if (oldskb->nf_bridge){
> +               nf_bridge_get(oldskb->nf_bridge);
> +               nskb->nf_bridge = oldskb->nf_bridge;
> +       }
> +#endif
>
>        addr_type = RTN_UNSPEC;
>        if (hook != NF_INET_FORWARD
>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: Fix ipt_REJECT problem with nf_bridge

[Philip Craig]

Nir Tzachar wrote:
>> The problem arises from the following code
>> (net/ipv4/netfilter/ipt_REJECT.c line 221:)
>>
>>        if (hook != NF_INET_FORWARD
>> #ifdef CONFIG_BRIDGE_NETFILTER
>>            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
>> #endif
>>           )
>>                addr_type = RTN_LOCAL;
>>
>> but, as nskb was newly allocated just a few line back, the
>> oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.

Is there a reason you need to copy it into nskb, rather
than just changing the test to check oldskb?

I don't think ipv4 netfilter should be setting this field
for new packets.  The bridging code will do that if needed
when it receives the packet.

Re: [PATCH]: Fix ipt_REJECT problem with nf_bridge

[Nir Tzachar]

Hello.

Thanks for your reply.

On Tue, Mar 10, 2009 at 7:43 AM, Philip Craig <philipc@snapgear.com> wrote:
> Nir Tzachar wrote:
>>> The problem arises from the following code
>>> (net/ipv4/netfilter/ipt_REJECT.c line 221:)
>>>
>>>        if (hook != NF_INET_FORWARD
>>> #ifdef CONFIG_BRIDGE_NETFILTER
>>>            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
>>> #endif
>>>           )
>>>                addr_type = RTN_LOCAL;
>>>
>>> but, as nskb was newly allocated just a few line back, the
>>> oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.
>
> Is there a reason you need to copy it into nskb, rather
> than just changing the test to check oldskb?
>
> I don't think ipv4 netfilter should be setting this field
> for new packets.  The bridging code will do that if needed
> when it receives the packet.

I agree. However, when I tried it (before setting the bridge argument
on the new skb), the kernel crashes. I do not exactly remember where,
but I think route_me_harder tries to dereference the nf_bridge
pointer. I may be entirely wrong, so I'll give it another check.

Cheers.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: Fix ipt_REJECT problem with nf_bridge

[Nir Tzachar]

Hi.

On Wed, Mar 11, 2009 at 11:29 AM, Nir Tzachar <nir.tzachar@gmail.com> wrote:
> Hello.
>
> Thanks for your reply.
>
> On Tue, Mar 10, 2009 at 7:43 AM, Philip Craig <philipc@snapgear.com> wrote:
>> Nir Tzachar wrote:
>>>> The problem arises from the following code
>>>> (net/ipv4/netfilter/ipt_REJECT.c line 221:)
>>>>
>>>>        if (hook != NF_INET_FORWARD
>>>> #ifdef CONFIG_BRIDGE_NETFILTER
>>>>            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
>>>> #endif
>>>>           )
>>>>                addr_type = RTN_LOCAL;
>>>>
>>>> but, as nskb was newly allocated just a few line back, the
>>>> oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.
>>
>> Is there a reason you need to copy it into nskb, rather
>> than just changing the test to check oldskb?
>>
>> I don't think ipv4 netfilter should be setting this field
>> for new packets.  The bridging code will do that if needed
>> when it receives the packet.
>
> I agree. However, when I tried it (before setting the bridge argument
> on the new skb), the kernel crashes. I do not exactly remember where,
> but I think route_me_harder tries to dereference the nf_bridge
> pointer. I may be entirely wrong, so I'll give it another check.

I have checked the change you proposed (checking oldskb instead of
nskb) , and indeed it works. Should I resubmit a patch for it?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: Fix ipt_REJECT problem with nf_bridge

[Philip Craig]

Nir Tzachar wrote:
> I have checked the change you proposed (checking oldskb instead of
> nskb) , and indeed it works. Should I resubmit a patch for it?

Yes please, but it will need someone that knows more about
routing internals than me to review it still.

Re: Fix ipt_REJECT problem with nf_bridge

[Jan Engelhardt]

On Thursday 2009-03-12 02:04, Philip Craig wrote:
>Nir Tzachar wrote:

>>       if (hook != NF_INET_FORWARD
>>#ifdef CONFIG_BRIDGE_NETFILTER
>>            || (nskb->nf_bridge && nskb->nf_bridge->mask & BRNF_BRIDGED)
>>#endif
>>           )
>>                addr_type = RTN_LOCAL;
>>
>>but, as nskb was newly allocated just a few line back, the
>>oldskb->nf_bridge was never copied, so nskb->nf_bridge is always NULL.
>>The following patch corrects the problem. Please review and apply.
>>
>>I have checked the change you proposed (checking oldskb instead of
>>nskb) , and indeed it works. Should I resubmit a patch for it?
>
>Yes please, but it will need someone that knows more about
>routing internals than me to review it still.

Ok, let me step in then.

(Terminology. Plain packet: whatever triggers RST. RST packet: what
REJECT will send)

If I read the comments in ip_route_me_harder correctly, RTN_LOCAL
must not be used when ipt_REJECT generates the RST packet with a
source address that does not belong to the firewall itself. In other
words, if it is forwarded (hence the NF_INET_FORWARD check).

If hook is INPUT or OUTPUT, saddr will be ours anyway (except maybe
in TPROXY setups, yayay, don't wanna think about that right now ;-)
so RTN_LOCAL is ok.

Now look at (hook == NF_INET_FORWARD && BRNF_BRIDGED). For a plain
packet to arrive in the FORWARD chain, daddr must not be local. So
when the address tuple is reversed for the RST packet, saddr will not
be a local address.

This is the commit that originally introduced the BRNF_BRIDGED clause:

commit dbf3813cdabe3c0336667bbf3347652cb897e365 (v2.6.5-rc3-27-gdbf3813)
Author: Bart De Schuymer <bdschuym@pandora.be>
Date:   Tue Mar 30 23:18:10 2004 -0800

    [NETFILTER]: Do not require ip_forwarding for reset on a bridge.
    
    Currently, to be able to send a reset in the FORWARD chain of iptables
    for bridged traffic, ip forwarding must be enabled. This causes confusion
    and in some situations people really don't want to enable ip forwarding.
    The patch below lets the user send reset packets for bridged frames in
    the FORWARD chain, with ip forwarding disabled (as long as there is a
    route).

FYI: This is talking about "pure"-bridged traffic, i.e. traffic that will
go from one bridge port to another without touching the "routing decision"
box in [1]. In iptables terminology, that's

	FORWARD -i br0 -o br0

style traffic. Since the RST packet REJECT creates goes through
OUTPUT, so I would assume no forwarding would take place, and the
ip_forward flag not be relevant. The BRNF_BRIDGED clause therefore
seems wrong because it will always be a non-local saddr.

[1] http://jengelh.medozas.de/images/nf-packet-flow.png

Re: Fix ipt_REJECT problem with nf_bridge

[Philip Craig]

Jan Engelhardt wrote:
> FYI: This is talking about "pure"-bridged traffic, i.e. traffic that will
> go from one bridge port to another without touching the "routing decision"
> box in [1]. In iptables terminology, that's
> 
> 	FORWARD -i br0 -o br0
> 
> style traffic. Since the RST packet REJECT creates goes through
> OUTPUT, so I would assume no forwarding would take place, and the
> ip_forward flag not be relevant. The BRNF_BRIDGED clause therefore
> seems wrong because it will always be a non-local saddr.

I can understand this better now based on your explanation :-)
But I think this clause is okay.

The addr_type == RTN_LOCAL path works for pure bridged traffic
because ip_route_me_harder() avoids routing using saddr if
saddr is foreign.

We can't specify addr_type != RTN_LOCAL for the pure bridging case,
because that tries to route the RST as though we received it
from the network, which will fail if ip forwarding is disabled.

Re: Fix ipt_REJECT problem with nf_bridge

[Jan Engelhardt]

On Thursday 2009-03-12 07:04, Philip Craig wrote:
>Jan Engelhardt wrote:
>> FYI: This is talking about "pure"-bridged traffic, i.e. traffic that will
>> go from one bridge port to another without touching the "routing decision"
>> box in [1]. In iptables terminology, that's
>> 
>> 	FORWARD -i br0 -o br0
>> 
>> style traffic. Since the RST packet REJECT creates goes through
>> OUTPUT, so I would assume no forwarding would take place, and the
>> ip_forward flag not be relevant. The BRNF_BRIDGED clause therefore
>> seems wrong because it will always be a non-local saddr.
>
>I can understand this better now based on your explanation :-)
>But I think this clause is okay.
>
>The addr_type == RTN_LOCAL path works for pure bridged traffic
>because ip_route_me_harder() avoids routing using saddr if
>saddr is foreign.
>
>We can't specify addr_type != RTN_LOCAL for the pure bridging case,
>because that tries to route the RST as though we received it
>from the network, which will fail if ip forwarding is disabled.
>
So what about the OP's observation that nskb->nf_bridge == NULL?
Just because the incoming packet came in over a bridge does
not mean the RST is going over one too, and that being the
deciding factor for RTN_LOCAL or not, is it?

Re: Fix ipt_REJECT problem with nf_bridge

[Nir Tzachar]

> So what about the OP's observation that nskb->nf_bridge == NULL?>
> Just because the incoming packet came in over a bridge does
> not mean the RST is going over one too, and that being the
> deciding factor for RTN_LOCAL or not, is it?

Well, in my case the RST goes over the bridge also. But the main
factor is the following comment made by Philip:
>
> >We can't specify addr_type != RTN_LOCAL for the pure bridging case,
> >because that tries to route the RST as though we received it
> >from the network, which will fail if ip forwarding is disabled.
> >
>

In my settings, I have a transparent bridge which does not allow ip
forwarding. According to the comment above, this would prevent the RST
from being sent.

In any way, I will happily test any suggestions/patches after the weekend.
Cheers.

Re: Fix ipt_REJECT problem with nf_bridge

[Philip Craig]

Jan Engelhardt wrote:
> So what about the OP's observation that nskb->nf_bridge == NULL?
> Just because the incoming packet came in over a bridge does
> not mean the RST is going over one too, and that being the
> deciding factor for RTN_LOCAL or not, is it?

Agreed that the RST may not be going out a bridge.

We don't know yet whether the RST will be bridged; that's why
we are calling ip_route_me_harder().

The deciding factor for RTN_LOCAL should be whether the incoming
packet is purely bridged, because that means the RST will have
a foreign source address.  We must follow the first code path in
ip_route_me_harder() or we drop it when ip forwarding is disabled,
but if the RST has a foreign source address then it will normally
follow the second code path, so we must force the first by
specifying RTN_LOCAL.

Re: Fix ipt_REJECT problem with nf_bridge

[Patrick McHardy]

Jan Engelhardt wrote:
> If I read the comments in ip_route_me_harder correctly, RTN_LOCAL
> must not be used when ipt_REJECT generates the RST packet with a
> source address that does not belong to the firewall itself. In other
> words, if it is forwarded (hence the NF_INET_FORWARD check).

Correct.

> If hook is INPUT or OUTPUT, saddr will be ours anyway (except maybe
> in TPROXY setups, yayay, don't wanna think about that right now ;-)
> so RTN_LOCAL is ok.
> 
> Now look at (hook == NF_INET_FORWARD && BRNF_BRIDGED). For a plain
> packet to arrive in the FORWARD chain, daddr must not be local. So
> when the address tuple is reversed for the RST packet, saddr will not
> be a local address.
> 
> This is the commit that originally introduced the BRNF_BRIDGED clause:
> 
> commit dbf3813cdabe3c0336667bbf3347652cb897e365 (v2.6.5-rc3-27-gdbf3813)
> Author: Bart De Schuymer <bdschuym@pandora.be>
> Date:   Tue Mar 30 23:18:10 2004 -0800
> 
>     [NETFILTER]: Do not require ip_forwarding for reset on a bridge.
>     
>     Currently, to be able to send a reset in the FORWARD chain of iptables
>     for bridged traffic, ip forwarding must be enabled. This causes confusion
>     and in some situations people really don't want to enable ip forwarding.
>     The patch below lets the user send reset packets for bridged frames in
>     the FORWARD chain, with ip forwarding disabled (as long as there is a
>     route).
> 
> FYI: This is talking about "pure"-bridged traffic, i.e. traffic that will
> go from one bridge port to another without touching the "routing decision"
> box in [1]. In iptables terminology, that's
> 
> 	FORWARD -i br0 -o br0
> 
> style traffic. Since the RST packet REJECT creates goes through
> OUTPUT, so I would assume no forwarding would take place, and the
> ip_forward flag not be relevant. The BRNF_BRIDGED clause therefore
> seems wrong because it will always be a non-local saddr.

Packets with non-local saddrs are routed similar to forward packets
(meaning the use ip_route_input and set an input device for policy
routing). This is also where the forwarding check kicks in.

We should actually be able to avoid these hacks nowadays since
routing allows to override the "output must use local source"
check using FLOWI_FLAG_ANYSRC.