[PATCH v2 ipsec 0/2] xfrm: Fix bugs in stacked XFRM-I tunnels

[PATCH v2 ipsec 0/2] xfrm: Fix bugs in stacked XFRM-I tunnels

[Benedict Wong]

This patch set fixes bugs that prevent stacked IPsec tunnels (via XFRM
interfaces) from receiving packets properly. The apparent cause of the
issues is that the inner tunnel’s policy checks fail to validate the
outer tunnel’s secpath entries (since it no longer has a reference to
the outer tunnel policies, and each call validates ALL secpath entries)
before verifying the inner tunnel’s. This fixes this by caching the list
of verified secpath entries, and skipping them upon future validation
runs.

PATCH 1/2 Makes template matching for previously verified entries in the
secpath an optional match. This ensures that lost context for previous
tunnel secpath entries does not trigger a template mismatch if it was
previously verified. This allows each tunnel layer to incrementally
verify only the secpath entries associated with it.

PATCH 2/2 Ensures that policies for nested tunnel mode transforms are
verified (and marked as such) before additional decapsulation. This
ensures that entries in the secpath are verified while the context
(intermediate IP addresses, marks, etc) can be appropriately matched.
Notably, unencapsulated ESP did not perform policy checks before handing
to the next protocol for processing.

v1 -> v2:
- Reordered and rescoped patches to make changes clearer; policy
    check in xfrm_input necessary due to the incremental-verification
    process.
- Code style updates to conform with networking code formatting
- No net functional changes across both patches.

[PATCH v2 ipsec 1/2] xfrm: Skip checking of already-verified secpath entries

[Benedict Wong]

This change fixes a bug where inbound packets to nested IPsec tunnels
fails to pass policy checks due to the inner tunnel's policy checks
not having a reference to the outer policy/template. This causes the
policy check to fail, since the first entries in the secpath correlate
to the outer tunnel, while the templates being verified are for the
inner tunnel.

In order to ensure that the appropriate policy and template context is
searchable, the policy checks must be done incrementally between each
decryption step. As such, this marks secpath entries as having been
successfully matched, skipping them (treating as optional) on subsequent
policy checks

By skipping the immediate error return in the case where the secpath
entry had previously been validated, this change allows secpath entries
that matched a policy/template previously, while still requiring that
each searched template find a match in the secpath.

For security:
- All templates must have matching secpath entries
  - Unchanged by current patch; templates that do not match any secpath
    entry still return -1. This patch simply allows skipping earlier
    blocks of verified secpath entries
- All entries (except trailing transport mode entries) must have a
  matching template
  - Unvalidated entries, including transport-mode entries still return
    the errored index if it does not match the correct template.

Test: Tested against Android Kernel Unit Tests
Signed-off-by: Benedict Wong <benedictwong@google.com>
---
 include/net/xfrm.h     |  1 +
 net/xfrm/xfrm_input.c  |  1 +
 net/xfrm/xfrm_policy.c | 13 ++++++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index c39d910d4b45..a2f2840aba6b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1031,6 +1031,7 @@ struct xfrm_offload {
 struct sec_path {
 	int			len;
 	int			olen;
+	int			verified_cnt;
 
 	struct xfrm_state	*xvec[XFRM_MAX_DEPTH];
 	struct xfrm_offload	ovec[XFRM_MAX_OFFLOAD_DEPTH];
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 144238a50f3d..bcb9ee25474b 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -129,6 +129,7 @@ struct sec_path *secpath_set(struct sk_buff *skb)
 	memset(sp->ovec, 0, sizeof(sp->ovec));
 	sp->olen = 0;
 	sp->len = 0;
+	sp->verified_cnt = 0;
 
 	return sp;
 }
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f1a0bab920a5..71a5beebb6a0 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3261,7 +3261,7 @@ xfrm_state_ok(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x,
  */
 static inline int
 xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int start,
-	       unsigned short family)
+			   unsigned short family)
 {
 	int idx = start;
 
@@ -3274,6 +3274,13 @@ xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int star
 		if (xfrm_state_ok(tmpl, sp->xvec[idx], family))
 			return ++idx;
 		if (sp->xvec[idx]->props.mode != XFRM_MODE_TRANSPORT) {
+			if (idx < sp->verified_cnt) {
+				/* Secpath entry previously verified, consider optional and
+				 * continue searching
+				 */
+				continue;
+			}
+
 			if (start == -1)
 				start = -2-idx;
 			break;
@@ -3650,6 +3657,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		 * Order is _important_. Later we will implement
 		 * some barriers, but at the moment barriers
 		 * are implied between each two transformations.
+		 * Upon success, marks secpath entries as having been verified to allow
+		 * them to be skipped in future policy checks (e.g. nested tunnels).
 		 */
 		for (i = xfrm_nr-1, k = 0; i >= 0; i--) {
 			k = xfrm_policy_ok(tpp[i], sp, k, family);
@@ -3668,6 +3677,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 		}
 
 		xfrm_pols_put(pols, npols);
+		sp->verified_cnt = k;
+
 		return 1;
 	}
 	XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLBLOCK);
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Benedict Wong]

This change ensures that all nested XFRM packets have their policy
checked before decryption of the next layer, so that policies are
verified at each intermediate step of the decryption process.

Notably, raw ESP/AH packets do not perform policy checks inherently,
whereas all other encapsulated packets (UDP, TCP encapsulated) do policy
checks after calling xfrm_input handling in the respective encapsulation
layer.

This is necessary especially for nested tunnels, as the IP addresses,
protocol and ports may all change, thus not matching the previous
policies. In order to ensure that packets match the relevant inbound
templates, the xfrm_policy_check should be done before handing off to
the inner XFRM protocol to decrypt and decapsulate.

In order to prevent double-checking packets both here and in the
encapsulation layers, this check is currently limited to nested
tunnel-mode transforms and checked prior to decapsulation of inner
tunnel layers (prior to hitting a nested tunnel's xfrm_input, there
is no great way to detect a nested tunnel). This is primarily a
performance consideration, as a general blanket check at the end of
xfrm_input would suffice, but may result in multiple policy checks.

Test: Tested against Android Kernel Unit Tests
Signed-off-by: Benedict Wong <benedictwong@google.com>
---
 net/xfrm/xfrm_input.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index bcb9ee25474b..a3b55d109836 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			goto drop;
 		}
 
+		/* If nested tunnel, check outer states before context is lost.
+		 * Only nested tunnels need to be checked, since IP addresses change
+		 * as a result of the tunnel mode decapsulation. Similarly, this check
+		 * is limited to nested tunnels to avoid performing another policy
+		 * check on non-nested tunnels. On success, this check also updates the
+		 * secpath's verified_cnt variable, skipping future verifications of
+		 * previously-verified secpath entries.
+		 */
+		if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) &&
+		    sp->verified_cnt < sp->len &&
+		    !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) {
+			goto drop;
+		}
+
 		skb->mark = xfrm_smark_get(skb->mark, x);
 
 		sp->xvec[sp->len++] = x;
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Steffen Klassert]

On Wed, Aug 24, 2022 at 10:12:52PM +0000, Benedict Wong wrote:
> This change ensures that all nested XFRM packets have their policy
> checked before decryption of the next layer, so that policies are
> verified at each intermediate step of the decryption process.
> 
> Notably, raw ESP/AH packets do not perform policy checks inherently,
> whereas all other encapsulated packets (UDP, TCP encapsulated) do policy
> checks after calling xfrm_input handling in the respective encapsulation
> layer.
> 
> This is necessary especially for nested tunnels, as the IP addresses,
> protocol and ports may all change, thus not matching the previous
> policies. In order to ensure that packets match the relevant inbound
> templates, the xfrm_policy_check should be done before handing off to
> the inner XFRM protocol to decrypt and decapsulate.
> 
> In order to prevent double-checking packets both here and in the
> encapsulation layers, this check is currently limited to nested
> tunnel-mode transforms and checked prior to decapsulation of inner
> tunnel layers (prior to hitting a nested tunnel's xfrm_input, there
> is no great way to detect a nested tunnel). This is primarily a
> performance consideration, as a general blanket check at the end of
> xfrm_input would suffice, but may result in multiple policy checks.
> 
> Test: Tested against Android Kernel Unit Tests
> Signed-off-by: Benedict Wong <benedictwong@google.com>
> ---
>  net/xfrm/xfrm_input.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index bcb9ee25474b..a3b55d109836 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  			goto drop;
>  		}
>  
> +		/* If nested tunnel, check outer states before context is lost.
> +		 * Only nested tunnels need to be checked, since IP addresses change
> +		 * as a result of the tunnel mode decapsulation. Similarly, this check
> +		 * is limited to nested tunnels to avoid performing another policy
> +		 * check on non-nested tunnels. On success, this check also updates the
> +		 * secpath's verified_cnt variable, skipping future verifications of
> +		 * previously-verified secpath entries.
> +		 */
> +		if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) &&
> +		    sp->verified_cnt < sp->len &&
> +		    !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) {
> +			goto drop;
> +		}

This is not the right place to do the policy lookup. We don't know
if we should check XFRM_POLICY_IN or XFRM_POLICY_FWD.

But it looks like we don't reset the secpath in the receive path
like other virtual interfaces do.

Would such a patch fix your issue too?

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index cc6ab79609e2..429de6a28f59 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 	int xerr_idx = -1;
 	const struct xfrm_if_cb *ifcb;
 	struct sec_path *sp;
-	struct xfrm_if *xi;
+	struct xfrm_if *xi = NULL;
 	u32 if_id = 0;
 
 	rcu_read_lock();
@@ -3668,6 +3668,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
 			goto reject;
 		}
 
+		if (xi)
+			secpath_reset(skb);
+
 		xfrm_pols_put(pols, npols);
 		return 1;
 	}

Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Benedict Wong]

Thanks for the response; apologies for taking a while to re-patch this
and verify.

I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6
ESP tunnels failing; I think it's due to the fact that the IPv6 ESP
codepath does not trigger policy checks in the receive codepath until it
hits the socket, or changes namespace.

Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined
with your change above, this should ensure IPv6 ESP also checks policies,
and inside that clear the secpath?

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 5113fa0fbcee..4288d87c9249 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -236,23 +236,21 @@ static int xfrmi_rcv_cb(struct sk_buff *skb, int err)

        xnet = !net_eq(xi->net, dev_net(skb->dev));

-       if (xnet) {
-               inner_mode = &x->inner_mode;
-
-               if (x->sel.family == AF_UNSPEC) {
-                       inner_mode = xfrm_ip2inner_mode(x,
XFRM_MODE_SKB_CB(skb)->protocol);
-                       if (inner_mode == NULL) {
-                               XFRM_INC_STATS(dev_net(skb->dev),
-                                              LINUX_MIB_XFRMINSTATEMODEERROR);
-                               return -EINVAL;
-                       }
+       inner_mode = &x->inner_mode;
+
+       if (x->sel.family == AF_UNSPEC) {
+               inner_mode = xfrm_ip2inner_mode(x,
XFRM_MODE_SKB_CB(skb)->protocol);
+               if (inner_mode == NULL) {
+                       XFRM_INC_STATS(dev_net(skb->dev),
+                                               LINUX_MIB_XFRMINSTATEMODEERROR);
+                       return -EINVAL;
                }
-
-               if (!xfrm_policy_check(NULL, XFRM_POLICY_IN, skb,
-                                      inner_mode->family))
-                       return -EPERM;
        }

+       if (!xfrm_policy_check(NULL, XFRM_POLICY_IN, skb,
+                                       inner_mode->family))
+               return -EPERM;
+
        xfrmi_scrub_packet(skb, xnet);
        dev_sw_netstats_rx_add(dev, skb->len);

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f1a0bab920a5..04f66f6d5729 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int
dir, struct sk_buff *skb,
        int xerr_idx = -1;
        const struct xfrm_if_cb *ifcb;
        struct sec_path *sp;
-       struct xfrm_if *xi;
+       struct xfrm_if *xi = NULL;
        u32 if_id = 0;

        rcu_read_lock();
@@ -3667,6 +3667,9 @@ int __xfrm_policy_check(struct sock *sk, int



dir, struct sk_buff *skb,

                        goto reject;
                }

+               if (xi)
+                       secpath_reset(skb);
+
                xfrm_pols_put(pols, npols);
                return 1;
        }




On Mon, Aug 29, 2022 at 11:25 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:12:52PM +0000, Benedict Wong wrote:
> > This change ensures that all nested XFRM packets have their policy
> > checked before decryption of the next layer, so that policies are
> > verified at each intermediate step of the decryption process.
> >
> > Notably, raw ESP/AH packets do not perform policy checks inherently,
> > whereas all other encapsulated packets (UDP, TCP encapsulated) do policy
> > checks after calling xfrm_input handling in the respective encapsulation
> > layer.
> >
> > This is necessary especially for nested tunnels, as the IP addresses,
> > protocol and ports may all change, thus not matching the previous
> > policies. In order to ensure that packets match the relevant inbound
> > templates, the xfrm_policy_check should be done before handing off to
> > the inner XFRM protocol to decrypt and decapsulate.
> >
> > In order to prevent double-checking packets both here and in the
> > encapsulation layers, this check is currently limited to nested
> > tunnel-mode transforms and checked prior to decapsulation of inner
> > tunnel layers (prior to hitting a nested tunnel's xfrm_input, there
> > is no great way to detect a nested tunnel). This is primarily a
> > performance consideration, as a general blanket check at the end of
> > xfrm_input would suffice, but may result in multiple policy checks.
> >
> > Test: Tested against Android Kernel Unit Tests
> > Signed-off-by: Benedict Wong <benedictwong@google.com>
> > ---
> >  net/xfrm/xfrm_input.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > index bcb9ee25474b..a3b55d109836 100644
> > --- a/net/xfrm/xfrm_input.c
> > +++ b/net/xfrm/xfrm_input.c
> > @@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> >                       goto drop;
> >               }
> >
> > +             /* If nested tunnel, check outer states before context is lost.
> > +              * Only nested tunnels need to be checked, since IP addresses change
> > +              * as a result of the tunnel mode decapsulation. Similarly, this check
> > +              * is limited to nested tunnels to avoid performing another policy
> > +              * check on non-nested tunnels. On success, this check also updates the
> > +              * secpath's verified_cnt variable, skipping future verifications of
> > +              * previously-verified secpath entries.
> > +              */
> > +             if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) &&
> > +                 sp->verified_cnt < sp->len &&
> > +                 !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) {
> > +                     goto drop;
> > +             }
>
> This is not the right place to do the policy lookup. We don't know
> if we should check XFRM_POLICY_IN or XFRM_POLICY_FWD.
>
> But it looks like we don't reset the secpath in the receive path
> like other virtual interfaces do.
>
> Would such a patch fix your issue too?
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cc6ab79609e2..429de6a28f59 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>         int xerr_idx = -1;
>         const struct xfrm_if_cb *ifcb;
>         struct sec_path *sp;
> -       struct xfrm_if *xi;
> +       struct xfrm_if *xi = NULL;
>         u32 if_id = 0;
>
>         rcu_read_lock();
> @@ -3668,6 +3668,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
>                         goto reject;
>                 }
>
> +               if (xi)
> +                       secpath_reset(skb);
> +
>                 xfrm_pols_put(pols, npols);
>                 return 1;
>         }

Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Steffen Klassert]

On Fri, Sep 16, 2022 at 10:44:42PM -0700, Benedict Wong wrote:
> Thanks for the response; apologies for taking a while to re-patch this
> and verify.
> 
> I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6
> ESP tunnels failing; I think it's due to the fact that the IPv6 ESP
> codepath does not trigger policy checks in the receive codepath until it
> hits the socket, or changes namespace.
> Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined
> with your change above, this should ensure IPv6 ESP also checks policies,
> and inside that clear the secpath?

Hm, do you know why this is different to IPv4? IPv4 and IPv6 should
do the same regarding to policy checks.

Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Benedict Wong]

Ahh, I've never had an IPv4 server without a NAT to test against, I'd presume
this is identical there. The only comparison that I've been able to do  was IPv4
UDP-encap vs IPv6 ESP.

We could instead add the policy check to the ESP input path if that is
the correct place.


On Wed, Sep 21, 2022 at 11:27 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Fri, Sep 16, 2022 at 10:44:42PM -0700, Benedict Wong wrote:
> > Thanks for the response; apologies for taking a while to re-patch this
> > and verify.
> >
> > I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6
> > ESP tunnels failing; I think it's due to the fact that the IPv6 ESP
> > codepath does not trigger policy checks in the receive codepath until it
> > hits the socket, or changes namespace.
> > Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined
> > with your change above, this should ensure IPv6 ESP also checks policies,
> > and inside that clear the secpath?
>
> Hm, do you know why this is different to IPv4? IPv4 and IPv6 should
> do the same regarding to policy checks.
>

Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels

[Steffen Klassert]

On Thu, Sep 22, 2022 at 06:33:55PM -0700, Benedict Wong wrote:
> Ahh, I've never had an IPv4 server without a NAT to test against, I'd presume
> this is identical there. The only comparison that I've been able to do  was IPv4
> UDP-encap vs IPv6 ESP.
> 
> We could instead add the policy check to the ESP input path if that is
> the correct place.

Ok, looks like there is a policy check missing for xfrm_interfaces
when already one (or more) transformations happened.

The best would be to add a separate xfrm_interfaces rcv handler
(in struct xfrm6_protocol/xfrm4_protocol) for esp4/6 and do
the policy check if we have a secpath present.

That should fix it in combination with reseting the secpath in
the policy_check as I did in my previous patch.