| Message ID | Y+N4Q2B01iRfXlQu@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: Zero padding when dumping algos and encap | expand |
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 >
On Wed, Feb 08, 2023 at 02:15:47PM +0100, Sabrina Dubroca wrote: > > 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. It's meant to be a replacement but yes we should still zero x->encap because that will leak out in other ways, e.g., on the wire. Hyunwoo, could you please repost your patch just for x->encap? > > +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 Good catch. I will repost the patch. > > + 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. It doesn't really matter. Thanks,
On Thu, Feb 09, 2023 at 07:08:57AM +0800, Herbert Xu wrote: > On Wed, Feb 08, 2023 at 02:15:47PM +0100, Sabrina Dubroca wrote: > > > > 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. > > It's meant to be a replacement but yes we should still zero x->encap > because that will leak out in other ways, e.g., on the wire. > > Hyunwoo, could you please repost your patch just for x->encap? Can the x->encap patch do this? I didn't add the syzbot hash as x->encap is not the flow reported by syzbot: Subject: [PATCH] af_key: Fix heap information leak Since x->encap of pfkey_msg2xfrm_state() is not initialized to 0, kernel heap data can be leaked. Fix with kzalloc() to prevent this. Signed-off-by: Hyunwoo Kim <v4bel@theori.io> --- net/key/af_key.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/key/af_key.c b/net/key/af_key.c index 2bdbcec781cd..a815f5ab4c49 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -1261,7 +1261,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net, const struct sadb_x_nat_t_type* n_type; struct xfrm_encap_tmpl *natt; - x->encap = kmalloc(sizeof(*x->encap), GFP_KERNEL); + x->encap = kzalloc(sizeof(*x->encap), GFP_KERNEL); if (!x->encap) { err = -ENOMEM; goto out; > > > > +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 > > Good catch. I will repost the patch. > > > > + 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. > > It doesn't really matter. > > Thanks, > -- > 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
On Wed, Feb 08, 2023 at 09:34:16PM -0800, Hyunwoo Kim wrote: > > Can the x->encap patch do this? Yes I think it should be sufficient. > Subject: [PATCH] af_key: Fix heap information leak > > Since x->encap of pfkey_msg2xfrm_state() is not > initialized to 0, kernel heap data can be leaked. > > Fix with kzalloc() to prevent this. > > Signed-off-by: Hyunwoo Kim <v4bel@theori.io> > --- > net/key/af_key.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks,
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)); + 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; + + 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; }