: Check against correct policy list in ip_forward/ip6_forward

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

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/10/17 05:19:03+02:00 [email protected]
# [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#
# Signed-off-by: Patrick McHardy <[email protected]>
#

# net/ipv6/ip6_output.c
#   2004/10/17 05:18:26+02:00 [email protected] +1 -1
#   [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/ipv4/ip_forward.c
#   2004/10/17 05:18:26+02:00 [email protected] +1 -1
#   [IPSEC]: Check against correct policy list in ip_forward/ip6_forward
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
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
@@ -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;
 	}

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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/10/18 00:09:08+02:00 [email protected]
# [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 <[email protected]>
# 
# include/net/xfrm.h
#   2004/10/18 00:08:14+02:00 [email protected] +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 <[email protected]>
# 
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);
}

Obsiously I did :) This patch should be better.

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

Agreed.

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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

You're right.

__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 ?

Agreed. I have a patch I'm currently testing. Judging from a quick grep isakmpd also doesn't add forward policies.

Regards
Patrick

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.

Thanks,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

===== net/xfrm/xfrm_policy.c 1.54 vs edited =====

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

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

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/10/19 16:04:44+02:00 [email protected]
# [XFRM]: Apply policy checks to packets with a secpath when the policy list is empty
#
# Signed-off-by: Patrick McHardy <[email protected]>
#

# include/net/xfrm.h
#   2004/10/19 16:04:18+02:00 [email protected] +1 -1
#   [XFRM]: Apply policy checks to packets with a secpath when the policy list is empty
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
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);
}

Agreed. Thanks.

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Also applied. Thanks Patrick and Herbert.

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

Applied, thanks Herbert.

I'm sorry, what is wrong with racoon?

Aidas Kasparas
IT administrator
GM Consult Group, UAB

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

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.

Aidas Kasparas
IT administrator
GM Consult Group, UAB

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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt