[PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Patrick McHardy]

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

Hi Dave,

currently forwarded packets from a tunnel mode SA are checked
in ip_forward/ip6_forward against the XFRM_POLICY_FWD policy
list. Neither racoon nor pluto generate a policy for
IPSEC_DIR_FWD, so the checks are performed against an empty
list. I'm not sure who is wrong here, Linux or the keying daemons,
but I think using XFRM_POLICY_IN is more logical since we also
use XFRM_POLICY_OUT for forwarded packets, not XFRM_POLICY_FWD.

This patch changes ip_forward/ip6_forward to check against the
XFRM_POLICY_IN list.

BTW: The policy checks succeed as long as the policy list really is
empty because xfrm_policy_check skips the check if
xfrm_policy_list[dir] == NULL:

        return  !xfrm_policy_list[dir] ||
                (skb->dst->flags & DST_NOPOLICY) ||
                __xfrm_policy_check(sk, dir, skb, family);

I think this should really read (!xfrm_policy_list[dir] && !skb->sp)
so decapsulated IPsec packets are dropped if no policy exists.

Regards
Patrick


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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/17 05:19:03+02:00 kaber@coreworks.de 
#   [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv6/ip6_output.c
#   2004/10/17 05:18:26+02:00 kaber@coreworks.de +1 -1
#   [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/ip_forward.c
#   2004/10/17 05:18:26+02:00 kaber@coreworks.de +1 -1
#   [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
--- a/net/ipv4/ip_forward.c	2004-10-17 16:57:28 +02:00
+++ b/net/ipv4/ip_forward.c	2004-10-17 16:57:28 +02:00
@@ -60,7 +60,7 @@
 	struct rtable *rt;	/* Route we use */
 	struct ip_options * opt	= &(IPCB(skb)->opt);
 
-	if (!xfrm4_policy_check(NULL, XFRM_POLICY_FWD, skb))
+	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto drop;
 
 	if (IPCB(skb)->opt.router_alert && ip_call_ra_chain(skb))
diff -Nru a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
--- a/net/ipv6/ip6_output.c	2004-10-17 16:57:28 +02:00
+++ b/net/ipv6/ip6_output.c	2004-10-17 16:57:28 +02:00
@@ -355,7 +355,7 @@
 	if (ipv6_devconf.forwarding == 0)
 		goto error;
 
-	if (!xfrm6_policy_check(NULL, XFRM_POLICY_FWD, skb)) {
+	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 		IP6_INC_STATS(IPSTATS_MIB_INDISCARDS);
 		goto drop;
 	}

Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Herbert Xu]

On Sun, Oct 17, 2004 at 05:48:11PM +0200, Patrick McHardy wrote:
> 
> currently forwarded packets from a tunnel mode SA are checked
> in ip_forward/ip6_forward against the XFRM_POLICY_FWD policy
> list. Neither racoon nor pluto generate a policy for
> IPSEC_DIR_FWD, so the checks are performed against an empty

Actually I made damn sure that pluto does generate rules for
IPSEC_DIR_FWD after discussing it with Alexey :) Search for
XFRM_POLICY_FWD in openswan/programs/pluto.

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: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Patrick McHardy]

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

Herbert Xu wrote:

>On Sun, Oct 17, 2004 at 05:48:11PM +0200, Patrick McHardy wrote:
>  
>
>>currently forwarded packets from a tunnel mode SA are checked
>>in ip_forward/ip6_forward against the XFRM_POLICY_FWD policy
>>list. Neither racoon nor pluto generate a policy for
>>IPSEC_DIR_FWD, so the checks are performed against an empty
>>    
>>
>
>Actually I made damn sure that pluto does generate rules for
>IPSEC_DIR_FWD after discussing it with Alexey :) Search for
>XFRM_POLICY_FWD in openswan/programs/pluto.
>  
>
Thanks, I didn't know pluto uses the xfrm_user interface, so
I only looked for pfkey symbolic names. So it seems only racoon
needs to be fixed. I think we should apply the attached patch
to make xfrm_policy_check reject packets decapsulated by IPsec
without a policy for this direction, so people will notice
something is wrong. It also prevents skipping checks against the
socket policy if there is an empty policy list .. or am I missing
something ?

Regards
Patrick


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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/18 00:09:08+02:00 kaber@coreworks.de 
#   [XFRM]: Fix invalid optimization in xfrm_policy_check
#   
#   By skipping policy checks when the policy list is empty we
#   allow packets decapsulated by IPsec without a policy and
#   miss checking against socket policies.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/xfrm.h
#   2004/10/18 00:08:14+02:00 kaber@coreworks.de +1 -1
#   [XFRM]: Fix invalid optimization in xfrm_policy_check
#   
#   By skipping policy checks when the policy list is empty we
#   allow packets decapsulated by IPsec without a policy and
#   miss checking against socket policies.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
--- a/include/net/xfrm.h	2004-10-18 00:15:18 +02:00
+++ b/include/net/xfrm.h	2004-10-18 00:15:18 +02:00
@@ -601,7 +601,7 @@
 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
 		return __xfrm_policy_check(sk, dir, skb, family);
 		
-	return	!xfrm_policy_list[dir] ||
+	return	((!sk || !sk->sk_policy[dir]) && !xfrm_policy_list[dir] && !skb->sp) ||
 		(skb->dst->flags & DST_NOPOLICY) ||
 		__xfrm_policy_check(sk, dir, skb, family);
 }

Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Patrick McHardy]

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

Patrick McHardy wrote:

> Thanks, I didn't know pluto uses the xfrm_user interface, so
> I only looked for pfkey symbolic names. So it seems only racoon
> needs to be fixed. I think we should apply the attached patch
> to make xfrm_policy_check reject packets decapsulated by IPsec
> without a policy for this direction, so people will notice
> something is wrong. It also prevents skipping checks against the
> socket policy if there is an empty policy list .. or am I missing
> something ?

Obsiously I did :) This patch should be better.

> Regards
> Patrick
>
>--- a/include/net/xfrm.h	2004-10-18 00:15:18 +02:00
>+++ b/include/net/xfrm.h	2004-10-18 00:15:18 +02:00
>@@ -601,7 +601,7 @@
> 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
> 		return __xfrm_policy_check(sk, dir, skb, family);
> 		
>-	return	!xfrm_policy_list[dir] ||
>+	return	((!sk || !sk->sk_policy[dir]) && !xfrm_policy_list[dir] && !skb->sp) ||
> 		(skb->dst->flags & DST_NOPOLICY) ||
> 		__xfrm_policy_check(sk, dir, skb, family);
> }
>  
>


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

===== include/net/xfrm.h 1.68 vs edited =====
--- 1.68/include/net/xfrm.h	2004-09-10 23:35:53 +02:00
+++ edited/include/net/xfrm.h	2004-10-18 00:41:28 +02:00
@@ -601,7 +601,7 @@
 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
 		return __xfrm_policy_check(sk, dir, skb, family);
 		
-	return	!xfrm_policy_list[dir] ||
+	return	(!xfrm_policy_list[dir] && !skb->sp) ||
 		(skb->dst->flags & DST_NOPOLICY) ||
 		__xfrm_policy_check(sk, dir, skb, family);
 }

Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Herbert Xu]

On Mon, Oct 18, 2004 at 12:26:51AM +0200, Patrick McHardy wrote:
>
> I only looked for pfkey symbolic names. So it seems only racoon
> needs to be fixed.

Agreed.

> I think we should apply the attached patch
> to make xfrm_policy_check reject packets decapsulated by IPsec
> without a policy for this direction, so people will notice
> something is wrong. It also prevents skipping checks against the

Well it's too late to change the default policy.  People rely on the
default policy being allow so changing it will wreak havoc.  Even if
you do it only for packets with an IPsec encapsulation by checking
skb->sp it may still break people who use manual keying and rely on
the property that you can always add optional SAs.  More importantly
that it'll stick out like a sore thumb in terms of its semantics.

So let's just fix racoon.

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: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Patrick McHardy]

Herbert Xu wrote:

>
>Well it's too late to change the default policy.  People rely on the
>default policy being allow so changing it will wreak havoc.  Even if
>you do it only for packets with an IPsec encapsulation by checking
>skb->sp it may still break people who use manual keying and rely on
>the property that you can always add optional SAs.
>
You're right.

>  More importantly that it'll stick out like a sore thumb in terms of
>
 > its semantics.

__xfrm_policy_check already rejects packets without a matching policy
and skb->sp set, but it is skipped while the policy list is empty.
What, from a semantics point of view, would be wrong with making
xfrm_policy_check behave the same way ?

>
>So let's just fix racoon.
>
Agreed. I have a patch I'm currently testing. Judging from a quick
grep isakmpd also doesn't add forward policies.

Regards
Patrick

[XFRM] Allow transport SAs even when there is no policy

[Herbert Xu]

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

On Mon, Oct 18, 2004 at 10:34:23PM +0200, Patrick McHardy wrote:
> 
> > More importantly that it'll stick out like a sore thumb in terms of
> >
> > its semantics.
> 
> __xfrm_policy_check already rejects packets without a matching policy
> and skb->sp set, but it is skipped while the policy list is empty.
> What, from a semantics point of view, would be wrong with making
> xfrm_policy_check behave the same way ?

Good catch.  That was a bug introduced by yours truly :)

What I meant to say is all packets with tunnel mode SAs should be
rejected since we don't allow optional tunnel transforms for security
reasons.

This patch fixes it.

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

Thanks,
-- 
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: 896 bytes --]

===== net/xfrm/xfrm_policy.c 1.54 vs edited =====
--- 1.54/net/xfrm/xfrm_policy.c	2004-09-18 08:16:56 +10:00
+++ edited/net/xfrm/xfrm_policy.c	2004-10-19 07:42:37 +10:00
@@ -893,6 +893,16 @@
 	return 0;
 }
 
+static inline int secpath_has_tunnel(struct sec_path *sp, int k)
+{
+	for (; k < sp->len; k++) {
+		if (sp->x[k].xvec->props.mode)
+			return 1;
+	}
+
+	return 0;
+}
+
 int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, 
 			unsigned short family)
 {
@@ -930,7 +940,7 @@
 					xfrm_policy_lookup);
 
 	if (!pol)
-		return !skb->sp;
+		return !skb->sp || !secpath_has_tunnel(skb->sp, 0);
 
 	pol->curlft.use_time = (unsigned long)xtime.tv_sec;
 
@@ -954,10 +964,8 @@
 				goto reject;
 		}
 
-		for (; k < sp->len; k++) {
-			if (sp->x[k].xvec->props.mode)
-				goto reject;
-		}
+		if (secpath_has_tunnel(sp, k))
+			goto reject;
 
 		xfrm_pol_put(pol);
 		return 1;

Re: [XFRM] Allow transport SAs even when there is no policy

[Patrick McHardy]

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

Herbert Xu wrote:

>What I meant to say is all packets with tunnel mode SAs should be
>rejected since we don't allow optional tunnel transforms for security
>reasons.
>
>This patch fixes it.
>
Looks good. So you agree we should also apply my patch to
xfrm_policy_lookup (attached again with less confusing subject) ? It makes
packets with a secpath fall through to __xfrm_policy_check when the policy
list is empty, so the default policy is always the same. This will break
setups with keying daemons that don't add forward policies for tunnel mode
SAs.

Regards
Patrick


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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/19 16:04:44+02:00 kaber@coreworks.de 
#   [XFRM]: Apply policy checks to packets with a secpath when the policy list is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/xfrm.h
#   2004/10/19 16:04:18+02:00 kaber@coreworks.de +1 -1
#   [XFRM]: Apply policy checks to packets with a secpath when the policy list is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
--- a/include/net/xfrm.h	2004-10-19 16:05:39 +02:00
+++ b/include/net/xfrm.h	2004-10-19 16:05:39 +02:00
@@ -601,7 +601,7 @@
 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
 		return __xfrm_policy_check(sk, dir, skb, family);
 		
-	return	!xfrm_policy_list[dir] ||
+	return	(!xfrm_policy_list[dir] && !skb->sp) ||
 		(skb->dst->flags & DST_NOPOLICY) ||
 		__xfrm_policy_check(sk, dir, skb, family);
 }

Re: [Ipsec-tools-devel] Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Aidas Kasparas]

Herbert Xu wrote:
> 
> So let's just fix racoon.
> 

I'm sorry, what is wrong with racoon?

-- 
Aidas Kasparas
IT administrator
GM Consult Group, UAB

Re: [Ipsec-tools-devel] Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Patrick McHardy]

Aidas Kasparas wrote:

> I'm sorry, what is wrong with racoon?

When generate_policy is set to on racoon doesn't generate forward
policies for tunnel mode SAs, so traffic forwarded from a tunnel
is not subject to policy checks.

I have a patch which fixes this, I will post it a couple of days.

Regards
Patrick

Re: [Ipsec-tools-devel] Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Aidas Kasparas]

Patrick McHardy wrote:
> Aidas Kasparas wrote:
> 
>> I'm sorry, what is wrong with racoon?
> 
> 
> When generate_policy is set to on racoon doesn't generate forward
> policies for tunnel mode SAs, so traffic forwarded from a tunnel
> is not subject to policy checks.

Patrick,

	what _forward_ policies should racoon generate. And WHY?!

Could you please specify for the case when:
- remote host has address A.A.A.A
- security gateway have insecure adress B.B.B.B
- secured network is C.C.C.0/24, security gateway's address C.C.C.C

what policies in your oppinion has to be inserted into SPD for this 
setup by racoon?

	Thanks in advance.

> 
> I have a patch which fixes this, I will post it a couple of days.
> 
> Regards
> Patrick
> 

-- 
Aidas Kasparas
IT administrator
GM Consult Group, UAB

Re: [XFRM] Allow transport SAs even when there is no policy

[Herbert Xu]

On Tue, Oct 19, 2004 at 04:16:02PM +0200, Patrick McHardy wrote:
>
> Looks good. So you agree we should also apply my patch to
> xfrm_policy_lookup (attached again with less confusing subject) ? It makes
> packets with a secpath fall through to __xfrm_policy_check when the policy
> list is empty, so the default policy is always the same. This will break
> setups with keying daemons that don't add forward policies for tunnel mode
> SAs.

Agreed.  Thanks.
-- 
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: [Ipsec-tools-devel] Re: [PATCH 2.6]: Check against correct policy list in ip_forward/ip6_forward

[Herbert Xu]

On Tue, Oct 19, 2004 at 06:31:23PM +0300, Aidas Kasparas wrote:
> 
> I'm sorry, what is wrong with racoon?

For tunnel SAs it needs to add forward policies as well as incoming
policies.
-- 
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: [XFRM] Allow transport SAs even when there is no policy

[David S. Miller]

On Tue, 19 Oct 2004 07:43:26 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, Oct 18, 2004 at 10:34:23PM +0200, Patrick McHardy wrote:
> > 
> > > More importantly that it'll stick out like a sore thumb in terms of
> > >
> > > its semantics.
> > 
> > __xfrm_policy_check already rejects packets without a matching policy
> > and skb->sp set, but it is skipped while the policy list is empty.
> > What, from a semantics point of view, would be wrong with making
> > xfrm_policy_check behave the same way ?
> 
> Good catch.  That was a bug introduced by yours truly :)
> 
> What I meant to say is all packets with tunnel mode SAs should be
> rejected since we don't allow optional tunnel transforms for security
> reasons.
> 
> This patch fixes it.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.

Re: [XFRM] Allow transport SAs even when there is no policy

[David S. Miller]

On Wed, 20 Oct 2004 07:25:29 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Tue, Oct 19, 2004 at 04:16:02PM +0200, Patrick McHardy wrote:
> >
> > Looks good. So you agree we should also apply my patch to
> > xfrm_policy_lookup (attached again with less confusing subject) ? It makes
> > packets with a secpath fall through to __xfrm_policy_check when the policy
> > list is empty, so the default policy is always the same. This will break
> > setups with keying daemons that don't add forward policies for tunnel mode
> > SAs.
> 
> Agreed.  Thanks.

Also applied.  Thanks Patrick and Herbert.

BTW, Herbert, you can use a signed-off-by: line as an "ACK"
if you want :-)