| Message ID | 20200116211109.9119-1-fw@strlen.de |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: nf_tables: check for valid chain type pointer before dereference | expand |
On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > Its possible to create tables in a family that isn't supported/known. > Then, when adding a base chain, the table pointer can be NULL. > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/netfilter/nf_tables_api.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 65f51a2e9c2a..e8976128cdb1 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > struct nft_ctx ctx; > int err; > > + if (family >= NFPROTO_NUMPROTO) > + return -EAFNOSUPPORT; > + > lockdep_assert_held(&net->nft.commit_mutex); > attr = nla[NFTA_TABLE_NAME]; > table = nft_table_lookup(net, attr, family, genmask); > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > ha[NFTA_HOOK_PRIORITY] == NULL) > return -EINVAL; > > + if (family >= NFPROTO_NUMPROTO) > + return -EAFNOSUPPORT; > + > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > family, autoload); > if (IS_ERR(type)) > return PTR_ERR(type); > + } else if (!type) { > + return -EOPNOTSUPP; I think this check should be enough. I mean, NFPROTO_NUMPROTO still allows for creating tables for families that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such table. As long as such table does not crash the kernel, I think it's fine. No changes can be attached anymore anyway. Otherwise, if a helper function to check for the families that are really supported could be another alternative. But not sure it is worth? Let me know, thanks.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > > Its possible to create tables in a family that isn't supported/known. > > Then, when adding a base chain, the table pointer can be NULL. > > > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/netfilter/nf_tables_api.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 65f51a2e9c2a..e8976128cdb1 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > > struct nft_ctx ctx; > > int err; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > lockdep_assert_held(&net->nft.commit_mutex); > > attr = nla[NFTA_TABLE_NAME]; > > table = nft_table_lookup(net, attr, family, genmask); > > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > > ha[NFTA_HOOK_PRIORITY] == NULL) > > return -EINVAL; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > > family, autoload); > > if (IS_ERR(type)) > > return PTR_ERR(type); > > + } else if (!type) { > > + return -EOPNOTSUPP; > > I think this check should be enough. > > I mean, NFPROTO_NUMPROTO still allows for creating tables for families > that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such > table. As long as such table does not crash the kernel, I think it's > fine. No changes can be attached anymore anyway. I had a previous (not-sent) version that added a stronger validation of nfproto during table creation but I ditched it because it got ugly due to ifdefs. As you alrady point out, creating "bogus" family tables is fine, base chain registration will fail later on from netfilter core. The NFPROTO_NUMPROTO range check is needed, we use it as index to table[] in nft_chain_parse_hook(). > Otherwise, if a helper function to check for the families that are > really supported could be another alternative. But not sure it is > worth? It did not look nice so I did not go for it in the end, but I think doing a simple range check doesn't hurt.
On Sat, Jan 18, 2020 at 09:30:57PM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > > Its possible to create tables in a family that isn't supported/known. > > Then, when adding a base chain, the table pointer can be NULL. > > > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/netfilter/nf_tables_api.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 65f51a2e9c2a..e8976128cdb1 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > > struct nft_ctx ctx; > > int err; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > lockdep_assert_held(&net->nft.commit_mutex); > > attr = nla[NFTA_TABLE_NAME]; > > table = nft_table_lookup(net, attr, family, genmask); > > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > > ha[NFTA_HOOK_PRIORITY] == NULL) > > return -EINVAL; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > > family, autoload); > > if (IS_ERR(type)) > > return PTR_ERR(type); > > + } else if (!type) { > > + return -EOPNOTSUPP; > > I think this check should be enough. > > I mean, NFPROTO_NUMPROTO still allows for creating tables for families > that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such > table. As long as such table does not crash the kernel, I think it's > fine. No changes can be attached anymore anyway. > > Otherwise, if a helper function to check for the families that are > really supported could be another alternative. But not sure it is > worth? Not worth. Probably this patch instead? Just make sure that access to the chain type array is safe, no direct access to chain_type[][] anymore. This includes the check for the default type too, since it cannot be assume to always have a filter chain for unsupported families. Thanks for explaining.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Otherwise, if a helper function to check for the families that are > > really supported could be another alternative. But not sure it is > > worth? > > Not worth. > > Probably this patch instead? Just make sure that access to the chain > type array is safe, no direct access to chain_type[][] anymore. > > This includes the check for the default type too, since it cannot be > assume to always have a filter chain for unsupported families. > > Thanks for explaining. LGTM, this will prbably also fix the other sytbot splat wrt. nla_strcmp().
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 65f51a2e9c2a..e8976128cdb1 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, struct nft_ctx ctx; int err; + if (family >= NFPROTO_NUMPROTO) + return -EAFNOSUPPORT; + lockdep_assert_held(&net->nft.commit_mutex); attr = nla[NFTA_TABLE_NAME]; table = nft_table_lookup(net, attr, family, genmask); @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, ha[NFTA_HOOK_PRIORITY] == NULL) return -EINVAL; + if (family >= NFPROTO_NUMPROTO) + return -EAFNOSUPPORT; + hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, family, autoload); if (IS_ERR(type)) return PTR_ERR(type); + } else if (!type) { + return -EOPNOTSUPP; } if (hook->num > NF_MAX_HOOKS || !(type->hook_mask & (1 << hook->num))) return -EOPNOTSUPP;
Its possible to create tables in a family that isn't supported/known. Then, when adding a base chain, the table pointer can be NULL. This gets us a NULL ptr dereference in nf_tables_addchain(). Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/nf_tables_api.c | 8 ++++++++ 1 file changed, 8 insertions(+)