[PATCH ipsec] xfrm: Ensure consistent address families when resolving templates

[PATCH ipsec] xfrm: Ensure consistent address families when resolving templates

[Tobias Brunner]

xfrm_state_find() uses `encap_family` of the current template with
the passed local and remote addresses to find a matching state.
This check makes sure that there is no mismatch and out-of-bounds
read in mixed-family scenarios where optional tunnel or BEET mode
templates were skipped that would have changed the addresses to
match the current template's family.

This basically enforces the same check as validate_tmpl(), just at
runtime when one or more optional templates might have been skipped.

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

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 62be042f2ebc..e6dfa55f1c3a 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2440,6 +2440,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
  	struct net *net = xp_net(policy);
  	int nx;
  	int i, error;
+	unsigned short prev_family = family;
  	xfrm_address_t *daddr = xfrm_flowi_daddr(fl, family);
  	xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family);
  	xfrm_address_t tmp;
@@ -2462,6 +2463,9 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
  					goto fail;
  				local = &tmp;
  			}
+		} else if (prev_family != tmpl->encap_family) {
+			error = -EINVAL;
+			goto fail;
  		}
  
  		x = xfrm_state_find(remote, local, fl, tmpl, policy, &error,
@@ -2471,6 +2475,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
  			xfrm[nx++] = x;
  			daddr = remote;
  			saddr = local;
+			prev_family = tmpl->encap_family;
  			continue;
  		}
  		if (x) {
-- 
2.34.1

Re: [PATCH ipsec] xfrm: Ensure consistent address families when resolving templates

[Herbert Xu]

On Mon, Apr 24, 2023 at 03:23:02PM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> This check makes sure that there is no mismatch and out-of-bounds
> read in mixed-family scenarios where optional tunnel or BEET mode
> templates were skipped that would have changed the addresses to
> match the current template's family.
> 
> This basically enforces the same check as validate_tmpl(), just at
> runtime when one or more optional templates might have been skipped.
> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> ---
>  net/xfrm/xfrm_policy.c | 5 +++++
>  1 file changed, 5 insertions(+)

I'm confused.  By skipping, you're presumably referring to IPcomp.

For IPcomp, skipping should only occur on inbound, but your patch
is changing a code path that's only invoked for outbound.  What's
going on?

Thanks,
-- 
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 ipsec] xfrm: Ensure consistent address families when resolving templates

[Steffen Klassert]

On Tue, Apr 25, 2023 at 01:34:44PM +0800, Herbert Xu wrote:
> On Mon, Apr 24, 2023 at 03:23:02PM +0200, Tobias Brunner wrote:
> > xfrm_state_find() uses `encap_family` of the current template with
> > the passed local and remote addresses to find a matching state.
> > This check makes sure that there is no mismatch and out-of-bounds
> > read in mixed-family scenarios where optional tunnel or BEET mode
> > templates were skipped that would have changed the addresses to
> > match the current template's family.
> > 
> > This basically enforces the same check as validate_tmpl(), just at
> > runtime when one or more optional templates might have been skipped.
> > 
> > Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> > ---
> >  net/xfrm/xfrm_policy.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> I'm confused.  By skipping, you're presumably referring to IPcomp.
> 
> For IPcomp, skipping should only occur on inbound, but your patch
> is changing a code path that's only invoked for outbound.  What's
> going on?

The problem is, that you can configure it for outbound too.
Even though, it does not make much sense. syzbot reported
a stack-out-of-bounds issue with intermediate optional
templates that change the address family:

https://www.spinics.net/lists/netdev/msg890567.html

I tried to fix this by rejecting such a configuration:

https://lore.kernel.org/netdev/ZCZ79IlUW53XxaVr@gauss3.secunet.de/T/

This broke some strongswan configurations.

Tobias patch is the next attempt to fix that.

Re: [PATCH ipsec] xfrm: Ensure consistent address families when resolving templates

[Herbert Xu]

On Tue, Apr 25, 2023 at 08:47:15AM +0200, Steffen Klassert wrote:
>
> The problem is, that you can configure it for outbound too.
> Even though, it does not make much sense. syzbot reported
> a stack-out-of-bounds issue with intermediate optional
> templates that change the address family:

Does anyone actually use this in the real world?

If not we should try to restrict its usage rather than supporting
pointless features.

I think it should be safe to limit the use of optional transforms
on outbound policies to transport mode only.   You can then easily
verify the sanity of the policy in xfrm_user.

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 ipsec] xfrm: Ensure consistent address families when resolving templates

[Tobias Brunner]

Hi Herbert,

> I'm confused.  By skipping, you're presumably referring to IPcomp.
> 
> For IPcomp, skipping should only occur on inbound, but your patch
> is changing a code path that's only invoked for outbound.  What's
> going on?

At least in theory, there could be applications for optional outbound 
templates, e.g. an optional ESP transform that's only applied to some of 
the traffic matching the policy (based on the selector on the state, 
which is matched against the original flow) followed by a mandatory AH 
transform (there could even be multiple optional transforms, e.g. using 
different algorithms, that are selectively applied to traffic).  No idea 
if anybody actually uses this, but the API allows configuring it.  And 
syzbot showed that some combinations are problematic.

Regards,
Tobias

Re: [PATCH ipsec] xfrm: Ensure consistent address families when resolving templates

[Herbert Xu]

On Tue, Apr 25, 2023 at 10:00:32AM +0200, Tobias Brunner wrote:
> 
> At least in theory, there could be applications for optional outbound
> templates, e.g. an optional ESP transform that's only applied to some of the
> traffic matching the policy (based on the selector on the state, which is
> matched against the original flow) followed by a mandatory AH transform
> (there could even be multiple optional transforms, e.g. using different
> algorithms, that are selectively applied to traffic).  No idea if anybody
> actually uses this, but the API allows configuring it.  And syzbot showed
> that some combinations are problematic.

OK, if nobody actually is using this we should restrict its usage.

How about limiting optional transforms on outbound policies to
transport mode only?

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

[PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Tobias Brunner]

xfrm_state_find() uses `encap_family` of the current template with
the passed local and remote addresses to find a matching state.
If an optional tunnel or BEET mode template is skipped in a mixed-family
scenario, there could be a mismatch causing an out-of-bounds read as
the addresses were not replaced to match the family of the next template.

While there are theoretical use cases for optional templates in outbound
policies, the only practical one is to skip IPComp states in inbound
policies if uncompressed packets are received that are handled by an
implicitly created IPIP state instead.

Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
  net/xfrm/xfrm_user.c | 14 +++++++++-----
  1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index af8fbcbfbe69..6794b9dea27a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1768,7 +1768,7 @@ static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,
  }
  
  static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
-			 struct netlink_ext_ack *extack)
+			 int dir, struct netlink_ext_ack *extack)
  {
  	u16 prev_family;
  	int i;
@@ -1794,6 +1794,10 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
  		switch (ut[i].mode) {
  		case XFRM_MODE_TUNNEL:
  		case XFRM_MODE_BEET:
+			if (ut[i].optional && dir == XFRM_POLICY_OUT) {
+				NL_SET_ERR_MSG(extack, "Mode in optional template not allowed in outbound policy");
+				return -EINVAL;
+			}
  			break;
  		default:
  			if (ut[i].family != prev_family) {
@@ -1831,7 +1835,7 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
  }
  
  static int copy_from_user_tmpl(struct xfrm_policy *pol, struct nlattr **attrs,
-			       struct netlink_ext_ack *extack)
+			       int dir, struct netlink_ext_ack *extack)
  {
  	struct nlattr *rt = attrs[XFRMA_TMPL];
  
@@ -1842,7 +1846,7 @@ static int copy_from_user_tmpl(struct xfrm_policy *pol, struct nlattr **attrs,
  		int nr = nla_len(rt) / sizeof(*utmpl);
  		int err;
  
-		err = validate_tmpl(nr, utmpl, pol->family, extack);
+		err = validate_tmpl(nr, utmpl, pol->family, dir, extack);
  		if (err)
  			return err;
  
@@ -1919,7 +1923,7 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net,
  	if (err)
  		goto error;
  
-	if (!(err = copy_from_user_tmpl(xp, attrs, extack)))
+	if (!(err = copy_from_user_tmpl(xp, attrs, p->dir, extack)))
  		err = copy_from_user_sec_ctx(xp, attrs);
  	if (err)
  		goto error;
@@ -3498,7 +3502,7 @@ static struct xfrm_policy *xfrm_compile_policy(struct sock *sk, int opt,
  		return NULL;
  
  	nr = ((len - sizeof(*p)) / sizeof(*ut));
-	if (validate_tmpl(nr, ut, p->sel.family, NULL))
+	if (validate_tmpl(nr, ut, p->sel.family, p->dir, NULL))
  		return NULL;
  
  	if (p->dir > XFRM_POLICY_OUT)
-- 
2.34.1

Re: [PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Herbert Xu]

On Fri, May 05, 2023 at 12:16:16PM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> ---
>  net/xfrm/xfrm_user.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

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

However, I think a similar check needs to be added to af_key.c
as it too seems to allow optional outbound tunnel-mode templates.

Thanks,
-- 
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

[PATCH ipsec] af_key: Reject optional tunnel/BEET mode templates in outbound policies

[Tobias Brunner]

xfrm_state_find() uses `encap_family` of the current template with
the passed local and remote addresses to find a matching state.
If an optional tunnel or BEET mode template is skipped in a mixed-family
scenario, there could be a mismatch causing an out-of-bounds read as
the addresses were not replaced to match the family of the next template.

While there are theoretical use cases for optional templates in outbound
policies, the only practical one is to skip IPComp states in inbound
policies if uncompressed packets are received that are handled by an
implicitly created IPIP state instead.

Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
  net/key/af_key.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index a815f5ab4c49..31ab12fd720a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1940,7 +1940,8 @@ static u32 gen_reqid(struct net *net)
  }
  
  static int
-parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
+parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_policy *pol,
+		   struct sadb_x_ipsecrequest *rq)
  {
  	struct net *net = xp_net(xp);
  	struct xfrm_tmpl *t = xp->xfrm_vec + xp->xfrm_nr;
@@ -1958,9 +1959,12 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
  	if ((mode = pfkey_mode_to_xfrm(rq->sadb_x_ipsecrequest_mode)) < 0)
  		return -EINVAL;
  	t->mode = mode;
-	if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_USE)
+	if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_USE) {
+		if ((mode == XFRM_MODE_TUNNEL || mode == XFRM_MODE_BEET) &&
+		    pol->sadb_x_policy_dir == IPSEC_DIR_OUTBOUND)
+			return -EINVAL;
  		t->optional = 1;
-	else if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_UNIQUE) {
+	} else if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_UNIQUE) {
  		t->reqid = rq->sadb_x_ipsecrequest_reqid;
  		if (t->reqid > IPSEC_MANUAL_REQID_MAX)
  			t->reqid = 0;
@@ -2002,7 +2006,7 @@ parse_ipsecrequests(struct xfrm_policy *xp, struct sadb_x_policy *pol)
  		    rq->sadb_x_ipsecrequest_len < sizeof(*rq))
  			return -EINVAL;
  
-		if ((err = parse_ipsecrequest(xp, rq)) < 0)
+		if ((err = parse_ipsecrequest(xp, pol, rq)) < 0)
  			return err;
  		len -= rq->sadb_x_ipsecrequest_len;
  		rq = (void*)((u8*)rq + rq->sadb_x_ipsecrequest_len);
-- 
2.34.1

Re: [PATCH ipsec] af_key: Reject optional tunnel/BEET mode templates in outbound policies

[Herbert Xu]

On Fri, May 05, 2023 at 01:36:15PM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> ---
>  net/key/af_key.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

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

Thanks,
-- 
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 ipsec] af_key: Reject optional tunnel/BEET mode templates in outbound policies

[Steffen Klassert]

On Fri, May 05, 2023 at 01:36:15PM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>

Same corruption with this patch, and please add a 'Fixes' tag here too.

Thanks!

[PATCH ipsec] af_key: Reject optional tunnel/BEET mode templates in outbound policies

[Tobias Brunner]

xfrm_state_find() uses `encap_family` of the current template with
the passed local and remote addresses to find a matching state.
If an optional tunnel or BEET mode template is skipped in a mixed-family
scenario, there could be a mismatch causing an out-of-bounds read as
the addresses were not replaced to match the family of the next template.

While there are theoretical use cases for optional templates in outbound
policies, the only practical one is to skip IPComp states in inbound
policies if uncompressed packets are received that are handled by an
implicitly created IPIP state instead.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/key/af_key.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index a815f5ab4c49..31ab12fd720a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1940,7 +1940,8 @@ static u32 gen_reqid(struct net *net)
 }
 
 static int
-parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
+parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_policy *pol,
+		   struct sadb_x_ipsecrequest *rq)
 {
 	struct net *net = xp_net(xp);
 	struct xfrm_tmpl *t = xp->xfrm_vec + xp->xfrm_nr;
@@ -1958,9 +1959,12 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
 	if ((mode = pfkey_mode_to_xfrm(rq->sadb_x_ipsecrequest_mode)) < 0)
 		return -EINVAL;
 	t->mode = mode;
-	if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_USE)
+	if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_USE) {
+		if ((mode == XFRM_MODE_TUNNEL || mode == XFRM_MODE_BEET) &&
+		    pol->sadb_x_policy_dir == IPSEC_DIR_OUTBOUND)
+			return -EINVAL;
 		t->optional = 1;
-	else if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_UNIQUE) {
+	} else if (rq->sadb_x_ipsecrequest_level == IPSEC_LEVEL_UNIQUE) {
 		t->reqid = rq->sadb_x_ipsecrequest_reqid;
 		if (t->reqid > IPSEC_MANUAL_REQID_MAX)
 			t->reqid = 0;
@@ -2002,7 +2006,7 @@ parse_ipsecrequests(struct xfrm_policy *xp, struct sadb_x_policy *pol)
 		    rq->sadb_x_ipsecrequest_len < sizeof(*rq))
 			return -EINVAL;
 
-		if ((err = parse_ipsecrequest(xp, rq)) < 0)
+		if ((err = parse_ipsecrequest(xp, pol, rq)) < 0)
 			return err;
 		len -= rq->sadb_x_ipsecrequest_len;
 		rq = (void*)((u8*)rq + rq->sadb_x_ipsecrequest_len);
-- 
2.34.1

Re: [PATCH ipsec] af_key: Reject optional tunnel/BEET mode templates in outbound policies

[Steffen Klassert]

On Tue, May 09, 2023 at 11:00:06AM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Also applied, thanks a lot!

Re: [PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Steffen Klassert]

On Fri, May 05, 2023 at 12:16:16PM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>

Your patch seems to be corrupt:

warning: Patch sent with format=flowed; space at the end of lines might be lost.
Applying: af_key: Reject optional tunnel/BEET mode templates in outbound policies
error: corrupt patch at line 18

Also, please add a 'Fixes' tag, so that it can be backported.

Thanks!

Re: [PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Tobias Brunner]

On 08.05.23 07:59, Steffen Klassert wrote:
> On Fri, May 05, 2023 at 12:16:16PM +0200, Tobias Brunner wrote:
>> xfrm_state_find() uses `encap_family` of the current template with
>> the passed local and remote addresses to find a matching state.
>> If an optional tunnel or BEET mode template is skipped in a mixed-family
>> scenario, there could be a mismatch causing an out-of-bounds read as
>> the addresses were not replaced to match the family of the next template.
>>
>> While there are theoretical use cases for optional templates in outbound
>> policies, the only practical one is to skip IPComp states in inbound
>> policies if uncompressed packets are received that are handled by an
>> implicitly created IPIP state instead.
>>
>> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> 
> Your patch seems to be corrupt:
> 
> warning: Patch sent with format=flowed; space at the end of lines might be lost.
> Applying: af_key: Reject optional tunnel/BEET mode templates in outbound policies
> error: corrupt patch at line 18

Sorry about that, I'll resend.

> Also, please add a 'Fixes' tag, so that it can be backported.

What should the target commit be?  I saw you used 1da177e4c3f4
("Linux-2.6.12-rc2") in your fix, but maybe the more recent 8444cf712c5f
("xfrm: Allow different selector family in temporary state") would fit
better as that changed `family` to `encap_family` in
`xfrm_state_find()`?  (I guess it doesn't matter in practice as 2.6.36
is way older than any LTS kernel this will get backported to.)

Regards,
Tobias

Re: [PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Steffen Klassert]

On Mon, May 08, 2023 at 11:03:36AM +0200, Tobias Brunner wrote:
> On 08.05.23 07:59, Steffen Klassert wrote:
> > On Fri, May 05, 2023 at 12:16:16PM +0200, Tobias Brunner wrote:
> >> xfrm_state_find() uses `encap_family` of the current template with
> >> the passed local and remote addresses to find a matching state.
> >> If an optional tunnel or BEET mode template is skipped in a mixed-family
> >> scenario, there could be a mismatch causing an out-of-bounds read as
> >> the addresses were not replaced to match the family of the next template.
> >>
> >> While there are theoretical use cases for optional templates in outbound
> >> policies, the only practical one is to skip IPComp states in inbound
> >> policies if uncompressed packets are received that are handled by an
> >> implicitly created IPIP state instead.
> >>
> >> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> > 
> > Your patch seems to be corrupt:
> > 
> > warning: Patch sent with format=flowed; space at the end of lines might be lost.
> > Applying: af_key: Reject optional tunnel/BEET mode templates in outbound policies
> > error: corrupt patch at line 18
> 
> Sorry about that, I'll resend.
> 
> > Also, please add a 'Fixes' tag, so that it can be backported.
> 
> What should the target commit be?  I saw you used 1da177e4c3f4
> ("Linux-2.6.12-rc2") in your fix, but maybe the more recent 8444cf712c5f
> ("xfrm: Allow different selector family in temporary state") would fit
> better as that changed `family` to `encap_family` in
> `xfrm_state_find()`?  (I guess it doesn't matter in practice as 2.6.36
> is way older than any LTS kernel this will get backported to.)

I think it was broken, even before 8444cf712c5f "xfrm: Allow different
selector family in temporary state"), so I used 1da177e4c3f4.
But as you said, it doesn't really matter. Both commits are
much older than any currently active LTS kernel.

[PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Tobias Brunner]

xfrm_state_find() uses `encap_family` of the current template with
the passed local and remote addresses to find a matching state.
If an optional tunnel or BEET mode template is skipped in a mixed-family
scenario, there could be a mismatch causing an out-of-bounds read as
the addresses were not replaced to match the family of the next template.

While there are theoretical use cases for optional templates in outbound
policies, the only practical one is to skip IPComp states in inbound
policies if uncompressed packets are received that are handled by an
implicitly created IPIP state instead.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/xfrm/xfrm_user.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index af8fbcbfbe69..6794b9dea27a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1768,7 +1768,7 @@ static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,
 }
 
 static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
-			 struct netlink_ext_ack *extack)
+			 int dir, struct netlink_ext_ack *extack)
 {
 	u16 prev_family;
 	int i;
@@ -1794,6 +1794,10 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
 		switch (ut[i].mode) {
 		case XFRM_MODE_TUNNEL:
 		case XFRM_MODE_BEET:
+			if (ut[i].optional && dir == XFRM_POLICY_OUT) {
+				NL_SET_ERR_MSG(extack, "Mode in optional template not allowed in outbound policy");
+				return -EINVAL;
+			}
 			break;
 		default:
 			if (ut[i].family != prev_family) {
@@ -1831,7 +1835,7 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family,
 }
 
 static int copy_from_user_tmpl(struct xfrm_policy *pol, struct nlattr **attrs,
-			       struct netlink_ext_ack *extack)
+			       int dir, struct netlink_ext_ack *extack)
 {
 	struct nlattr *rt = attrs[XFRMA_TMPL];
 
@@ -1842,7 +1846,7 @@ static int copy_from_user_tmpl(struct xfrm_policy *pol, struct nlattr **attrs,
 		int nr = nla_len(rt) / sizeof(*utmpl);
 		int err;
 
-		err = validate_tmpl(nr, utmpl, pol->family, extack);
+		err = validate_tmpl(nr, utmpl, pol->family, dir, extack);
 		if (err)
 			return err;
 
@@ -1919,7 +1923,7 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net,
 	if (err)
 		goto error;
 
-	if (!(err = copy_from_user_tmpl(xp, attrs, extack)))
+	if (!(err = copy_from_user_tmpl(xp, attrs, p->dir, extack)))
 		err = copy_from_user_sec_ctx(xp, attrs);
 	if (err)
 		goto error;
@@ -3498,7 +3502,7 @@ static struct xfrm_policy *xfrm_compile_policy(struct sock *sk, int opt,
 		return NULL;
 
 	nr = ((len - sizeof(*p)) / sizeof(*ut));
-	if (validate_tmpl(nr, ut, p->sel.family, NULL))
+	if (validate_tmpl(nr, ut, p->sel.family, p->dir, NULL))
 		return NULL;
 
 	if (p->dir > XFRM_POLICY_OUT)
-- 
2.34.1

Re: [PATCH ipsec] xfrm: Reject optional tunnel/BEET mode templates in outbound policies

[Steffen Klassert]

On Tue, May 09, 2023 at 10:59:58AM +0200, Tobias Brunner wrote:
> xfrm_state_find() uses `encap_family` of the current template with
> the passed local and remote addresses to find a matching state.
> If an optional tunnel or BEET mode template is skipped in a mixed-family
> scenario, there could be a mismatch causing an out-of-bounds read as
> the addresses were not replaced to match the family of the next template.
> 
> While there are theoretical use cases for optional templates in outbound
> policies, the only practical one is to skip IPComp states in inbound
> policies if uncompressed packets are received that are handled by an
> implicitly created IPIP state instead.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Tobias Brunner <tobias@strongswan.org>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks a lot Tobias!