[NETFILTER] Apply IPsec to ipt_REJECT packets

[NETFILTER] Apply IPsec to ipt_REJECT packets

[Herbert Xu]

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

Hi:

I found out today that packets generated by ipt_REJECT weren't protected
by IPsec.  This is because the proto field isn't set at all in the flow
supplied to ip_route_output_key.

The following patch sets that as well as protocol-specific fields so
that the appropriate IPsec policy can be applied.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1592 bytes --]

===== net/ipv4/netfilter/ipt_REJECT.c 1.32 vs edited =====
--- 1.32/net/ipv4/netfilter/ipt_REJECT.c	2004-11-14 00:41:07 +11:00
+++ edited/net/ipv4/netfilter/ipt_REJECT.c	2004-11-23 19:35:22 +11:00
@@ -38,7 +38,8 @@
 #define DEBUGP(format, args...)
 #endif
 
-static inline struct rtable *route_reverse(struct sk_buff *skb, int hook)
+static inline struct rtable *route_reverse(struct sk_buff *skb, 
+					   struct tcphdr *tcph, int hook)
 {
 	struct iphdr *iph = skb->nh.iph;
 	struct dst_entry *odst;
@@ -56,6 +57,9 @@
 		if (hook == NF_IP_LOCAL_IN)
 			fl.nl_u.ip4_u.saddr = iph->daddr;
 		fl.nl_u.ip4_u.tos = RT_TOS(iph->tos);
+		fl.proto = IPPROTO_TCP;
+		fl.fl_ip_sport = tcph->dest;
+		fl.fl_ip_dport = tcph->source;
 
 		if (ip_route_output_key(&rt, &fl) != 0)
 			return NULL;
@@ -110,7 +114,7 @@
 		return;
 
 	/* FIXME: Check checksum --RR */
-	if ((rt = route_reverse(oldskb, hook)) == NULL)
+	if ((rt = route_reverse(oldskb, oth, hook)) == NULL)
 		return;
 
 	hh_len = LL_RESERVED_SPACE(rt->u.dst.dev);
@@ -282,10 +286,23 @@
 	tos = (iph->tos & IPTOS_TOS_MASK) | IPTOS_PREC_INTERNETCONTROL;
 
 	{
-		struct flowi fl = { .nl_u = { .ip4_u =
-					      { .daddr = skb_in->nh.iph->saddr,
-						.saddr = saddr,
-						.tos = RT_TOS(tos) } } };
+		struct flowi fl = {
+			.nl_u = {
+				.ip4_u = {
+					.daddr = skb_in->nh.iph->saddr,
+					.saddr = saddr,
+					.tos = RT_TOS(tos)
+				}
+			},
+			.proto = IPPROTO_ICMP,
+			.uli_u = {
+				.icmpt = {
+					.type = ICMP_DEST_UNREACH,
+					.code = code
+				}
+			}
+		};
+
 		if (ip_route_output_key(&rt, &fl))
 			return;
 	}

Re: [NETFILTER] Apply IPsec to ipt_REJECT packets

[Harald Welte]

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

On Tue, Nov 23, 2004 at 07:42:25PM +1100, Herbert Xu wrote:
> Hi:
> 
> I found out today that packets generated by ipt_REJECT weren't protected
> by IPsec.  This is because the proto field isn't set at all in the flow
> supplied to ip_route_output_key.

I see.  I guess REJECT is actually longer in the kernel than the IPsec
code, so nobody with a thorough understanding of both pieces of code did
notice that it needs to change.

> The following patch sets that as well as protocol-specific fields so
> that the appropriate IPsec policy can be applied.

The patch looks fine to me.  Dave: Please apply at your convenience.

> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Signed-off-by: Harald Welte <laforge@netfilter.org>

(in case this is needed)

> Cheers,

-- 
- 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Patrick McHardy]

Herbert Xu wrote:

>Hi:
>
>I found out today that packets generated by ipt_REJECT weren't protected
>by IPsec.  This is because the proto field isn't set at all in the flow
>supplied to ip_route_output_key.
>
>The following patch sets that as well as protocol-specific fields so
>that the appropriate IPsec policy can be applied.
>  
>

The patch doesn't handle tcp resets sent in response to a forwarded packet.
I'll send a patch later tonight.

Regards
Patrick

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Herbert Xu]

On Tue, Nov 23, 2004 at 07:17:36PM +0100, Patrick McHardy wrote:
> 
> The patch doesn't handle tcp resets sent in response to a forwarded packet.
> I'll send a patch later tonight.

Isn't that handled by ip_forward itself?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Herbert Xu]

On Wed, Nov 24, 2004 at 08:16:30AM +1100, herbert wrote:
> On Tue, Nov 23, 2004 at 07:17:36PM +0100, Patrick McHardy wrote:
> > 
> > The patch doesn't handle tcp resets sent in response to a forwarded packet.
> > I'll send a patch later tonight.
> 
> Isn't that handled by ip_forward itself?

In fact that's probably the reason why nobody has noticed this bug
until now :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Patrick McHardy]

Herbert Xu wrote:

>On Tue, Nov 23, 2004 at 07:17:36PM +0100, Patrick McHardy wrote:
>  
>
>>The patch doesn't handle tcp resets sent in response to a forwarded packet.
>>I'll send a patch later tonight.
>>    
>>
>
>Isn't that handled by ip_forward itself?
>  
>
No. ip_forward handles the original packet, not the packet generated
by ipt_REJECT. RSTs generated in NF_IP_FORWARD are routed using
ip_route_input because they have a non-local source, so xfrm_route_forward
or xfrm_lookup needs to be called for them.

Regards
Patrick

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Herbert Xu]

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

On Tue, Nov 23, 2004 at 10:44:33PM +0100, Patrick McHardy wrote:
>
> No. ip_forward handles the original packet, not the packet generated
> by ipt_REJECT. RSTs generated in NF_IP_FORWARD are routed using
> ip_route_input because they have a non-local source, so xfrm_route_forward
> or xfrm_lookup needs to be called for them.

You're absolutely right.  How about this patch then?

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> 

Now I'm puzzled as to how I haven't noticed this behaviour before.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 2049 bytes --]

===== net/ipv4/netfilter/ipt_REJECT.c 1.32 vs edited =====
--- 1.32/net/ipv4/netfilter/ipt_REJECT.c	2004-11-14 00:41:07 +11:00
+++ edited/net/ipv4/netfilter/ipt_REJECT.c	2004-11-24 09:18:19 +11:00
@@ -22,6 +22,7 @@
 #include <net/ip.h>
 #include <net/tcp.h>
 #include <net/route.h>
+#include <net/dst.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_REJECT.h>
 #ifdef CONFIG_BRIDGE_NETFILTER
@@ -38,7 +39,8 @@
 #define DEBUGP(format, args...)
 #endif
 
-static inline struct rtable *route_reverse(struct sk_buff *skb, int hook)
+static inline struct rtable *route_reverse(struct sk_buff *skb, 
+					   struct tcphdr *tcph, int hook)
 {
 	struct iphdr *iph = skb->nh.iph;
 	struct dst_entry *odst;
@@ -75,6 +77,10 @@
 		dst_release(&rt->u.dst);
 		rt = (struct rtable *)skb->dst;
 		skb->dst = odst;
+
+		fl.nl_u.ip4_u.daddr = iph->saddr;
+		fl.nl_u.ip4_u.saddr = iph->daddr;
+		fl.nl_u.ip4_u.tos = RT_TOS(iph->tos);
 	}
 
 	if (rt->u.dst.error) {
@@ -82,6 +88,15 @@
 		rt = NULL;
 	}
 
+	fl.proto = IPPROTO_TCP;
+	fl.fl_ip_sport = tcph->dest;
+	fl.fl_ip_dport = tcph->source;
+
+	if (xfrm_lookup((struct dst_entry **)&rt, &fl, NULL, 0)) {
+		dst_release(&rt->u.dst);
+		return NULL;
+	}
+
 	return rt;
 }
 
@@ -110,7 +125,7 @@
 		return;
 
 	/* FIXME: Check checksum --RR */
-	if ((rt = route_reverse(oldskb, hook)) == NULL)
+	if ((rt = route_reverse(oldskb, oth, hook)) == NULL)
 		return;
 
 	hh_len = LL_RESERVED_SPACE(rt->u.dst.dev);
@@ -282,10 +297,23 @@
 	tos = (iph->tos & IPTOS_TOS_MASK) | IPTOS_PREC_INTERNETCONTROL;
 
 	{
-		struct flowi fl = { .nl_u = { .ip4_u =
-					      { .daddr = skb_in->nh.iph->saddr,
-						.saddr = saddr,
-						.tos = RT_TOS(tos) } } };
+		struct flowi fl = {
+			.nl_u = {
+				.ip4_u = {
+					.daddr = skb_in->nh.iph->saddr,
+					.saddr = saddr,
+					.tos = RT_TOS(tos)
+				}
+			},
+			.proto = IPPROTO_ICMP,
+			.uli_u = {
+				.icmpt = {
+					.type = ICMP_DEST_UNREACH,
+					.code = code
+				}
+			}
+		};
+
 		if (ip_route_output_key(&rt, &fl))
 			return;
 	}

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Herbert Xu]

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

On Wed, Nov 24, 2004 at 02:54:30AM +0100, Patrick McHardy wrote:
>
> >	if (rt->u.dst.error) {
> >@@ -82,6 +88,15 @@
> >		rt = NULL;
> >
> ^ you should return here so xfrm_lookup isn't called without a dst_entry 
> below.

Good point.  Fixed in the following patch.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks for all your help, Patrick.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: q --]
[-- Type: text/plain, Size: 2041 bytes --]

===== net/ipv4/netfilter/ipt_REJECT.c 1.32 vs edited =====
--- 1.32/net/ipv4/netfilter/ipt_REJECT.c	2004-11-14 00:41:07 +11:00
+++ edited/net/ipv4/netfilter/ipt_REJECT.c	2004-11-24 17:26:04 +11:00
@@ -22,6 +22,7 @@
 #include <net/ip.h>
 #include <net/tcp.h>
 #include <net/route.h>
+#include <net/dst.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_REJECT.h>
 #ifdef CONFIG_BRIDGE_NETFILTER
@@ -38,7 +39,8 @@
 #define DEBUGP(format, args...)
 #endif
 
-static inline struct rtable *route_reverse(struct sk_buff *skb, int hook)
+static inline struct rtable *route_reverse(struct sk_buff *skb, 
+					   struct tcphdr *tcph, int hook)
 {
 	struct iphdr *iph = skb->nh.iph;
 	struct dst_entry *odst;
@@ -75,10 +77,23 @@
 		dst_release(&rt->u.dst);
 		rt = (struct rtable *)skb->dst;
 		skb->dst = odst;
+
+		fl.nl_u.ip4_u.daddr = iph->saddr;
+		fl.nl_u.ip4_u.saddr = iph->daddr;
+		fl.nl_u.ip4_u.tos = RT_TOS(iph->tos);
 	}
 
 	if (rt->u.dst.error) {
 		dst_release(&rt->u.dst);
+		return NULL;
+	}
+
+	fl.proto = IPPROTO_TCP;
+	fl.fl_ip_sport = tcph->dest;
+	fl.fl_ip_dport = tcph->source;
+
+	if (xfrm_lookup((struct dst_entry **)&rt, &fl, NULL, 0)) {
+		dst_release(&rt->u.dst);
 		rt = NULL;
 	}
 
@@ -110,7 +125,7 @@
 		return;
 
 	/* FIXME: Check checksum --RR */
-	if ((rt = route_reverse(oldskb, hook)) == NULL)
+	if ((rt = route_reverse(oldskb, oth, hook)) == NULL)
 		return;
 
 	hh_len = LL_RESERVED_SPACE(rt->u.dst.dev);
@@ -282,10 +297,23 @@
 	tos = (iph->tos & IPTOS_TOS_MASK) | IPTOS_PREC_INTERNETCONTROL;
 
 	{
-		struct flowi fl = { .nl_u = { .ip4_u =
-					      { .daddr = skb_in->nh.iph->saddr,
-						.saddr = saddr,
-						.tos = RT_TOS(tos) } } };
+		struct flowi fl = {
+			.nl_u = {
+				.ip4_u = {
+					.daddr = skb_in->nh.iph->saddr,
+					.saddr = saddr,
+					.tos = RT_TOS(tos)
+				}
+			},
+			.proto = IPPROTO_ICMP,
+			.uli_u = {
+				.icmpt = {
+					.type = ICMP_DEST_UNREACH,
+					.code = code
+				}
+			}
+		};
+
 		if (ip_route_output_key(&rt, &fl))
 			return;
 	}

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[Harald Welte]

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

On Wed, Nov 24, 2004 at 12:52:37AM +0100, Patrick McHardy wrote:

> I would prefer something like this (based on your patch, untested). 
> Currently
> ICMP packets are handled different than TCP packets, saddr is set to 0 for
> them if it is non-local, so they can't be source-routed properly. This patch
> also uses route_reverse for ICMP packets, properly sets fl->proto for output
> routed packets and adds a call to xfrm_lookup for input routed packets.

Just a quick side note: Once we've found a final solution, please don't
forget to merge the changes to ip6t_REJECT in patch-o-matic.

> Regards
> 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: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [netfilter-core] [NETFILTER] Apply IPsec to ipt_REJECT packets

[David S. Miller]

On Wed, 24 Nov 2004 17:27:47 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, Nov 24, 2004 at 02:54:30AM +0100, Patrick McHardy wrote:
> >
> > >	if (rt->u.dst.error) {
> > >@@ -82,6 +88,15 @@
> > >		rt = NULL;
> > >
> > ^ you should return here so xfrm_lookup isn't called without a dst_entry 
> > below.
> 
> Good point.  Fixed in the following patch.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Thanks for all your help, Patrick.

Applied for 2.6.10, thanks everyone.