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

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] [day] [month] [year] [list]

Message-ID: <20220815085001.GB2950045@gauss3.secunet.de>
Date:   Mon, 15 Aug 2022 10:50:01 +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 ipsec 2/2] xfrm: Skip checking of already-verified
 secpath entries

On Wed, Aug 10, 2022 at 06:22:10PM +0000, Benedict Wong wrote:
> 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 after each
> decryption step. As such, this marks secpath entries as having been
> successfully matched, skipping these 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@...gle.com>
> Change-Id: Ic32831cb00151d0de2e465f18ec37d5f7b680e54

This ID is meaningless on a mainline kernel, please remove it.

> ---
>  include/net/xfrm.h     |  1 +
>  net/xfrm/xfrm_input.c  |  3 ++-
>  net/xfrm/xfrm_policy.c | 11 ++++++++++-
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> 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 b24df8a44585..895935077a91 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;
>  }
> @@ -587,7 +588,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  
>  		// If nested tunnel, check outer states before context is lost.

Please use networking style comments here too.

>  		if (x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL
> -				&& sp->len > 0
> +				&& sp->len > sp->verified_cnt
>  				&& !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) {

As in the first patch, please use common networking code
alignment.

Thanks!