Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 1 Jul 2009 01:00:27 +0800

Adding appropriate lists and persons to CC:

> On Tue, Jun 30, 2009 at 05:27:47PM +0100, Mark McLoughlin wrote:
>> 
>> However, because nf_conntrack introduces an skb_orphan(), it is now
>> recommended that bridge-nf-call-iptables be disabled completely so as
>> to ensure features like TUNSETSNDBUF work as expected.
> 
> Patrick, does conntrack ever make sense for bridging? Perhaps
> we should get rid of that completely?
> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Jan Engelhardt]

On Tuesday 2009-06-30 21:06, David Miller wrote:
>Adding appropriate lists and persons to CC:
>
>> On Tue, Jun 30, 2009 at 05:27:47PM +0100, Mark McLoughlin wrote:
>>> 
>>> However, because nf_conntrack introduces an skb_orphan(), it is now
>>> recommended that bridge-nf-call-iptables be disabled completely so as
>>> to ensure features like TUNSETSNDBUF work as expected.
>> 
>> Patrick, does conntrack ever make sense for bridging? Perhaps
>> we should get rid of that completely?

It makes sense absolutely. Consider:

* packet enters bridge
* NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
* (connection tracking entry is set up)
* let bridging decision be "local delivery"

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Mark Smith]

Hi,

On Tue, 30 Jun 2009 22:16:35 +0200 (CEST)
Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Tuesday 2009-06-30 21:06, David Miller wrote:
> >Adding appropriate lists and persons to CC:
> >
> >> On Tue, Jun 30, 2009 at 05:27:47PM +0100, Mark McLoughlin wrote:
> >>> 
> >>> However, because nf_conntrack introduces an skb_orphan(), it is now
> >>> recommended that bridge-nf-call-iptables be disabled completely so as
> >>> to ensure features like TUNSETSNDBUF work as expected.
> >> 
> >> Patrick, does conntrack ever make sense for bridging? Perhaps
> >> we should get rid of that completely?
> 
> It makes sense absolutely. Consider:
> 
> * packet enters bridge
> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
> * (connection tracking entry is set up)
> * let bridging decision be "local delivery"

I really like this feature, although it is only because I've spent
time thinking about it, and it's usefulness, after having been burnt
quite a lot by it, due to it's quite strange side effects on traffic.
e.g. it'll defragment bridged IP packets, and then, if the outbound
bridge interface MTU is big enough, will send off large single ethernet
frames. If you're not expecting that, or don't work out what is
going on, you'll believe you're seeing the input traffic in the form
it arrived, and you'll believe it for quite a long time :-(

I'm not sure if it supposed to work on IP traffic carried within
bridge PPPoE/PPP, but it does - and that was very, very confusing to
work out what had happened too. PPPoE comes up, as does PPP and IPCP,
but forwarded IP packets are dropped unless there is a FORWARD
iptables rule.

I do agree though, either it should default to off, or the behaviour be
made far more prominent somehow.

Regards,
Mark.

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Jan Engelhardt]

On Tuesday 2009-06-30 22:57, Mark Smith wrote:
>>> 
>>> does conntrack ever make sense for bridging? Perhaps
>>> we should get rid of that completely?
>> 
>> It makes sense absolutely. Consider:
>> 
>> * packet enters bridge
>> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
>> * (connection tracking entry is set up)
>> * let bridging decision be "local delivery"
>quite a lot by it, due to it's quite strange side effects on traffic.
>e.g. it'll defragment bridged IP packets[...]

Hm not good. Then again, Netfilter does not know where the packet comes
from or where it goes, and Bridge does not know that Conntrack is
(potentially, it even varies) part of PREROUTING.

>I'm not sure if it supposed to work on IP traffic carried within
>bridge PPPoE/PPP, but it does

Bridge unpacks such frames if I've seen the code right :-/

        if (skb->protocol == htons(ETH_P_IPV6) || IS_VLAN_IPV6(skb) ||
            IS_PPPOE_IPV6(skb)) {
                nf_bridge_pull_encap_header_rcsum(skb);
                return br_nf_pre_routing_ipv6(hook, skb, in, out, okfn);
        }

I, too, wonder, why it would unpack PPP here.

However, there is a sysctl flag called vlan_tagged/pppoe_tagged which 
you can se to zero to not pull_encap VLAN/PPP.

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Herbert Xu]

On Wed, Jul 01, 2009 at 06:27:37AM +0930, Mark Smith wrote:
>
> I'm not sure if it supposed to work on IP traffic carried within
> bridge PPPoE/PPP, but it does - and that was very, very confusing to
> work out what had happened too. PPPoE comes up, as does PPP and IPCP,
> but forwarded IP packets are dropped unless there is a FORWARD
> iptables rule.

At least this bit should be off by default now, along as VLAN
decoding which was a gaping hole.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Herbert Xu]

On Tue, Jun 30, 2009 at 10:16:35PM +0200, Jan Engelhardt wrote:
>
> It makes sense absolutely. Consider:
> 
> * packet enters bridge
> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
> * (connection tracking entry is set up)
> * let bridging decision be "local delivery"

No, my question is does it ever make sense to use conntrack as
part of bridge netfilter.  That is, do you ever want to test it
in your rules that are run as part of bridge netfilter.

conntrack is inherently a security hole when used as part of
bridging, because it ignores the Ethernet header so two unrelated
connections can be tracked as one.

It used to be an even bigger security hole when we ignored VLAN
and PPPOE headers but at least that's now off by default.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Jan Engelhardt]

On Wednesday 2009-07-01 03:15, Herbert Xu wrote:
>On Tue, Jun 30, 2009 at 10:16:35PM +0200, Jan Engelhardt wrote:
>>
>> It makes sense absolutely. Consider:
>> 
>> * packet enters bridge
>> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
>> * (connection tracking entry is set up)
>> * let bridging decision be "local delivery"
>
>No, my question is does it ever make sense to use conntrack as
>part of bridge netfilter.  That is, do you ever want to test it
>in your rules that are run as part of bridge netfilter.

There is the possibility that some users have -m conntrack in their
mangle table in the PREROUTING chain. However, I am pretty sure that
if there are such users, they do it because of the layer-3/4/5/7 part
and not care about bridge so much.

>conntrack is inherently a security hole when used as part of
>bridging, because it ignores the Ethernet header so two unrelated
>connections can be tracked as one.

On secondary thought, one could also argue that because conntrack 
ignores the interface, two unrelated connections happening to be routed 
through the same machine(*) are tracked as one, too.




(*)
ip rule iif eth0 table 7
ip rule iif eth1 table 7
ip rule iif eth2 table 8
ip rule iif eth3 table 8
ip route add 2003::1/128 dev eth0 table 7
ip route add 2003::2/128 dev eth1 table 7
ip route add 2003::1/128 dev eth2 table 8
ip route add 2003::2/128 dev eth3 table 8
(four single nets, four single hosts)
In Unicode:

    A
    0
  ┌─║───┐
B1══╝   │
  │   ╔══2C
  └───║─┘
      3
      D

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Herbert Xu]

On Wed, Jul 01, 2009 at 05:50:18AM +0200, Jan Engelhardt wrote:
> 
> On secondary thought, one could also argue that because conntrack 
> ignores the interface, two unrelated connections happening to be routed 
> through the same machine(*) are tracked as one, too.

Good point.  We really should make these risks much more explicit.

However, I still think the risk with bridging is higher, especially
in the presence of virtualisation.  Consider the scenario where you
have to VMs on the one host, each with a dedicated bridge with the
intention that neither should know anything about the other's
traffic.

With conntrack running as part of bridging, the traffic can now
cross over which is a serious security hole.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Ben Greear]

Jan Engelhardt wrote:
> On Wednesday 2009-07-01 03:15, Herbert Xu wrote:
>   
>> On Tue, Jun 30, 2009 at 10:16:35PM +0200, Jan Engelhardt wrote:
>>     
>>> It makes sense absolutely. Consider:
>>>
>>> * packet enters bridge
>>> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
>>> * (connection tracking entry is set up)
>>> * let bridging decision be "local delivery"
>>>       
>> No, my question is does it ever make sense to use conntrack as
>> part of bridge netfilter.  That is, do you ever want to test it
>> in your rules that are run as part of bridge netfilter.
>>     
>
> There is the possibility that some users have -m conntrack in their
> mangle table in the PREROUTING chain. However, I am pretty sure that
> if there are such users, they do it because of the layer-3/4/5/7 part
> and not care about bridge so much.
>
>   
>> conntrack is inherently a security hole when used as part of
>> bridging, because it ignores the Ethernet header so two unrelated
>> connections can be tracked as one.
>>     
>
> On secondary thought, one could also argue that because conntrack 
> ignores the interface, two unrelated connections happening to be routed 
> through the same machine(*) are tracked as one, too.
>   
I had a similar problem while trying to implement virtual routers using 
different
routing tables and something like 'veth' to connect them.

My solution was to add a 'mark' field to the netdevice and allow 
user-space to set
the mark on the device.  This mark was included as part of the 
connection identifier.

The mark is set before the pkt hits the bridge code on ingress, so a pkt 
entering from
eth1 can get a different connection hash from a pkt entering eth2, even 
if all other
data in the packet is the same.

I'll dig it out of my monster patch if something like this is deemed 
useful upstream.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] bridge: make bridge-nf-call-*tables default configurable

[Patrick McHardy]

Herbert Xu wrote:
> On Tue, Jun 30, 2009 at 10:16:35PM +0200, Jan Engelhardt wrote:
>> It makes sense absolutely. Consider:
>>
>> * packet enters bridge
>> * NF_HOOK(PF_INET6, NF_INET_PRE_ROUTING, ...) is called by nr_netfilter.c
>> * (connection tracking entry is set up)
>> * let bridging decision be "local delivery"
> 
> No, my question is does it ever make sense to use conntrack as
> part of bridge netfilter.  That is, do you ever want to test it
> in your rules that are run as part of bridge netfilter.

Probably not, but thats not how its used currently. The packets are
passed to IP netfilter, which performs connection tracking. I'm not
sure how we could avoid the negative effects while still allowing this.