Patchwork netfilter: nf_tables: set names cannot be larger than 15 bytes

login
register
mail settings
Submitter Pablo Neira
Date March 24, 2014, 2:10 p.m.
Message ID <1395670237-3841-1-git-send-email-pablo@netfilter.org>
Download mbox | patch
Permalink /patch/333057/
State Superseded
Headers show

Comments

Pablo Neira - March 24, 2014, 2:10 p.m.
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(-)
Florian Westphal - March 24, 2014, 10:41 p.m.
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?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - March 25, 2014, 8:45 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - March 26, 2014, 12:32 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - March 28, 2014, 11:20 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - March 28, 2014, 1:18 p.m.
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).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 },