[PATCH:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[PATCH:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Bart De Schuymer]

bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

Currently connection tracking isn't aware of the vlan id that bridged
encapsulated IP packets belong to. Since it's possible to enable
filtering of bridged IP traffic encapsulated in a vlan header, we
have a security risk if an overlapping IP network range is used on
different vlan networks. By making the connection tracking code use
the vlan id as part of the tuple, we eliminate this security risk.
Something similar could be done for IP traffic encapsulated in PPPoE,
but I'm not sure it's worth the effort.

Signed-off-by: Bart De Schuymer <bdschuym@pandora.be>

--- linux-2.6.33/net/netfilter/nf_conntrack_core.c	2010-02-24 19:52:17.000000000 +0100
+++ linux-2.6.33-uml/net/netfilter/nf_conntrack_core.c	2010-03-30 12:33:00.000000000 +0200
@@ -32,6 +32,7 @@
 #include <linux/mm.h>
 #include <linux/nsproxy.h>
 #include <linux/rculist_nulls.h>
+#include <linux/netfilter_bridge.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_l3proto.h>
@@ -110,6 +111,11 @@ nf_ct_get_tuple(const struct sk_buff *sk
 
 	tuple->dst.protonum = protonum;
 	tuple->dst.dir = IP_CT_DIR_ORIGINAL;
+#ifdef CONFIG_BRIDGE_NETFILTER
+	/* +1 because 0 identifies no vlan encapsulation */
+	if (skb->nf_bridge && (skb->nf_bridge->mask & BRNF_8021Q))
+		tuple->dst.vlan_id = (ntohs(*(u_int16_t *)(skb->data - VLAN_HLEN)) & VLAN_VID_MASK) + 1;
+#endif
 
 	return l4proto->pkt_to_tuple(skb, dataoff, tuple);
 }
@@ -158,6 +164,9 @@ nf_ct_invert_tuple(struct nf_conntrack_t
 	inverse->dst.dir = !orig->dst.dir;
 
 	inverse->dst.protonum = orig->dst.protonum;
+#ifdef CONFIG_BRIDGE_NETFILTER
+	inverse->dst.vlan_id = orig->dst.vlan_id;
+#endif
 	return l4proto->invert_tuple(inverse, orig);
 }
 EXPORT_SYMBOL_GPL(nf_ct_invert_tuple);
--- linux-2.6.33/include/net/netfilter/nf_conntrack_tuple.h	2010-02-24 19:52:17.000000000 +0100
+++ linux-2.6.33-uml/include/net/netfilter/nf_conntrack_tuple.h	2010-03-30 12:34:13.000000000 +0200
@@ -94,6 +94,9 @@ struct nf_conntrack_tuple {
 
 		/* The direction (for tuplehash) */
 		u_int8_t dir;
+#ifdef CONFIG_BRIDGE_NETFILTER
+		u_int16_t vlan_id;
+#endif
 	} dst;
 };
 
@@ -163,7 +166,11 @@ static inline bool __nf_ct_tuple_dst_equ
 {
 	return (nf_inet_addr_cmp(&t1->dst.u3, &t2->dst.u3) &&
 		t1->dst.u.all == t2->dst.u.all &&
-		t1->dst.protonum == t2->dst.protonum);
+		t1->dst.protonum == t2->dst.protonum
+#ifdef CONFIG_BRIDGE_NETFILTER
+		&& likely(t1->dst.vlan_id == t2->dst.vlan_id)
+#endif
+		);
 }
 
 static inline bool nf_ct_tuple_equal(const struct nf_conntrack_tuple *t1,

-- 
Bart De Schuymer
www.artinalgorithms.be

Re: [PATCH:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Eric Dumazet]

Le mardi 30 mars 2010 à 16:16 +0200, Bart De Schuymer a écrit :
> bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic
> 
> Currently connection tracking isn't aware of the vlan id that bridged
> encapsulated IP packets belong to. Since it's possible to enable
> filtering of bridged IP traffic encapsulated in a vlan header, we
> have a security risk if an overlapping IP network range is used on
> different vlan networks. By making the connection tracking code use
> the vlan id as part of the tuple, we eliminate this security risk.
> Something similar could be done for IP traffic encapsulated in PPPoE,
> but I'm not sure it's worth the effort.
> 
> Signed-off-by: Bart De Schuymer <bdschuym@pandora.be>
> 
> --- linux-2.6.33/net/netfilter/nf_conntrack_core.c	2010-02-24 19:52:17.000000000 +0100
> +++ linux-2.6.33-uml/net/netfilter/nf_conntrack_core.c	2010-03-30 12:33:00.000000000 +0200
> @@ -32,6 +32,7 @@
>  #include <linux/mm.h>
>  #include <linux/nsproxy.h>
>  #include <linux/rculist_nulls.h>
> +#include <linux/netfilter_bridge.h>
>  
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_l3proto.h>
> @@ -110,6 +111,11 @@ nf_ct_get_tuple(const struct sk_buff *sk
>  
>  	tuple->dst.protonum = protonum;
>  	tuple->dst.dir = IP_CT_DIR_ORIGINAL;
> +#ifdef CONFIG_BRIDGE_NETFILTER
> +	/* +1 because 0 identifies no vlan encapsulation */
> +	if (skb->nf_bridge && (skb->nf_bridge->mask & BRNF_8021Q))
> +		tuple->dst.vlan_id = (ntohs(*(u_int16_t *)(skb->data - VLAN_HLEN)) & VLAN_VID_MASK) + 1;
> +#endif
>  
>  	return l4proto->pkt_to_tuple(skb, dataoff, tuple);
>  }
> @@ -158,6 +164,9 @@ nf_ct_invert_tuple(struct nf_conntrack_t
>  	inverse->dst.dir = !orig->dst.dir;
>  
>  	inverse->dst.protonum = orig->dst.protonum;
> +#ifdef CONFIG_BRIDGE_NETFILTER
> +	inverse->dst.vlan_id = orig->dst.vlan_id;
> +#endif
>  	return l4proto->invert_tuple(inverse, orig);
>  }
>  EXPORT_SYMBOL_GPL(nf_ct_invert_tuple);
> --- linux-2.6.33/include/net/netfilter/nf_conntrack_tuple.h	2010-02-24 19:52:17.000000000 +0100
> +++ linux-2.6.33-uml/include/net/netfilter/nf_conntrack_tuple.h	2010-03-30 12:34:13.000000000 +0200
> @@ -94,6 +94,9 @@ struct nf_conntrack_tuple {
>  
>  		/* The direction (for tuplehash) */
>  		u_int8_t dir;
> +#ifdef CONFIG_BRIDGE_NETFILTER
> +		u_int16_t vlan_id;
> +#endif
>  	} dst;
>  };
>  
> @@ -163,7 +166,11 @@ static inline bool __nf_ct_tuple_dst_equ
>  {
>  	return (nf_inet_addr_cmp(&t1->dst.u3, &t2->dst.u3) &&
>  		t1->dst.u.all == t2->dst.u.all &&
> -		t1->dst.protonum == t2->dst.protonum);
> +		t1->dst.protonum == t2->dst.protonum
> +#ifdef CONFIG_BRIDGE_NETFILTER
> +		&& likely(t1->dst.vlan_id == t2->dst.vlan_id)
> +#endif
> +		);
>  }
>  
>  static inline bool nf_ct_tuple_equal(const struct nf_conntrack_tuple *t1,
> 

This really sounds very strange, layering violation or something.

You mix conntracking, bridge and vlan here.

Why setups without bridge should not care of vlan + conntracking side
effects ?

This whole idea was discussed last November : 

http://www.spinics.net/lists/netfilter-devel/msg10692.html

Patrick spoke of 'conntrack zone', and we added this concept.



commit 5d0aa2ccd4699a01cfdf14886191c249d7b45a01
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Feb 15 18:13:33 2010 +0100

    netfilter: nf_conntrack: add support for "conntrack zones"

    Normally, each connection needs a unique identity. Conntrack zones allow
    to specify a numerical zone using the CT target, connections in different
    zones can use the same identity.

    Example:

    iptables -t raw -A PREROUTING -i veth0 -j CT --zone 1
    iptables -t raw -A OUTPUT -o veth1 -j CT --zone 1

    Signed-off-by: Patrick McHardy <kaber@trash.net>


--
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:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Patrick McHardy]

Eric Dumazet wrote:
> Le mardi 30 mars 2010 à 16:16 +0200, Bart De Schuymer a écrit :
>> @@ -163,7 +166,11 @@ static inline bool __nf_ct_tuple_dst_equ
>>  {
>>  	return (nf_inet_addr_cmp(&t1->dst.u3, &t2->dst.u3) &&
>>  		t1->dst.u.all == t2->dst.u.all &&
>> -		t1->dst.protonum == t2->dst.protonum);
>> +		t1->dst.protonum == t2->dst.protonum
>> +#ifdef CONFIG_BRIDGE_NETFILTER
>> +		&& likely(t1->dst.vlan_id == t2->dst.vlan_id)
>> +#endif
>> +		);
>>  }
>>  
>>  static inline bool nf_ct_tuple_equal(const struct nf_conntrack_tuple *t1,
>>
> 
> This really sounds very strange, layering violation or something.
> 
> You mix conntracking, bridge and vlan here.

I agree, this is really wrong.

> Why setups without bridge should not care of vlan + conntracking side
> effects ?
> 
> This whole idea was discussed last November : 
> 
> http://www.spinics.net/lists/netfilter-devel/msg10692.html
> 
> Patrick spoke of 'conntrack zone', and we added this concept.

Indeed, this seems like a better way.
--
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:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Bart De Schuymer]

Patrick McHardy wrote:
> Eric Dumazet wrote:
>   
>> Le mardi 30 mars 2010 à 16:16 +0200, Bart De Schuymer a écrit :
>>     
>>> @@ -163,7 +166,11 @@ static inline bool __nf_ct_tuple_dst_equ
>>>  {
>>>  	return (nf_inet_addr_cmp(&t1->dst.u3, &t2->dst.u3) &&
>>>  		t1->dst.u.all == t2->dst.u.all &&
>>> -		t1->dst.protonum == t2->dst.protonum);
>>> +		t1->dst.protonum == t2->dst.protonum
>>> +#ifdef CONFIG_BRIDGE_NETFILTER
>>> +		&& likely(t1->dst.vlan_id == t2->dst.vlan_id)
>>> +#endif
>>> +		);
>>>  }
>>>  
>>>  static inline bool nf_ct_tuple_equal(const struct nf_conntrack_tuple *t1,
>>>
>>>       
>> This really sounds very strange, layering violation or something.
>>
>> You mix conntracking, bridge and vlan here.
>>     
>
> I agree, this is really wrong.
>   
Using the conntrack zone workaround to achieve what my patch does is
basically doing the same thing, IMHO.
But it's indeed much cleaner to use the generic CT target scheme, which
also solves the "multiple bridges" scenario mentioned by Pascal. I
wasn't yet aware of this target.

I've tested the following setup with 2.6.34-rc3 and it successfully
separates the networks (without my vlan patch).

# set up the connection tracking zones
iptables -t raw -A PREROUTING -m mark --mark 1 -j CT --zone 1
iptables -t raw -A PREROUTING -m mark --mark 2 -j CT --zone 2
# mark packets according to the vlan id
ebtables -t nat -A PREROUTING -p 802_1Q --vlan-id 1 -j mark --mark-set 1
ebtables -t nat -A PREROUTING -p 802_1Q --vlan-id 5 -j mark --mark-set 2

So this is good news. The security risk is solvable starting from
v2.6.34. I'll need to mention this clearly in the documentation.

What about the other patches? I'm aware they don't all cleanly patch
versus the most recent kernel, but do you have any objections apart from
that?

cheers,
Bart

-- 
Bart De Schuymer
www.artinalgorithms.be

--
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:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Patrick McHardy]

Bart De Schuymer wrote:
> Patrick McHardy wrote:
>> Eric Dumazet wrote:
>>>>       
>>> This really sounds very strange, layering violation or something.
>>>
>>> You mix conntracking, bridge and vlan here.
>>>     
>> I agree, this is really wrong.
>>   
> Using the conntrack zone workaround to achieve what my patch does is
> basically doing the same thing, IMHO.
> But it's indeed much cleaner to use the generic CT target scheme, which
> also solves the "multiple bridges" scenario mentioned by Pascal. I
> wasn't yet aware of this target.
> 
> I've tested the following setup with 2.6.34-rc3 and it successfully
> separates the networks (without my vlan patch).
> 
> # set up the connection tracking zones
> iptables -t raw -A PREROUTING -m mark --mark 1 -j CT --zone 1
> iptables -t raw -A PREROUTING -m mark --mark 2 -j CT --zone 2
> # mark packets according to the vlan id
> ebtables -t nat -A PREROUTING -p 802_1Q --vlan-id 1 -j mark --mark-set 1
> ebtables -t nat -A PREROUTING -p 802_1Q --vlan-id 5 -j mark --mark-set 2
> 
> So this is good news. The security risk is solvable starting from
> v2.6.34. I'll need to mention this clearly in the documentation.

Great.

> What about the other patches? I'm aware they don't all cleanly patch
> versus the most recent kernel, but do you have any objections apart from
> that?

I haven't fully reviewed them yet, but that should happen soon.

Re: [PATCH:RFC 5/5] bridge-netfilter: use the vlan id as part of the connection tracking tuple for bridged traffic

[Pascal Hambourg]

Eric Dumazet a écrit :
> 
> This really sounds very strange, layering violation or something.

Isn't the whole bridge-netfilter concept already a layering violation by
design ?

> You mix conntracking, bridge and vlan here.
> 
> Why setups without bridge should not care of vlan + conntracking side
> effects ?

Because without bridge, the host is attached at the IP layer level to
the VLANs, so their IP ranges are not supposed to overlap.

Anyway your objection applies to hosts with multiple bridges without
VLAN so the bridges may see overlapping IP ranges. Conntrack zones with
a dedicated target seems a more generic approach.

--
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