bug in iptables

bug in iptables

[justin joseph]

Hi,

I were testing shorewall with some configuration and found a bug in
shorewall version 3.4.4.

It seems to be there in iptables as well.

23:51 < justin007> I were testing shorewall and got a bug which seems
to be there in netfilter as well.
23:51 < justin007> iptables -t mangle -A tcpost -i lan1 -s
192.168.10.10 -o wan1 -p tcp --dport 22 -j CLASSIFY --set-class 1:11
23:52 < justin007> in tcpost the -i interface name is invalid,
iptables takes it though.
23:53 < jengelh> interesting
23:53 < jengelh> actually
23:53 < jengelh> ...
23:55 < jengelh> and, is it bad? no.
23:55 < jengelh> it does not crash the machine so all is fine for now
23:57 < justin007> yes it does not crach the machine. But it matches
all ports, *. which is not expected behaviour. man page does say that
the -i interfacenmae option is valid only in pre,
                   foreward, input chains
23:57 < justin007> Just wanted to mention this.
23:59 < jengelh> right
23:59 < jengelh> post it to the mailing list  (or I will do) so noone
forgets about it
Day changed to 15 Feb 2008
00:00 < justin007> please do post, I would need to join the list in
the first place :-)
00:01 < jengelh> you don't need to subscribe
00:01 < jengelh> just post to netfilter-devel@vger
00:01 < justin007> ok, I will post.
00:02 < jengelh> "Use of interface specification (e.g. -i) is not
checked against hooks when custom chain is used"
00:02 < jengelh> iptables -N foo; iptables -A foo -i eth0; iptables -A
OUTPUT -j foo;
00:03 < jengelh> That's all :)
00:03 < jengelh> short, sweet and to the point
00:05 < justin007> where is that from, I don't see it with man iptables
00:05 < jengelh> oh I just wrote that
00:05 < jengelh> that's what I would have written into the mail
00:05 < justin007> :-)

-justin

Re: bug in iptables

[justin joseph]

justin joseph wrote:
> Hi,


> It seems to be there in iptables as well.

To be specific I am able to add a rule thus:

iptables -t mangle -A tcpost -i lan1 -s 192.168.10.10 -o wan1 -p tcp --dport 22 -j CLASSIFY 
--set-class 1:11

Relevant "shorewall show mangle" output is:



Chain tcpost (1 references)
  pkts bytes target     prot opt in     out     source               destination
     0     0 CLASSIFY   tcp  --  lan1   wan1    192.168.10.10        0.0.0.0/0           tcp dpt:22 
CLASSIFY set 1:11
     0     0 CLASSIFY   all  --  *      wan1    0.0.0.0/0            0.0.0.0/0           MARK match 
0x1/0xff CLASSIFY set 1:11
     0     0 CLASSIFY   all  --  *      wan1    0.0.0.0/0            0.0.0.0/0           MARK match 
0xfe/0xff CLASSIFY set 1:1254


Man iptables says:

-i, --in-interface [!] name
Name of an interface via which a packet was received (only for packets entering the 
INPUT, FORWARD and PREROUTING chains).  When the "!" argument is used before  the  interface
name,  the  sense  is  inverted.  If the interface name ends in a "+", then any interface which 
begins with this name will match.  If this option is omitted, any interface name will match.

But iptables is taking the -i option in case of POSTROUTING as well.  In my case, I were trying to 
classify traffic coming from lan1:192.168.10.10 and although this rule was taken it was not being
hit because I understand from what Tom Eastep said, "It is because packets in the Postrouting chain 
are not guaranteed to even have an input chain"

-justin

Re: bug in iptables

[Patrick McHardy]

justin joseph wrote:
> 
> 
> justin joseph wrote:
>> Hi,
> 
> 
>> It seems to be there in iptables as well.
> 
> To be specific I am able to add a rule thus:
> 
> iptables -t mangle -A tcpost -i lan1 -s 192.168.10.10 -o wan1 -p tcp 
> --dport 22 -j CLASSIFY --set-class 1:11
> 
> Relevant "shorewall show mangle" output is:
> 
> 
> 
> Chain tcpost (1 references)
>  pkts bytes target     prot opt in     out     source               
> destination
>     0     0 CLASSIFY   tcp  --  lan1   wan1    192.168.10.10        
> 0.0.0.0/0           tcp dpt:22 CLASSIFY set 1:11
>     0     0 CLASSIFY   all  --  *      wan1    0.0.0.0/0            
> 0.0.0.0/0           MARK match 0x1/0xff CLASSIFY set 1:11
>     0     0 CLASSIFY   all  --  *      wan1    0.0.0.0/0            
> 0.0.0.0/0           MARK match 0xfe/0xff CLASSIFY set 1:1254
> 
> 
> Man iptables says:
> 
> -i, --in-interface [!] name
> Name of an interface via which a packet was received (only for packets 
> entering the INPUT, FORWARD and PREROUTING chains).  When the "!" 
> argument is used before  the  interface
> name,  the  sense  is  inverted.  If the interface name ends in a "+", 
> then any interface which begins with this name will match.  If this 
> option is omitted, any interface name will match.
> 
> But iptables is taking the -i option in case of POSTROUTING as well.  In 
> my case, I were trying to classify traffic coming from 
> lan1:192.168.10.10 and although this rule was taken it was not being
> hit because I understand from what Tom Eastep said, "It is because 
> packets in the Postrouting chain are not guaranteed to even have an 
> input chain"

Your example doesn't contain the rule jumping to "tcpost", so
its not clear whether this really is a bug. Please post all
four rules (tcpost and -j tcpost) and the kernel version you're
using.

Re: bug in iptables

[justin joseph]

Patrick McHardy wrote:
> justin joseph wrote:
>>
>>
>> justin joseph wrote:
>>> Hi,
>>
>>
>>> It seems to be there in iptables as well.
>>
>> To be specific I am able to add a rule thus:
>>
>> iptables -t mangle -A tcpost -i lan1 -s 192.168.10.10 -o wan1 -p tcp 
>> --dport 22 -j CLASSIFY --set-class 1:11
> 
> Your example doesn't contain the rule jumping to "tcpost", so
> its not clear whether this really is a bug. Please post all
> four rules (tcpost and -j tcpost) and the kernel version you're
> using.
> 



Chain POSTROUTING (policy ACCEPT 2263 packets, 528K bytes)
  pkts bytes target     prot opt in     out     source               destination
  2227  523K MARK       all  --  any    any     anywhere             anywhere            MARK and 0xff
  2227  523K tcpost     all  --  any    any     anywhere             anywhere

Chain tcfor (1 references)
  pkts bytes target     prot opt in     out     source               destination

Chain tcout (1 references)
  pkts bytes target     prot opt in     out     source               destination

Chain tcpost (1 references)
  pkts bytes target     prot opt in     out     source               destination
     0     0 CLASSIFY   tcp  --  lan1   wan1    anywhere             anywhere            tcp dpt:ssh 
CLASSIFY set 1:11
     0     0 CLASSIFY   all  --  any    wan1    anywhere             anywhere            MARK match 
0x1/0xff CLASSIFY set 1:11
     0     0 CLASSIFY   all  --  any    wan1    anywhere             anywhere            MARK match 
0xfe/0xff CLASSIFY set 1:1254


root@hq.enpaq:~# uname -r
2.6.15-29-386
root@hq.enpaq:~#

Re: bug in iptables

[Patrick McHardy]

justin joseph wrote:
> Patrick McHardy wrote:
>> Your example doesn't contain the rule jumping to "tcpost", so
>> its not clear whether this really is a bug. Please post all
>> four rules (tcpost and -j tcpost) and the kernel version you're
>> using.
>>
> 
> Chain POSTROUTING (policy ACCEPT 2263 packets, 528K bytes)
>  pkts bytes target     prot opt in     out     source               
> destination
>  2227  523K MARK       all  --  any    any     anywhere             
> anywhere            MARK and 0xff
>  2227  523K tcpost     all  --  any    any     anywhere             
> anywhere
> 
> Chain tcfor (1 references)
>  pkts bytes target     prot opt in     out     source               
> destination
> 
> Chain tcout (1 references)
>  pkts bytes target     prot opt in     out     source               
> destination
> 
> Chain tcpost (1 references)
>  pkts bytes target     prot opt in     out     source               
> destination
>     0     0 CLASSIFY   tcp  --  lan1   wan1    anywhere             
> anywhere            tcp dpt:ssh CLASSIFY set 1:11
>     0     0 CLASSIFY   all  --  any    wan1    anywhere             
> anywhere            MARK match 0x1/0xff CLASSIFY set 1:11
>     0     0 CLASSIFY   all  --  any    wan1    anywhere             
> anywhere            MARK match 0xfe/0xff CLASSIFY set 1:1254
> 
> 
> root@hq.enpaq:~# uname -r
> 2.6.15-29-386
> root@hq.enpaq:~#


Thanks, I can reproduce it on current -git. I'll look into it.

Re: bug in iptables

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

Patrick McHardy wrote:
> justin joseph wrote:
>
>> root@hq.enpaq:~# uname -r
>> 2.6.15-29-386
>> root@hq.enpaq:~#
> 
> 
> 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?

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3286 bytes --]

commit 6a90fb84a7333789aa39810ea5548342967bd27c
Author: Patrick McHardy <kaber@trash.net>
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@trash.net>

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

Re: bug in iptables

[Patrick McHardy]

Patrick McHardy wrote:
> Patrick McHardy wrote:
>> justin joseph wrote:
>>
>>> root@hq.enpaq:~# uname -r
>>> 2.6.15-29-386
>>> root@hq.enpaq:~#
>>
>>
>> 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?


Well, I decided against pushing this patch since there's no
harm in the current behaviour and the risk of breaking things
seems to high.