[PATCH RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[PATCH RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[Tobias Brunner]

Currently inbound policies for transport mode SAs are not enforced.
If no policy is found or if the templates don't match this is not
considered an error for transport mode SAs.

The new sysctl option (net.core.xfrm_enforce_policies_transport_mode)
allows enforcing the inbound policies also for transport mode SAs.
By default this option remains disabled.

Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
Consider a transport mode SA between two peers over which only TCP and
ICMP traffic should be allowed.  Because two protocols are involved the
selector on the SA itself (xfrm_usersa_info.sel) can't be set.  Instead
one either has to negotiate separate SAs for each protocol (with a
selector on each) or negotiate one SA and install two policies that
use it.  The problem with the latter is that the peer does not have to
adhere to the negotiated traffic selectors, it is basically free to
send anything over the SA e.g. UDP packets.  So if no selectors are
installed on the SA itself, the current implementation would accept
such packets, even though the installed policies don't allow UDP
traffic (some might consider this a security issue - at the very least
it is not very intuitive, especially because the behavior is different
for tunnel mode SAs).
By enabling the new sysctl option the UDP packets would get dropped,
exactly as they would if the SA were negotiated in tunnel mode.

The behavior for optional transport mode templates also changes when
the option is enabled.  Basically, the special treatment of transport
mode SAs is disabled and the behavior is like it is for other modes.
I tested this with IPComp/ESP and strongSwan where the optional IPComp
transform is installed in transport or tunnel mode depending on the
negotiated mode of the SA (the ESP SA is always installed in transport
mode in this combination).  But I'm not sure if there are other uses
for optional transport mode transforms that might rely on the current
behavior (i.e. why are optional templates treated differently depending
on their mode in the first place?).

With this patch the default behavior remains as it is, but I wondered
why it is like that and if we could perhaps change it so that policies
are enforced by default, thus making such setups more secure out of
the box.  Or if we could even change the whole inbound processing so
that transport mode SAs are treated exactly like their counterparts
in other modes, without an option to change it.

 Documentation/networking/xfrm_sysctl.txt |  4 ++++
 include/net/netns/xfrm.h                 |  1 +
 net/xfrm/xfrm_policy.c                   | 24 +++++++++++++++---------
 net/xfrm/xfrm_sysctl.c                   |  8 ++++++++
 4 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/Documentation/networking/xfrm_sysctl.txt b/Documentation/networking/xfrm_sysctl.txt
index 5bbd167..2fbe539 100644
--- a/Documentation/networking/xfrm_sysctl.txt
+++ b/Documentation/networking/xfrm_sysctl.txt
@@ -2,3 +2,7 @@
  xfrm_acq_expires - INTEGER
 	default 30 - hard timeout in seconds for acquire requests
+
+xfrm_enforce_policies_transport_mode - BOOLEAN
+	default 0 (disabled) - whether to enforce inbound policies for transport
+	mode SAs
\ No newline at end of file
diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 9da7982..e045ecc 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -64,6 +64,7 @@ struct netns_xfrm {
 	u32			sysctl_aevent_rseqth;
 	int			sysctl_larval_drop;
 	u32			sysctl_acq_expires;
+	int			sysctl_enforce_policies_transport_mode;
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header	*sysctl_hdr;
 #endif
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 55bcb86..5296f6b 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2355,20 +2355,22 @@ xfrm_state_ok(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x,
  * Otherwise "-2 - errored_index" is returned.
  */
 static inline int
-xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int start,
-	       unsigned short family)
+xfrm_policy_ok(const struct net *net, const struct xfrm_tmpl *tmpl,
+	       const struct sec_path *sp, int start, unsigned short family)
 {
 	int idx = start;
  	if (tmpl->optional) {
-		if (tmpl->mode == XFRM_MODE_TRANSPORT)
+		if (tmpl->mode == XFRM_MODE_TRANSPORT &&
+		    !net->xfrm.sysctl_enforce_policies_transport_mode)
 			return start;
 	} else
 		start = -1;
 	for (; idx < sp->len; idx++) {
 		if (xfrm_state_ok(tmpl, sp->xvec[idx], family))
 			return ++idx;
-		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT) {
+		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT ||
+		    net->xfrm.sysctl_enforce_policies_transport_mode) {
 			if (start == -1)
 				start = -2-idx;
 			break;
@@ -2393,10 +2395,13 @@ int __xfrm_decode_session(struct sk_buff *skb, struct flowi *fl,
 }
 EXPORT_SYMBOL(__xfrm_decode_session);
 -static inline int secpath_has_nontransport(const struct sec_path *sp, int k, int *idxp)
+static inline int secpath_has_nontransport(const struct net *net,
+					   const struct sec_path *sp,
+					   int k, int *idxp)
 {
 	for (; k < sp->len; k++) {
-		if (sp->xvec[k]->props.mode != XFRM_MODE_TRANSPORT) {
+		if (sp->xvec[k]->props.mode != XFRM_MODE_TRANSPORT ||
+		    net->xfrm.sysctl_enforce_policies_transport_mode) {
 			*idxp = k;
 			return 1;
 		}
@@ -2469,7 +2474,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	}
  	if (!pol) {
-		if (skb->sp && secpath_has_nontransport(skb->sp, 0, &xerr_idx)) {
+		if (skb->sp &&
+		    secpath_has_nontransport(net, skb->sp, 0, &xerr_idx)) {
 			xfrm_secpath_reject(xerr_idx, skb, &fl);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
 			return 0;
@@ -2535,7 +2541,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		 * are implied between each two transformations.
 		 */
 		for (i = xfrm_nr-1, k = 0; i >= 0; i--) {
-			k = xfrm_policy_ok(tpp[i], sp, k, family);
+			k = xfrm_policy_ok(net, tpp[i], sp, k, family);
 			if (k < 0) {
 				if (k < -1)
 					/* "-2 - errored_index" returned */
@@ -2545,7 +2551,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 			}
 		}
 -		if (secpath_has_nontransport(sp, k, &xerr_idx)) {
+		if (secpath_has_nontransport(net, sp, k, &xerr_idx)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINTMPLMISMATCH);
 			goto reject;
 		}
diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c
index 05a6e3d..17671af 100644
--- a/net/xfrm/xfrm_sysctl.c
+++ b/net/xfrm/xfrm_sysctl.c
@@ -9,6 +9,7 @@ static void __net_init __xfrm_sysctl_init(struct net *net)
 	net->xfrm.sysctl_aevent_rseqth = XFRM_AE_SEQT_SIZE;
 	net->xfrm.sysctl_larval_drop = 1;
 	net->xfrm.sysctl_acq_expires = 30;
+	net->xfrm.sysctl_enforce_policies_transport_mode = 0;
 }
  #ifdef CONFIG_SYSCTL
@@ -37,6 +38,12 @@ static struct ctl_table xfrm_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
+	{
+		.procname	= "xfrm_enforce_policies_transport_mode",
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
 	{}
 };
 @@ -53,6 +60,7 @@ int __net_init xfrm_sysctl_init(struct net *net)
 	table[1].data = &net->xfrm.sysctl_aevent_rseqth;
 	table[2].data = &net->xfrm.sysctl_larval_drop;
 	table[3].data = &net->xfrm.sysctl_acq_expires;
+	table[4].data = &net->xfrm.sysctl_enforce_policies_transport_mode;
  	/* Don't export sysctls to unprivileged users */
 	if (net->user_ns != &init_user_ns)
-- 
1.9.1

Re: [PATCH RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[Steffen Klassert]

Ccing Herbert Xu.

On Tue, Sep 16, 2014 at 12:49:39PM +0200, Tobias Brunner wrote:
> Currently inbound policies for transport mode SAs are not enforced.
> If no policy is found or if the templates don't match this is not
> considered an error for transport mode SAs.
> 

The strict inbound policy enforcement was implemented by Herbert.
The commit predates our git history but can be found in the history
tree:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

It was the following commit:

commit 8fe7ee2ba983fd89b2555dce5930ffd0f7f6c361
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu Oct 23 14:57:11 2003 -0700

    [IPSEC]: Strengthen policy checks.

Maybe Herbert remembers why this was done only for tunnel mode.

If I read section 5.2.1 of RFC 2401 correct, the inbound policy
must be enforced regardless of the mode.

> The new sysctl option (net.core.xfrm_enforce_policies_transport_mode)
> allows enforcing the inbound policies also for transport mode SAs.
> By default this option remains disabled.

I'd not like to have a sysctl for this. I consider it as a bug
if an installed policy can not be enforced, and we don't fix bugs
with sysctls :).

> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> ---
> Consider a transport mode SA between two peers over which only TCP and
> ICMP traffic should be allowed.  Because two protocols are involved the
> selector on the SA itself (xfrm_usersa_info.sel) can't be set.  Instead
> one either has to negotiate separate SAs for each protocol (with a
> selector on each) or negotiate one SA and install two policies that
> use it.  The problem with the latter is that the peer does not have to
> adhere to the negotiated traffic selectors, it is basically free to
> send anything over the SA e.g. UDP packets.  So if no selectors are
> installed on the SA itself, the current implementation would accept
> such packets, even though the installed policies don't allow UDP
> traffic (some might consider this a security issue - at the very least
> it is not very intuitive, especially because the behavior is different
> for tunnel mode SAs).
> By enabling the new sysctl option the UDP packets would get dropped,
> exactly as they would if the SA were negotiated in tunnel mode.
> 
> The behavior for optional transport mode templates also changes when
> the option is enabled.  Basically, the special treatment of transport
> mode SAs is disabled and the behavior is like it is for other modes.
> I tested this with IPComp/ESP and strongSwan where the optional IPComp
> transform is installed in transport or tunnel mode depending on the
> negotiated mode of the SA (the ESP SA is always installed in transport
> mode in this combination).  But I'm not sure if there are other uses
> for optional transport mode transforms that might rely on the current
> behavior (i.e. why are optional templates treated differently depending
> on their mode in the first place?).

The code arround xfrm_policy_ok() looks a bit obscure, I'm not sure if
all combinations of required and optional templates are handled correct.

> 
> With this patch the default behavior remains as it is, but I wondered
> why it is like that and if we could perhaps change it so that policies
> are enforced by default, thus making such setups more secure out of
> the box.  Or if we could even change the whole inbound processing so
> that transport mode SAs are treated exactly like their counterparts
> in other modes, without an option to change it.
> 
>  Documentation/networking/xfrm_sysctl.txt |  4 ++++
>  include/net/netns/xfrm.h                 |  1 +
>  net/xfrm/xfrm_policy.c                   | 24 +++++++++++++++---------
>  net/xfrm/xfrm_sysctl.c                   |  8 ++++++++
>  4 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/networking/xfrm_sysctl.txt b/Documentation/networking/xfrm_sysctl.txt
> index 5bbd167..2fbe539 100644
> --- a/Documentation/networking/xfrm_sysctl.txt
> +++ b/Documentation/networking/xfrm_sysctl.txt
> @@ -2,3 +2,7 @@
>   xfrm_acq_expires - INTEGER
>  	default 30 - hard timeout in seconds for acquire requests
> +
> +xfrm_enforce_policies_transport_mode - BOOLEAN
> +	default 0 (disabled) - whether to enforce inbound policies for transport
> +	mode SAs
> \ No newline at end of file
> diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
> index 9da7982..e045ecc 100644
> --- a/include/net/netns/xfrm.h
> +++ b/include/net/netns/xfrm.h
> @@ -64,6 +64,7 @@ struct netns_xfrm {
>  	u32			sysctl_aevent_rseqth;
>  	int			sysctl_larval_drop;
>  	u32			sysctl_acq_expires;
> +	int			sysctl_enforce_policies_transport_mode;
>  #ifdef CONFIG_SYSCTL
>  	struct ctl_table_header	*sysctl_hdr;
>  #endif
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 55bcb86..5296f6b 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -2355,20 +2355,22 @@ xfrm_state_ok(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x,
>   * Otherwise "-2 - errored_index" is returned.
>   */
>  static inline int
> -xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int start,
> -	       unsigned short family)
> +xfrm_policy_ok(const struct net *net, const struct xfrm_tmpl *tmpl,
> +	       const struct sec_path *sp, int start, unsigned short family)
>  {
>  	int idx = start;
>   	if (tmpl->optional) {
> -		if (tmpl->mode == XFRM_MODE_TRANSPORT)
> +		if (tmpl->mode == XFRM_MODE_TRANSPORT &&
> +		    !net->xfrm.sysctl_enforce_policies_transport_mode)
>  			return start;
>  	} else
>  		start = -1;
>  	for (; idx < sp->len; idx++) {
>  		if (xfrm_state_ok(tmpl, sp->xvec[idx], family))
>  			return ++idx;
> -		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT) {
> +		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT ||
> +		    net->xfrm.sysctl_enforce_policies_transport_mode) {
>  			if (start == -1)
>  				start = -2-idx;
>  			break;
> @@ -2393,10 +2395,13 @@ int __xfrm_decode_session(struct sk_buff *skb, struct flowi *fl,
>  }
>  EXPORT_SYMBOL(__xfrm_decode_session);
>  -static inline int secpath_has_nontransport(const struct sec_path *sp, int k, int *idxp)
> +static inline int secpath_has_nontransport(const struct net *net,
> +					   const struct sec_path *sp,
> +					   int k, int *idxp)
>  {
>  	for (; k < sp->len; k++) {
> -		if (sp->xvec[k]->props.mode != XFRM_MODE_TRANSPORT) {
> +		if (sp->xvec[k]->props.mode != XFRM_MODE_TRANSPORT ||
> +		    net->xfrm.sysctl_enforce_policies_transport_mode) {
>  			*idxp = k;
>  			return 1;
>  		}
> @@ -2469,7 +2474,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>  	}
>   	if (!pol) {
> -		if (skb->sp && secpath_has_nontransport(skb->sp, 0, &xerr_idx)) {
> +		if (skb->sp &&
> +		    secpath_has_nontransport(net, skb->sp, 0, &xerr_idx)) {
>  			xfrm_secpath_reject(xerr_idx, skb, &fl);
>  			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
>  			return 0;
> @@ -2535,7 +2541,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>  		 * are implied between each two transformations.
>  		 */
>  		for (i = xfrm_nr-1, k = 0; i >= 0; i--) {
> -			k = xfrm_policy_ok(tpp[i], sp, k, family);
> +			k = xfrm_policy_ok(net, tpp[i], sp, k, family);
>  			if (k < 0) {
>  				if (k < -1)
>  					/* "-2 - errored_index" returned */
> @@ -2545,7 +2551,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>  			}
>  		}
>  -		if (secpath_has_nontransport(sp, k, &xerr_idx)) {
> +		if (secpath_has_nontransport(net, sp, k, &xerr_idx)) {
>  			XFRM_INC_STATS(net, LINUX_MIB_XFRMINTMPLMISMATCH);
>  			goto reject;
>  		}
> diff --git a/net/xfrm/xfrm_sysctl.c b/net/xfrm/xfrm_sysctl.c
> index 05a6e3d..17671af 100644
> --- a/net/xfrm/xfrm_sysctl.c
> +++ b/net/xfrm/xfrm_sysctl.c
> @@ -9,6 +9,7 @@ static void __net_init __xfrm_sysctl_init(struct net *net)
>  	net->xfrm.sysctl_aevent_rseqth = XFRM_AE_SEQT_SIZE;
>  	net->xfrm.sysctl_larval_drop = 1;
>  	net->xfrm.sysctl_acq_expires = 30;
> +	net->xfrm.sysctl_enforce_policies_transport_mode = 0;
>  }
>   #ifdef CONFIG_SYSCTL
> @@ -37,6 +38,12 @@ static struct ctl_table xfrm_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec
>  	},
> +	{
> +		.procname	= "xfrm_enforce_policies_transport_mode",
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec
> +	},
>  	{}
>  };
>  @@ -53,6 +60,7 @@ int __net_init xfrm_sysctl_init(struct net *net)
>  	table[1].data = &net->xfrm.sysctl_aevent_rseqth;
>  	table[2].data = &net->xfrm.sysctl_larval_drop;
>  	table[3].data = &net->xfrm.sysctl_acq_expires;
> +	table[4].data = &net->xfrm.sysctl_enforce_policies_transport_mode;
>   	/* Don't export sysctls to unprivileged users */
>  	if (net->user_ns != &init_user_ns)
> -- 
> 1.9.1

Re: [PATCH RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[Tobias Brunner]

>> Currently inbound policies for transport mode SAs are not enforced.
>> If no policy is found or if the templates don't match this is not
>> considered an error for transport mode SAs.
>>
> 
> The strict inbound policy enforcement was implemented by Herbert.
> The commit predates our git history but can be found in the history
> tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> It was the following commit:
> 
> commit 8fe7ee2ba983fd89b2555dce5930ffd0f7f6c361
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu Oct 23 14:57:11 2003 -0700
> 
>     [IPSEC]: Strengthen policy checks.
> 
> Maybe Herbert remembers why this was done only for tunnel mode.

Would be great to hear from Herbert about this.

> If I read section 5.2.1 of RFC 2401 correct, the inbound policy
> must be enforced regardless of the mode.

It looks like the wording changed with RFC 4301.  The SPD and its
policies are not mentioned explicitly anymore in section 5.2 (like
they were in step 3 in section 5.2.1 of RFC 2401).  Instead, packets
must be matched against the "selectors identified by the SAD entry".
It's not entirely clear to me whether these selectors are part of the
SPD or properties of the SAD entries themselves, like the single
selector that can currently be configured on SAs in the kernel.
Also, section 4.4.2 explicitly states that manually keyed SAD entries
do not necessarily need to have a corresponding SPD entry.  Which might
make sense for simple host-host (transport mode) SAs, but this wouldn't
be possible anymore when enforcing inbound policies for transport mode.

>> The new sysctl option (net.core.xfrm_enforce_policies_transport_mode)
>> allows enforcing the inbound policies also for transport mode SAs.
>> By default this option remains disabled.
> 
> I'd not like to have a sysctl for this. I consider it as a bug
> if an installed policy can not be enforced, and we don't fix bugs
> with sysctls :).

Alright, the patch below simplifies the whole thing by treating
transport mode policies like those in other modes.

----

Subject: [PATCH] xfrm: Enforce inbound transport mode policies like those in other modes

Currently inbound policies for transport mode SAs are not enforced.
If no policy is found or if the templates don't match this is not
considered an error for transport mode SAs.

With these changes the inbound policies are also enforced for transport
mode SAs like they are already for other modes.

Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
 net/xfrm/xfrm_policy.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 55bcb8604bc6..74ce57e7b53b 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2348,9 +2348,8 @@ xfrm_state_ok(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x,
 }
 
 /*
- * 0 or more than 0 is returned when validation is succeeded (either bypass
- * because of optional transport mode, or next index of the mathced secpath
- * state with the template.
+ * 0 or more than 0 is returned when validation succeeded (next index of the
+ * matched secpath state with the template).
  * -1 is returned when no matching template is found.
  * Otherwise "-2 - errored_index" is returned.
  */
@@ -2360,19 +2359,13 @@ xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int star
 {
 	int idx = start;
 
-	if (tmpl->optional) {
-		if (tmpl->mode == XFRM_MODE_TRANSPORT)
-			return start;
-	} else
+	if (!tmpl->optional)
 		start = -1;
-	for (; idx < sp->len; idx++) {
+	if (idx < sp->len) {
 		if (xfrm_state_ok(tmpl, sp->xvec[idx], family))
 			return ++idx;
-		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT) {
-			if (start == -1)
-				start = -2-idx;
-			break;
-		}
+		if (start == -1)
+			start = -2-idx;
 	}
 	return start;
 }
@@ -2393,18 +2386,6 @@ int __xfrm_decode_session(struct sk_buff *skb, struct flowi *fl,
 }
 EXPORT_SYMBOL(__xfrm_decode_session);
 
-static inline int secpath_has_nontransport(const struct sec_path *sp, int k, int *idxp)
-{
-	for (; k < sp->len; k++) {
-		if (sp->xvec[k]->props.mode != XFRM_MODE_TRANSPORT) {
-			*idxp = k;
-			return 1;
-		}
-	}
-
-	return 0;
-}
-
 int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 			unsigned short family)
 {
@@ -2469,8 +2450,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	}
 
 	if (!pol) {
-		if (skb->sp && secpath_has_nontransport(skb->sp, 0, &xerr_idx)) {
-			xfrm_secpath_reject(xerr_idx, skb, &fl);
+		if (skb->sp && skb->sp->len) {
+			xfrm_secpath_reject(0, skb, &fl);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
 			return 0;
 		}
@@ -2545,7 +2526,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 			}
 		}
 
-		if (secpath_has_nontransport(sp, k, &xerr_idx)) {
+		if (k < sp->len) {
+			xerr_idx = k;
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINTMPLMISMATCH);
 			goto reject;
 		}
-- 
1.9.1

Re: [PATCH RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[Herbert Xu]

On Fri, Sep 19, 2014 at 11:24:38AM +0200, Steffen Klassert wrote:
> Ccing Herbert Xu.
> 
> On Tue, Sep 16, 2014 at 12:49:39PM +0200, Tobias Brunner wrote:
> > Currently inbound policies for transport mode SAs are not enforced.
> > If no policy is found or if the templates don't match this is not
> > considered an error for transport mode SAs.
> > 
> 
> The strict inbound policy enforcement was implemented by Herbert.
> The commit predates our git history but can be found in the history
> tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> It was the following commit:
> 
> commit 8fe7ee2ba983fd89b2555dce5930ffd0f7f6c361
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu Oct 23 14:57:11 2003 -0700
> 
>     [IPSEC]: Strengthen policy checks.
> 
> Maybe Herbert remembers why this was done only for tunnel mode.

Yes I remember :) Please refer to the following thread:

http://www.spinics.net/lists/linux-net/msg07342.html

Cheers,
-- 
Email: Herbert Xu <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 RFC ipsec-next] xfrm: Add sysctl option to enforce inbound policies for transport mode

[Steffen Klassert]

On Mon, Sep 29, 2014 at 03:39:31PM +0200, Tobias Brunner wrote:
> > 
> > commit 8fe7ee2ba983fd89b2555dce5930ffd0f7f6c361
> > Author: Herbert Xu <herbert@gondor.apana.org.au>
> > Date:   Thu Oct 23 14:57:11 2003 -0700
> > 
> >     [IPSEC]: Strengthen policy checks.
> > 
> > Maybe Herbert remembers why this was done only for tunnel mode.
> 
> Would be great to hear from Herbert about this.
> 
> > If I read section 5.2.1 of RFC 2401 correct, the inbound policy
> > must be enforced regardless of the mode.
> 
> It looks like the wording changed with RFC 4301.  

RFC 4301 did not yet exist when this code change was made,
so I tried to find the reason for that in RFC 2401.

> The SPD and its
> policies are not mentioned explicitly anymore in section 5.2 (like
> they were in step 3 in section 5.2.1 of RFC 2401).  Instead, packets
> must be matched against the "selectors identified by the SAD entry".
> It's not entirely clear to me whether these selectors are part of the
> SPD or properties of the SAD entries themselves, like the single
> selector that can currently be configured on SAs in the kernel.

It reads like the packets must match against the selectors of the
used SA. That's what we do at the beginning of __xfrm_policy_check().

This was already required in RFC 2401. But there was a matching policy
required too, seems this is not necessary anymore with RFC 4301.

Instead they recommend: "Every SPD SHOULD have a nominal, final entry
that catches anything that is otherwise unmatched, and discards it."

> Also, section 4.4.2 explicitly states that manually keyed SAD entries
> do not necessarily need to have a corresponding SPD entry.  Which might
> make sense for simple host-host (transport mode) SAs, but this wouldn't
> be possible anymore when enforcing inbound policies for transport mode.

Well, this states just that such SAD entries can exist. But does not
say that packets transformed by such a SA are allowed to pass without
matching policy.

> 
> Subject: [PATCH] xfrm: Enforce inbound transport mode policies like those in other modes
> 
> Currently inbound policies for transport mode SAs are not enforced.
> If no policy is found or if the templates don't match this is not
> considered an error for transport mode SAs.

If we find a policy, the templates should match. We need to fix this.
But it seems our policy enforcement for tunnel mode is too strict if
no policy is found. So I think we should leave transport mode as
it is here.