netdev - Re: [PATCH net v2 1/3] net/sched: act_ipt: add sanity checks on table name and hook locations

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAM0EoMk6_CMeOGG1KMjNthgUXnB-fHtz4U2Uje+aYpxXe9HuPA@mail.gmail.com>
Date: Thu, 8 Jun 2023 12:51:51 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, kuba@...nel.org, edumazet@...gle.com, 
	davem@...emloft.net, pabeni@...hat.com, xiyou.wangcong@...il.com, 
	jiri@...nulli.us, Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net v2 1/3] net/sched: act_ipt: add sanity checks on table
 name and hook locations

On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@...len.de> wrote:
>
> Looks like "tc" hard-codes "mangle" as the only supported table
> name, but on kernel side there are no checks.
>
> This is wrong.  Not all xtables targets are safe to call from tc.
> E.g. "nat" targets assume skb has a conntrack object assigned to it.
> Normally those get called from netfilter nat core which consults the
> nat table to obtain the address mapping.
>
> "tc" userspace either sets PRE or POSTROUTING as hook number, but there
> is no validation of this on kernel side, so update netlink policy to
> reject bogus numbers.  Some targets may assume skb_dst is set for
> input/forward hooks, so prevent those from being used.
>
> act_ipt uses the hook number in two places:
> 1. the state hook number, this is fine as-is
> 2. to set par.hook_mask
>
> The latter is a bit mask, so update the assignment to make
> xt_check_target() to the right thing.
>
> Followup patch adds required checks for the skb/packet headers before
> calling the targets evaluation function.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Florian Westphal <fw@...len.de>
> Reviewed-by: Simon Horman <simon.horman@...igine.com>

Acked-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal

> ---
>  v2: add Fixes tag, diff unchanged.
>
>  net/sched/act_ipt.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
> index 5d96ffebd40f..ea7f151e7dd2 100644
> --- a/net/sched/act_ipt.c
> +++ b/net/sched/act_ipt.c
> @@ -48,7 +48,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
>         par.entryinfo = &e;
>         par.target    = target;
>         par.targinfo  = t->data;
> -       par.hook_mask = hook;
> +       par.hook_mask = 1 << hook;
>         par.family    = NFPROTO_IPV4;
>
>         ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false);
> @@ -85,7 +85,8 @@ static void tcf_ipt_release(struct tc_action *a)
>
>  static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
>         [TCA_IPT_TABLE] = { .type = NLA_STRING, .len = IFNAMSIZ },
> -       [TCA_IPT_HOOK]  = { .type = NLA_U32 },
> +       [TCA_IPT_HOOK]  = NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING,
> +                                          NF_INET_NUMHOOKS),
>         [TCA_IPT_INDEX] = { .type = NLA_U32 },
>         [TCA_IPT_TARG]  = { .len = sizeof(struct xt_entry_target) },
>  };
> @@ -158,15 +159,27 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
>                         return -EEXIST;
>                 }
>         }
> +
> +       err = -EINVAL;
>         hook = nla_get_u32(tb[TCA_IPT_HOOK]);
> +       switch (hook) {
> +       case NF_INET_PRE_ROUTING:
> +               break;
> +       case NF_INET_POST_ROUTING:
> +               break;
> +       default:
> +               goto err1;
> +       }
> +
> +       if (tb[TCA_IPT_TABLE]) {
> +               /* mangle only for now */
> +               if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle"))
> +                       goto err1;
> +       }
>
> -       err = -ENOMEM;
> -       tname = kmalloc(IFNAMSIZ, GFP_KERNEL);
> +       tname = kstrdup("mangle", GFP_KERNEL);
>         if (unlikely(!tname))
>                 goto err1;
> -       if (tb[TCA_IPT_TABLE] == NULL ||
> -           nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ)
> -               strcpy(tname, "mangle");
>
>         t = kmemdup(td, td->u.target_size, GFP_KERNEL);
>         if (unlikely(!t))
> --
> 2.40.1
>