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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220830062529.GM2950045@gauss3.secunet.de>
Date:   Tue, 30 Aug 2022 08:25:29 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Benedict Wong <benedictwong@...gle.com>
CC:     <netdev@...r.kernel.org>, <nharold@...gle.com>,
        <lorenzo@...gle.com>
Subject: Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP
 tunnels

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@...gle.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;
 	}