[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y+Oggx0YBA3kLLcw@hog>
Date: Wed, 8 Feb 2023 14:15:47 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Hyunwoo Kim <v4bel@...ori.io>, steffen.klassert@...unet.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, imv4bel@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] xfrm: Zero padding when dumping algos and encap
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@...kaller.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@...kaller.appspotmail.com
> Reported-by: Hyunwoo Kim <v4bel@...ori.io>
> Signed-off-by: Herbert Xu <herbert@...dor.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@...dor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
--
Sabrina
Powered by blists - more mailing lists