* [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
@ 2014-03-24 14:10 Pablo Neira Ayuso
2014-03-24 22:41 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-03-24 14:10 UTC (permalink / raw)
To: netfilter-devel; +Cc: giuseppelng, kaber
Currently, nf_tables trims off the set name if it exceeeds 15
bytes, so explicitly reject set names that are too large.
Reported-by: Giuseppe Longo <giuseppelng@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_tables_api.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 33045a5..43ae487 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
[NFTA_SET_TABLE] = { .type = NLA_STRING },
- [NFTA_SET_NAME] = { .type = NLA_STRING },
+ [NFTA_SET_NAME] = { .type = NLA_STRING,
+ .len = IFNAMSIZ - 1 },
[NFTA_SET_FLAGS] = { .type = NLA_U32 },
[NFTA_SET_KEY_TYPE] = { .type = NLA_U32 },
[NFTA_SET_KEY_LEN] = { .type = NLA_U32 },
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
2014-03-24 14:10 [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes Pablo Neira Ayuso
@ 2014-03-24 22:41 ` Florian Westphal
2014-03-25 20:45 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2014-03-24 22:41 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, giuseppelng, kaber
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 33045a5..43ae487 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
>
> static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
> [NFTA_SET_TABLE] = { .type = NLA_STRING },
> - [NFTA_SET_NAME] = { .type = NLA_STRING },
> + [NFTA_SET_NAME] = { .type = NLA_STRING,
> + .len = IFNAMSIZ - 1 },
Not related to your patch, but it looks to me as if all of these
should be NLA_NUL_STRING, no?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
2014-03-24 22:41 ` Florian Westphal
@ 2014-03-25 20:45 ` Pablo Neira Ayuso
2014-03-26 0:32 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-03-25 20:45 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel, giuseppelng, kaber
On Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 33045a5..43ae487 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
> >
> > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
> > [NFTA_SET_TABLE] = { .type = NLA_STRING },
> > - [NFTA_SET_NAME] = { .type = NLA_STRING },
> > + [NFTA_SET_NAME] = { .type = NLA_STRING,
> > + .len = IFNAMSIZ - 1 },
>
> Not related to your patch, but it looks to me as if all of these
> should be NLA_NUL_STRING, no?
This is what I originally thought. But all of the nla_* functions use
the nla_len to know length of the string coming from userspace, so
they don't rely on the trailing nul-termination.
Anyway, I'd appreciate if you can do a second look at this to make
sure I'm not overlooking anything. Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
2014-03-25 20:45 ` Pablo Neira Ayuso
@ 2014-03-26 0:32 ` Florian Westphal
2014-03-28 11:20 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2014-03-26 0:32 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel, giuseppelng, kaber
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index 33045a5..43ae487 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
> > >
> > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
> > > [NFTA_SET_TABLE] = { .type = NLA_STRING },
> > > - [NFTA_SET_NAME] = { .type = NLA_STRING },
> > > + [NFTA_SET_NAME] = { .type = NLA_STRING,
> > > + .len = IFNAMSIZ - 1 },
> >
> > Not related to your patch, but it looks to me as if all of these
> > should be NLA_NUL_STRING, no?
>
> This is what I originally thought. But all of the nla_* functions use
> the nla_len to know length of the string coming from userspace, so
> they don't rely on the trailing nul-termination.
Indeed. I reviewed this again but only 'bug' I spotted:
nf_tables_chain_type_lookup() does:
request_module("nft-chain-%u-%*.s", ....
('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is
NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
2014-03-26 0:32 ` Florian Westphal
@ 2014-03-28 11:20 ` Pablo Neira Ayuso
2014-03-28 13:18 ` Florian Westphal
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-03-28 11:20 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel, giuseppelng, kaber
On Wed, Mar 26, 2014 at 01:32:50AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > index 33045a5..43ae487 100644
> > > > --- a/net/netfilter/nf_tables_api.c
> > > > +++ b/net/netfilter/nf_tables_api.c
> > > > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const
> > > >
> > > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = {
> > > > [NFTA_SET_TABLE] = { .type = NLA_STRING },
> > > > - [NFTA_SET_NAME] = { .type = NLA_STRING },
> > > > + [NFTA_SET_NAME] = { .type = NLA_STRING,
> > > > + .len = IFNAMSIZ - 1 },
> > >
> > > Not related to your patch, but it looks to me as if all of these
> > > should be NLA_NUL_STRING, no?
> >
> > This is what I originally thought. But all of the nla_* functions use
> > the nla_len to know length of the string coming from userspace, so
> > they don't rely on the trailing nul-termination.
>
> Indeed. I reviewed this again but only 'bug' I spotted:
>
> nf_tables_chain_type_lookup() does:
> request_module("nft-chain-%u-%*.s", ....
>
> ('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is
> NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.
I think we can change that to NLA_STRING to make it consistent with
other attributes. libnftnl is anyway sending the nul-termination all
the time, so nothing should break IMO.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes
2014-03-28 11:20 ` Pablo Neira Ayuso
@ 2014-03-28 13:18 ` Florian Westphal
0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2014-03-28 13:18 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel, giuseppelng, kaber
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > This is what I originally thought. But all of the nla_* functions use
> > > the nla_len to know length of the string coming from userspace, so
> > > they don't rely on the trailing nul-termination.
> >
> > Indeed. I reviewed this again but only 'bug' I spotted:
> >
> > nf_tables_chain_type_lookup() does:
> > request_module("nft-chain-%u-%*.s", ....
> >
> > ('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is
> > NLA_NUL_STR so the erroneous fmt could also be changed to plain %s.
>
> I think we can change that to NLA_STRING to make it consistent with
> other attributes. libnftnl is anyway sending the nul-termination all
> the time, so nothing should break IMO.
Right, but the kernel shouldn't rely on that :)
I see no problem converting it to NLA_STRING (if request_module args are
fixed).
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-28 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-24 14:10 [PATCH] netfilter: nf_tables: set names cannot be larger than 15 bytes Pablo Neira Ayuso
2014-03-24 22:41 ` Florian Westphal
2014-03-25 20:45 ` Pablo Neira Ayuso
2014-03-26 0:32 ` Florian Westphal
2014-03-28 11:20 ` Pablo Neira Ayuso
2014-03-28 13:18 ` Florian Westphal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.