Re: [PATCH] xfrm: Zero padding when dumping algos and encap

[Sabrina Dubroca <sd@queasysnail.net>]

2023-02-08, 18:24:03 +0800, Herbert Xu wrote:
> On Wed, Feb 08, 2023 at 12:54:34AM -0800, Hyunwoo Kim wrote:
> > On Tue, Feb 07, 2023 at 02:04:43PM +0800, Herbert Xu wrote:
> > > On Sat, Feb 04, 2023 at 09:50:18AM -0800, Hyunwoo Kim wrote:
> > > > Since x->calg etc. are allocated with kmalloc, information
> > > > in kernel heap can be leaked using netlink socket on
> > > > systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> > > > 
> > Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> 
> Thanks.  This line should go into the patch description.
> 
> However, I don't think your patch is sufficient as xfrm_user
> does the same thing as af_key.
> 
> I think a better approach would be to not copy out past the
> end of string in copy_to_user_state_extra.  Something like
> this:

Do you mean as a replacement for Hyunwoo's patch, or that both are
needed? pfkey_msg2xfrm_state doesn't always initialize encap_sport and
encap_dport (and calg->alg_key_len, but you're not using that in
copy_to_user_calg), so I guess you mean both patches.

> ---8<---
> When copying data to user-space we should ensure that only valid
> data is copied over.  Padding in structures may be filled with
> random (possibly sensitve) data and should never be given directly
> to user-space.
> 
> This patch fixes the copying of xfrm algorithms and the encap
> template in xfrm_user so that padding is zeroed.
> 
> Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> Reported-by: Hyunwoo Kim <v4bel@theori.io>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index cf5172d4ce68..b5d50ae89840 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1012,7 +1012,9 @@ static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
>  		return -EMSGSIZE;
>  
>  	ap = nla_data(nla);
> -	memcpy(ap, aead, sizeof(*aead));
> +	strscpy_pad(ap->alg_name, aead->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = aead->alg_key_len;
> +	ap->alg_icv_len = aead->alg_icv_len;
>  
>  	if (redact_secret && aead->alg_key_len)
>  		memset(ap->alg_key, 0, (aead->alg_key_len + 7) / 8);
> @@ -1032,7 +1034,8 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
>  		return -EMSGSIZE;
>  
>  	ap = nla_data(nla);
> -	memcpy(ap, ealg, sizeof(*ealg));
> +	strscpy_pad(ap->alg_name, ealg->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = ealg->alg_key_len;
>  
>  	if (redact_secret && ealg->alg_key_len)
>  		memset(ap->alg_key, 0, (ealg->alg_key_len + 7) / 8);
> @@ -1043,6 +1046,40 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
>  	return 0;
>  }
>  
> +static int copy_to_user_calg(struct xfrm_algo *calg, struct sk_buff *skb)
> +{
> +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*calg));
> +	struct xfrm_algo *ap;
> +
> +	if (!nla)
> +		return -EMSGSIZE;
> +
> +	ap = nla_data(nla);
> +	strscpy_pad(ap->alg_name, calg->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = 0;
> +
> +	return 0;
> +}
> +
> +static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
> +{
> +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*ep));

XFRMA_ENCAP

> +	struct xfrm_encap_tmpl *uep;
> +
> +	if (!nla)
> +		return -EMSGSIZE;
> +
> +	uep = nla_data(nla);
> +	memset(uep, 0, sizeof(*uep));
> +
> +	uep->encap_type = ep->encap_type;
> +	uep->encap_sport = ep->encap_sport;
> +	uep->encap_dport = ep->encap_dport;
> +	uep->encap_oa = ep->encap_oa;

Should that be a memcpy? At least that's how xfrm_user.c usually does
copies of xfrm_address_t.

> +
> +	return 0;
> +}
> +
>  static int xfrm_smark_put(struct sk_buff *skb, struct xfrm_mark *m)
>  {
>  	int ret = 0;
> @@ -1098,12 +1135,12 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
>  			goto out;
>  	}
>  	if (x->calg) {
> -		ret = nla_put(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
> +		ret = copy_to_user_calg(x->calg, skb);
>  		if (ret)
>  			goto out;
>  	}
>  	if (x->encap) {
> -		ret = nla_put(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
> +		ret = copy_to_user_encap(x->encap, skb);
>  		if (ret)
>  			goto out;
>  	}
> -- 
> 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
> 

-- 
Sabrina