Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Leen Besselink]

Hi netfilter-hackers,

OK, first the situation...

I use policy routing to decide (based on the from-address) on what
out-interface to send a packet (and thus a different sender IP-address and
gateway).

But... when I use reject-with tcp-reset, for a port on a host on the
network behind my firewall, the tcp-reset-packet goes over the wrong
interface.

I looked at the code and it looks like a bug to me, but there is a
comment, maybe there is a good reason not to:

        /* Routing: if not headed for us, route won't like source */
        if (ip_route_output(&rt, nskb->nh.iph->daddr,
                            local ? nskb->nh.iph->saddr : 0,
                            RT_TOS(nskb->nh.iph->tos) | RTO_CONN,
                            0) != 0)
                goto free_nskb;

If it's not a local destinated-packet it sends the sender-address to 0
when making the routing decision (probably 0.0.0.0 ?).

The calling code for send_reset sets local:

        case IPT_TCP_RESET:
                send_reset(*pskb, hooknum == NF_IP_LOCAL_IN);

When I use the send_unreach (for example with: icmp_host_unreachable),
this is not done and thus the source-based routing rules have effect:

        if (ip_route_output(&rt, iph->saddr, saddr, RT_TOS(tos), 0))
                return;

So, I think the 'local ? nskb->nh.iph->saddr : 0' can actually be
removed, am I right ?

But if there is a good reason, then I don't understand why it's not doing
the same thing for an unreachable, maybe that needs fixing instead then.

Any help would be greatly appreciated and tia,
	Leen.
_____________________________________
New things are always on the horizon.

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Leen Besselink]

On Sun, 13 Apr 2003, Leen Besselink wrote:

> Hi netfilter-hackers,
> 
> So, I think the 'local ? nskb->nh.iph->saddr : 0' can actually be
> removed, am I right ?
> 

OK, tried that, didn't work..., strange, maybe I should just add a printk
or something, see what happends, or see if I have a routing problem as
well.

Works with the default-route, strange. BTW, I think the part
about: RTO_CONN can be removed as well..., it supposidly old:

#define RTO_CONN        0
/* RTO_CONN is not used (being alias for 0), but preserved not to break
 * some modules referring to it. */

> But if there is a good reason, then I don't understand why it's not doing
> the same thing for an unreachable, maybe that needs fixing instead then.
> 

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Leen Besselink]

On Sun, 13 Apr 2003, Leen Besselink wrote:

> On Sun, 13 Apr 2003, Leen Besselink wrote:
> 
> > Hi netfilter-hackers,
> > 
> > So, I think the 'local ? nskb->nh.iph->saddr : 0' can actually be
> > removed, am I right ?
> > 
> 
> OK, tried that, didn't work..., strange, maybe I should just add a printk
> or something, see what happends, or see if I have a routing problem as
> well.
> 
> Works with the default-route, strange. BTW, I think the part

OK, I took a closer look, the icmp's from --reject-with don't go past the
policy-routing rules either.

Any ideas on how I can make it so, any pointers ?

tia,
	Leen.

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Patrick Schaaf]

HiHo,

> OK, first the situation...
> 
> I use policy routing to decide (based on the from-address) on what
> out-interface to send a packet (and thus a different sender IP-address and
> gateway).
> 
> But... when I use reject-with tcp-reset, for a port on a host on the
> network behind my firewall, the tcp-reset-packet goes over the wrong
> interface.

I don't understand that description fully. Let's be more specific.
You have an outside host O, a policy router P, and an inside target T.
Further assume that O sends in packets on P.eth0, and T sits on P.eth1.

1) when O sends a packet to P, P will see a source-NATted address Q, right?
   If not, what did you mean with 'thus a different sender IP-address'?

2) is the reject-with-tcp-reset rule on P or T?

3) in the situation you describe, O tries to send to T?

4) when the RST goes out the wrong interface, what interface is that,
   and on what interface did the packet come in?

5) how do the source and destination IP addresses of that RST packet
   look at PREROUTING?

6) how do the source and destination IP addresses of that RST packet
   look at POSTROUTING / on the outgoing wire?

Maybe, if you can answer these questions, we'll be able to understand
your problem. Maybe somebody can do that even without those answers,
but the silence you received up to now indicates that's unlikely.

best regards
  Patrick

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Patrick McHardy]

Patrick Schaaf wrote:

>Maybe, if you can answer these questions, we'll be able to understand
>your problem. Maybe somebody can do that even without those answers,
>but the silence you received up to now indicates that's unlikely.
>  
>

May I ? ;)

Without beeing specific to his setup, the problem he describes is
that ipt_REJECT (and others) always choose lsrc to be 0 if it's 
non-local for
the routing decision. I'll just make a simple example:

Routing Rules (incomplete):
1000: from 10.0.0.0/8 lookup abc
32766:  from all lookup main

Routing tables (incomplete):
abc: default via 192.168.0.1 dev eth0
main: default via 172.20.0.1 dev eth1

So all packets except those with src=10.0.0.0/8 should go through 
172.20.0.1.
When ipt_REJECT send a reject for any non-local address in 10.0.0.0/8 it 
chooses
lsrc=0 for the routing lookup, so it ends up with default route of table 
main
instead of table abc.

I wonder why ip_route_input isn't used, this should eliminate the need for
using different information from what is contained in the actual packet for
the routing lookup ..

Bye,
Patrick

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Leen Besselink]

On Mon, 14 Apr 2003, Patrick McHardy wrote:

> Patrick Schaaf wrote:
> 
> >Maybe, if you can answer these questions, we'll be able to understand
> >your problem. Maybe somebody can do that even without those answers,
> >but the silence you received up to now indicates that's unlikely.
> >  
> >
> 
> May I ? ;)
> 

You sure can. I was going to make a long story, but you made a nice
consice explaination, thank you.

> Without beeing specific to his setup, the problem he describes is
> that ipt_REJECT (and others) always choose lsrc to be 0 if it's 
> non-local for
> the routing decision. I'll just make a simple example:
> 
> Routing Rules (incomplete):
> 1000: from 10.0.0.0/8 lookup abc
> 32766:  from all lookup main
> 
> Routing tables (incomplete):
> abc: default via 192.168.0.1 dev eth0
> main: default via 172.20.0.1 dev eth1
> 
> So all packets except those with src=10.0.0.0/8 should go through 
> 172.20.0.1.
> When ipt_REJECT send a reject for any non-local address in 10.0.0.0/8 it 
> chooses
> lsrc=0 for the routing lookup, so it ends up with default route of table 
> main
> instead of table abc.
> 
> I wonder why ip_route_input isn't used, this should eliminate the need for
> using different information from what is contained in the actual packet for
> the routing lookup ..
> 

It's a safety messure, maybe because they are afraid of loops...? And
because it touches/triggers other things, like NAT, so it seems, when I
look at the code.

You have a firewall-rule of which an incoming packet would trigger a new
packet, which might trigger a firewall-rule to trigger an other packet...
etc. But maybe I'm wrong, I'm not familair with the code (yet ?).

> Bye,
> Patrick

Thanks again.

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Patrick McHardy]

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

Hi Leen,

Leen Besselink wrote:

>It's a safety messure, maybe because they are afraid of loops...? And
>because it touches/triggers other things, like NAT, so it seems, when I
>look at the code.
>  
>
"dumb nat" is also triggered in ip_route_output, loops should not occur 
because
ipt_REJECT gives packets directly to ip_route_output2. a reason might be 
ip_route_input
doing policy checks, namely it checks if forwarding is configured on the 
receiving interface.

>You have a firewall-rule of which an incoming packet would trigger a new
>packet, which might trigger a firewall-rule to trigger an other packet...
>etc. But maybe I'm wrong, I'm not familair with the code (yet ?).
>

i've attached a patch which uses ip_route_input for tcp_resets in 
ipt_REJECT.
would you care to test it ? i confirmed it rejects correctly without 
policy routing,
but i have a very simple setup here so no good testing ..
The patch adds a call to ip_route_input after the ip_route_output for 
non-local
sources. The call to ip_route_output is still necesarry so we know an 
interface
where the source ip may actually have entered, otherwise it would only work
with reverse-path filters turned off.

@netfilter-people: would something like this be acceptable ? REJECT and 
MIRROR
(which looks broken wrt ip_route_output and dst handling) need something
like this to work correctly with policy routing.

Bye
Patrick


[-- Attachment #2: ipt_REJECT-tcprst-route.diff --]
[-- Type: text/plain, Size: 1176 bytes --]

===== ipt_REJECT.c 1.10 vs edited =====
--- 1.10/net/ipv4/netfilter/ipt_REJECT.c	Mon Mar 31 17:00:55 2003
+++ edited/ipt_REJECT.c	Mon Apr 14 23:00:54 2003
@@ -66,11 +66,26 @@
 
 	/* Routing: if not headed for us, route won't like source */
 	if (ip_route_output(&rt, oldskb->nh.iph->daddr,
-			    local ? oldskb->nh.iph->saddr : 0,
-			    RT_TOS(oldskb->nh.iph->tos) | RTO_CONN,
-			    0) != 0)
+	                    local ? oldskb->nh.iph->saddr : 0,
+	                    RT_TOS(oldskb->nh.iph->tos), 0) != 0)
 		return;
 
+	/* if src is not local use ip_route_input to respect policy routing
+	 * decision. the call to ip_route_output is still necessary so we
+	 * have a interface where the ip may actually have entered.
+	 */
+	if (local == 0) {
+		struct dst_entry *out_dst = (struct dst_entry *)rt;
+		if (ip_route_input(oldskb, oldskb->nh.iph->saddr,
+		                  oldskb->nh.iph->daddr,
+		                  RT_TOS(oldskb->nh.iph->tos),
+                                  out_dst->dev) != 0)
+			return;
+		dst_release(out_dst);
+		rt = (struct rtable *)oldskb->dst;
+		dst_hold(&rt->u.dst);
+	}
+	
 	hh_len = (rt->u.dst.dev->hard_header_len + 15)&~15;
 
 

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Harald Welte]

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

On Mon, Apr 14, 2003 at 11:09:44PM +0200, Patrick McHardy wrote:

> @netfilter-people: would something like this be acceptable ? REJECT and 
> MIRROR (which looks broken wrt ip_route_output and dst handling) need
> something like this to work correctly with policy routing.

I don't see a problem with your patch.  Please note, however, that I am
not very familiar with the linux routing code - so I feel a bit unable
to make that decision.

We should talk to David Miller about this issue.  Could you please write
a short summary like 'old behaviour was X and we had Problem Y with it,
now we try to fix it with this patch doing Z'?  I would then put the
patch together with this summary (as .help file) in patch-o-matic
pending and discuss it with davem.

> Bye
> Patrick

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Patrick McHardy]

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

Hi Harald,
this is an updated version, description follows below.
If davem gives his blessing i will make a patch for MIRROR as well.

Harald Welte wrote:

>We should talk to David Miller about this issue.  Could you please write
>a short summary like 'old behaviour was X and we had Problem Y with it,
>now we try to fix it with this patch doing Z'?  I would then put the
>patch together with this summary (as .help file) in patch-o-matic
>pending and discuss it with davem.
>

Ok here it goes:

ipt_REJECT uses saddr=0 for lookups with ip_route_output for non-local ips,
therefore routing rules like "from a.b.c.d/x" don't apply for tcp-resets 
generated
by ipt_REJECT.

This patch makes reject call ip_route_output to find an interface where 
the source
ip may have come from (for reverse-path filters) and then call 
ip_route_input with
the source/dest address of the tcp-reset. It also fixes a bug introduced 
by last fix
for asym. routing, source and dest were switched so ip_route_output 
returned a
route in the wrong direction.

Best regards,
Patrick


[-- Attachment #2: ipt_REJECT-tcprst-route.diff --]
[-- Type: text/plain, Size: 1544 bytes --]

===== net/ipv4/netfilter/ipt_REJECT.c 1.10 vs edited =====
--- 1.10/net/ipv4/netfilter/ipt_REJECT.c	Mon Mar 31 17:00:55 2003
+++ edited/net/ipv4/netfilter/ipt_REJECT.c	Tue Apr 15 15:55:36 2003
@@ -11,7 +11,6 @@
 #include <net/icmp.h>
 #include <net/ip.h>
 #include <net/tcp.h>
-struct in_device;
 #include <net/route.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_REJECT.h>
@@ -64,12 +63,29 @@
 			 csum_partial((char *)otcph, otcplen, 0)) != 0)
 		return;
 
-	/* Routing: if not headed for us, route won't like source */
-	if (ip_route_output(&rt, oldskb->nh.iph->daddr,
-			    local ? oldskb->nh.iph->saddr : 0,
-			    RT_TOS(oldskb->nh.iph->tos) | RTO_CONN,
-			    0) != 0)
+	if (local) {
+		if (ip_route_output(&rt, oldskb->nh.iph->saddr,
+		                    oldskb->nh.iph->daddr,
+				    RT_TOS(oldskb->nh.iph->tos), 0) != 0)
 		return;
+	} else {
+		/* non-local source - we use ip_route_input to respect policy
+		 * routing rules. the call to ip_route_output is necessary to
+		 * get a valid interface where the source may have come from.
+		 */ 
+		if (ip_route_output(&rt, oldskb->nh.iph->daddr, 0, 0, 0) != 0)
+			return;
+		if (ip_route_input(oldskb, oldskb->nh.iph->saddr,
+		                   oldskb->nh.iph->daddr,
+				   RT_TOS(oldskb->nh.iph->tos),
+				   rt->u.dst.dev) != 0) {
+			dst_release(&rt->u.dst);
+			return;
+		}
+		dst_release(&rt->u.dst);
+		rt = (struct rtable *)oldskb->dst;
+		dst_hold(&rt->u.dst);
+	}
 
 	hh_len = (rt->u.dst.dev->hard_header_len + 15)&~15;
 

Re: Bug (?) in ipt_reject doesn't follow policy routing (2.4.x)

[Patrick McHardy]

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

This is a once-again-updated patch, i discovered two memory leaks,
one introduced by my patch and one from asym. routing fix. Both
are fixed in this version.

Bye,
Patrick


[-- Attachment #2: ipt_REJECT-route.diff --]
[-- Type: text/plain, Size: 2078 bytes --]

diff -Nru a/net/ipv4/netfilter/ipt_REJECT.c b/net/ipv4/netfilter/ipt_REJECT.c
--- a/net/ipv4/netfilter/ipt_REJECT.c	Wed Apr 16 02:14:52 2003
+++ b/net/ipv4/netfilter/ipt_REJECT.c	Wed Apr 16 02:14:52 2003
@@ -11,7 +11,6 @@
 #include <net/icmp.h>
 #include <net/ip.h>
 #include <net/tcp.h>
-struct in_device;
 #include <net/route.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_REJECT.h>
@@ -40,6 +39,7 @@
 	struct sk_buff *nskb;
 	struct tcphdr *otcph, *tcph;
 	struct rtable *rt;
+	struct dst_entry *odst;
 	unsigned int otcplen;
 	u_int16_t tmp_port;
 	u_int32_t tmp_addr;
@@ -64,12 +64,30 @@
 			 csum_partial((char *)otcph, otcplen, 0)) != 0)
 		return;
 
-	/* Routing: if not headed for us, route won't like source */
-	if (ip_route_output(&rt, oldskb->nh.iph->daddr,
-			    local ? oldskb->nh.iph->saddr : 0,
-			    RT_TOS(oldskb->nh.iph->tos) | RTO_CONN,
-			    0) != 0)
-		return;
+	if (local) {
+		if (ip_route_output(&rt, oldskb->nh.iph->saddr,
+		                    oldskb->nh.iph->daddr,
+				    RT_TOS(oldskb->nh.iph->tos), 0) != 0)
+			return;
+	} else {
+		/* non-local source - we use ip_route_input to respect policy
+		 * routing rules. the call to ip_route_output is necessary to
+		 * get a valid interface where the source ip may have come from.
+		 */ 
+		if (ip_route_output(&rt, oldskb->nh.iph->daddr, 0, 0, 0) != 0)
+			return;
+		odst = oldskb->dst;
+		if (ip_route_input(oldskb, oldskb->nh.iph->saddr,
+		                   oldskb->nh.iph->daddr,
+				   RT_TOS(oldskb->nh.iph->tos),
+				   rt->u.dst.dev) != 0) {
+			dst_release(&rt->u.dst);
+			return;
+		}
+		dst_release(&rt->u.dst);
+		rt = (struct rtable *)oldskb->dst;
+		oldskb->dst = odst;
+	}
 
 	hh_len = (rt->u.dst.dev->hard_header_len + 15)&~15;
 
@@ -80,8 +98,10 @@
 	   hh_len of incoming interface < hh_len of outgoing interface */
 	nskb = skb_copy_expand(oldskb, hh_len, skb_tailroom(oldskb),
 			       GFP_ATOMIC);
-	if (!nskb)
+	if (!nskb) {
+		dst_release(&rt->u.dst);
 		return;
+	}
 
 	dst_release(nskb->dst);
 	nskb->dst = &rt->u.dst;