[PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Bart De Schuymer]

Hi Dave,

ip_sabotage_out() needs to distinguish IPv4 and IPv6 traffic. It
currently does that by looking at skb->protocol. However, for locally
originated packets, skb->protocol is not initialized.
The patch below instead looks at the version number of the packet's
data, which should be 4 or 6.

Thanks to Pasha (Crazy AMD K7 <snort2004@mail.ru>) for his patience.

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

--- linux-2.6.10/net/bridge/br_netfilter.c.old	2004-12-30 15:34:11.000000000 +0100
+++ linux-2.6.10/net/bridge/br_netfilter.c	2004-12-30 19:13:31.000000000 +0100
@@ -845,19 +845,6 @@ static unsigned int ip_sabotage_out(unsi
 {
 	struct sk_buff *skb = *pskb;
 
-#ifdef CONFIG_SYSCTL
-	if (!skb->nf_bridge) {
-		struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
-
-		if (skb->protocol == __constant_htons(ETH_P_IP) ||
-		    IS_VLAN_IP) {
-			if (!brnf_call_iptables)
-				return NF_ACCEPT;
-		} else if (!brnf_call_ip6tables)
-			return NF_ACCEPT;
-	}
-#endif
-
 	if ((out->hard_start_xmit == br_dev_xmit &&
 	    okfn != br_nf_forward_finish &&
 	    okfn != br_nf_local_out_finish &&
@@ -869,8 +856,24 @@ static unsigned int ip_sabotage_out(unsi
 	    ) {
 		struct nf_bridge_info *nf_bridge;
 
-		if (!skb->nf_bridge && !nf_bridge_alloc(skb))
-			return NF_DROP;
+		if (!skb->nf_bridge) {
+#ifdef CONFIG_SYSCTL
+			/* This code is executed while in the IP(v6) stack,
+			   the version should be 4 or 6. We can't use
+			   skb->protocol because that isn't set on
+			   PF_INET(6)/LOCAL_OUT. */
+			struct iphdr *ip = skb->nh.iph;
+
+			if (ip->version == 4 && !brnf_call_iptables)
+				return NF_ACCEPT;
+			else if (ip->version == 6 && !brnf_call_ip6tables)
+				return NF_ACCEPT;
+#endif
+			if (hook == NF_IP_POST_ROUTING)
+				return NF_ACCEPT;
+			if (!nf_bridge_alloc(skb))
+				return NF_DROP;
+		}
 
 		nf_bridge = skb->nf_bridge;
 

Re: [PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Lennert Buytenhek]

On Thu, Dec 30, 2004 at 07:55:14PM +0100, Bart De Schuymer wrote:

> ip_sabotage_out() needs to distinguish IPv4 and IPv6 traffic. It
> currently does that by looking at skb->protocol. However, for locally
> originated packets, skb->protocol is not initialized.
> The patch below instead looks at the version number of the packet's
> data, which should be 4 or 6.

A while ago there were a number of problems with bridging CIPE ethernet
devices, which turned out to be the bridge code not initialising
skb->protocol for locally originated STP frames.

At the time I was told that initialising skb->protocol for locally
originated packets is required, so that is how I fixed it then.


cheers,
Lennert

Re: [PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Bart De Schuymer]

Op do, 30-12-2004 te 23:24 +0100, schreef Lennert Buytenhek:
> On Thu, Dec 30, 2004 at 07:55:14PM +0100, Bart De Schuymer wrote:
> 
> > ip_sabotage_out() needs to distinguish IPv4 and IPv6 traffic. It
> > currently does that by looking at skb->protocol. However, for locally
> > originated packets, skb->protocol is not initialized.
> > The patch below instead looks at the version number of the packet's
> > data, which should be 4 or 6.
> 
> A while ago there were a number of problems with bridging CIPE ethernet
> devices, which turned out to be the bridge code not initialising
> skb->protocol for locally originated STP frames.
> 
> At the time I was told that initialising skb->protocol for locally
> originated packets is required, so that is how I fixed it then.

Hi Lennert,

skb->protocol is not set for locally generated packets when the packet
is still in the IP stack. I don't know what happens with it after the IP
stack is finished with the packet.
The comment in skbuff.h says "packet protocol from driver", from which I
tend to conclude that skb->protocol is only set by drivers when a packet
enters the box. Too bad stuff like this isn't clearly spelled out, the
FIXME for the dst field has been sitting there for probably more than a
year too. Anyway, it wouldn't hurt if the skb->protocol field always
held the right value.

cheers,
Bart

Re: [PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Patrick McHardy]

Bart De Schuymer wrote:
> Hi Lennert,
> 
> skb->protocol is not set for locally generated packets when the packet
> is still in the IP stack. I don't know what happens with it after the IP
> stack is finished with the packet.

It is set shortly before the packet leaves the IP stack,
in ip_finish_output. This is after LOCAL_OUT, but before
POST_ROUTING. So your fix looks fine.

> The comment in skbuff.h says "packet protocol from driver", from which I
> tend to conclude that skb->protocol is only set by drivers when a packet
> enters the box. Too bad stuff like this isn't clearly spelled out, the
> FIXME for the dst field has been sitting there for probably more than a
> year too. Anyway, it wouldn't hurt if the skb->protocol field always
> held the right value.

The IP stack knows it's IP anyway :) After that it has to
hold the right value.

Regards
Patrick

Re: [PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Lennert Buytenhek]

On Fri, Dec 31, 2004 at 12:10:48AM +0100, Bart De Schuymer wrote:

> > A while ago there were a number of problems with bridging CIPE ethernet
> > devices, which turned out to be the bridge code not initialising
> > skb->protocol for locally originated STP frames.
> > 
> > At the time I was told that initialising skb->protocol for locally
> > originated packets is required, so that is how I fixed it then.
> 
> Hi Lennert,

Hello,


> skb->protocol is not set for locally generated packets when the packet
> is still in the IP stack. I don't know what happens with it after the IP
> stack is finished with the packet.
> The comment in skbuff.h says "packet protocol from driver", from which I
> tend to conclude that skb->protocol is only set by drivers when a packet
> enters the box.
> Too bad stuff like this isn't clearly spelled out,

This is what I thought back then too.  Indeed, it's rather misleading.


> the FIXME for the dst field has been sitting there for probably more
> than a year too.

Yes :(

Just one more thing: AFAIK it is possible to inject a raw IPv4 packet
with an invalid IPv4 header.  So maybe the better 'fix' would be to have
different hooks for PF_INET and PF_INET6, and distinguish v4/v6 packets
that way instead of peeking into the header.  (The hook you're talking
about is a PF_INET* and not a PF_BRIDGE hook, right?)

Then again, that would add yet another function onto the already rather
deep call chains that we have in there.

Too bad I don't see any cleaner way of integrating the whole bridging
thing into the stack.  I wonder if any of the *BSDs found a cleaner way
of doing this.


--L

Re: [PATCH][BRIDGE-NF] Fix wrong use of skb->protocol

[Bart De Schuymer]

Op vr, 31-12-2004 te 09:33 +0100, schreef Lennert Buytenhek:

> Just one more thing: AFAIK it is possible to inject a raw IPv4 packet
> with an invalid IPv4 header.  So maybe the better 'fix' would be to have
> different hooks for PF_INET and PF_INET6, and distinguish v4/v6 packets
> that way instead of peeking into the header.  (The hook you're talking
> about is a PF_INET* and not a PF_BRIDGE hook, right?)

That was my original plan, but it seems such a waste.
Wouldn't injecting such an invalid IPv4 header also screw up iptables?
Is there any reason why someone should be allowed to do this?
I checked ip_tables.c::ipt_do_table() before using the IP version, and
it looks at the IP header too without any precautions AFAICT.

> Then again, that would add yet another function onto the already rather
> deep call chains that we have in there.

The netfilter scheme itself implies call chains.

> Too bad I don't see any cleaner way of integrating the whole bridging
> thing into the stack.  I wonder if any of the *BSDs found a cleaner way
> of doing this.

How about adding something like NF_STOP, which acts like NF_STOLEN but
still executes okfn in nf_hook_slow()?

cheers,
Bart