Re: bug in iptables

[Patrick McHardy <kaber@xxxxxxxxx>]

Patrick McHardy wrote:
> justin joseph wrote:
>
> > root@xxxxxxxx:~# uname -r
> > 2.6.15-29-386
> > root@xxxxxxxx:~#
>
>
> Thanks, I can reproduce it on current -git. I'll look into it.


OK actually we've never had a check for this in the kernel.
Userspace contains some basic checks based on the chainname,
but this only works for the built-in chains.

This patch adds the proper checks to the kernel. I'm a bit
worried though that this might break some rulesets. So
far we've allowed to create used-defined rules with these
"invalid" matches, which might even be useful to share
chains between multiple hooks, even if some matches will
not match depending on where the jump came from.

Opinions?

commit 6a90fb84a7333789aa39810ea5548342967bd27c
Author: Patrick McHardy <kaber@xxxxxxxxx>
Date:   Fri Feb 22 15:45:07 2008 +0100

    [NETFILTER]: {ip,ip6}_tables: check whether interface match is used on valid hooks
    
    Input device matches are only valid in PREROUTING/INPUT/FORWARD, output
    device matches in FORWARD/OUTPUT/POSTROUTING. Iptables userspace contains
    some checks based on the chainname, catching invalid uses in the built-in
    chains, but does't catch invalid matches in user-defined chains.
    
    Add checks for valid device-match usage based on the jumps to a chain.
    
    Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 600737f..0f4b16d 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -155,8 +155,11 @@ ip_packet_match(const struct iphdr *ip,
 }
 
 static bool
-ip_checkentry(const struct ipt_ip *ip)
+ip_checkentry(const struct ipt_entry *e)
 {
+	const struct ipt_ip *ip = &e->ip;
+	unsigned int i;
+
 	if (ip->flags & ~IPT_F_MASK) {
 		duprintf("Unknown flag bits set: %08X\n",
 			 ip->flags & ~IPT_F_MASK);
@@ -167,6 +170,20 @@ ip_checkentry(const struct ipt_ip *ip)
 			 ip->invflags & ~IPT_INV_MASK);
 		return false;
 	}
+	if (e->comefrom &
+	    ((1 << NF_INET_PRE_ROUTING) | (1 << NF_INET_LOCAL_IN))) {
+		for (i = 0; i < sizeof(ip->outiface); i++) {
+			if (ip->outiface_mask[i])
+				return false;
+		}
+	}
+	if (e->comefrom &
+	    ((1 << NF_INET_LOCAL_OUT) | (1 << NF_INET_POST_ROUTING))) {
+		for (i = 0; i < sizeof(ip->iniface); i++) {
+			if (ip->iniface_mask[i])
+				return false;
+		}
+	}
 	return true;
 }
 
@@ -589,7 +606,7 @@ check_entry(struct ipt_entry *e, const char *name)
 {
 	struct ipt_entry_target *t;
 
-	if (!ip_checkentry(&e->ip)) {
+	if (!ip_checkentry(e)) {
 		duprintf("ip_tables: ip check failed %p %s.\n", e, name);
 		return -EINVAL;
 	}
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index bf9bb6e..4f0cb7c 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -184,8 +184,11 @@ ip6_packet_match(const struct sk_buff *skb,
 
 /* should be ip6 safe */
 static bool
-ip6_checkentry(const struct ip6t_ip6 *ipv6)
+ip6_checkentry(const struct ip6t_entry *e)
 {
+	const struct ip6t_ip6 *ipv6 = &e->ipv6;
+	unsigned int i;
+
 	if (ipv6->flags & ~IP6T_F_MASK) {
 		duprintf("Unknown flag bits set: %08X\n",
 			 ipv6->flags & ~IP6T_F_MASK);
@@ -196,6 +199,20 @@ ip6_checkentry(const struct ip6t_ip6 *ipv6)
 			 ipv6->invflags & ~IP6T_INV_MASK);
 		return false;
 	}
+	if (e->comefrom &
+	    ((1 << NF_INET_PRE_ROUTING) | (1 << NF_INET_LOCAL_IN))) {
+		for (i = 0; i < sizeof(ipv6->outiface); i++) {
+			if (ipv6->outiface_mask[i])
+				return false;
+		}
+	}
+	if (e->comefrom &
+	    ((1 << NF_INET_LOCAL_OUT) | (1 << NF_INET_POST_ROUTING))) {
+		for (i = 0; i < sizeof(ipv6->iniface); i++) {
+			if (ipv6->iniface_mask[i])
+				return false;
+		}
+	}
 	return true;
 }
 
@@ -616,7 +633,7 @@ check_entry(struct ip6t_entry *e, const char *name)
 {
 	struct ip6t_entry_target *t;
 
-	if (!ip6_checkentry(&e->ipv6)) {
+	if (!ip6_checkentry(e)) {
 		duprintf("ip_tables: ip check failed %p %s.\n", e, name);
 		return -EINVAL;
 	}