[nft PATCH] src: check if the set name is too long

[nft PATCH] src: check if the set name is too long

[Giuseppe Longo]

checks if the name of set is larger than 16 chars
before to add it.
If so, the set is not added and an error message is printed.

I didn't figure out why, but another error message is printed, see below:

nft add set ip test thenameofthissetistoolooong { type ipv4_address\; }
<cmdline>:1:17-43: Error: set name is too long (> 16)
add set ip test thenameofthissetistoolooong { type ipv4_address; }
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
<cmdline>:1:66-66: Error: syntax error, unexpected '}'
add set ip test thenameofthissetistoolooong { type ipv4_address; }

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 src/parser.y | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/parser.y b/src/parser.y
index db6f493..17fbd5e 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -15,6 +15,7 @@
 #include <inttypes.h>
 #include <netinet/ip.h>
 #include <netinet/if_ether.h>
+#include <net/if.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nf_tables.h>
 #include <linux/netfilter/nf_conntrack_tuple_common.h>
@@ -986,6 +987,11 @@ chain_identifier	:	identifier
 
 set_spec		:	table_spec	identifier
 			{
+				if (strlen($2) > IFNAMSIZ) {
+					erec_queue(error(&@2, "set name too long (> %d)", IFNAMSIZ),
+						   state->msgs);
+					YYERROR;
+				}
 				$$		= $1;
 				$$.set		= $2;
 			}
@@ -993,6 +999,12 @@ set_spec		:	table_spec	identifier
 
 set_identifier		:	identifier
 			{
+				if (strlen($1) > IFNAMSIZ) {
+					erec_queue(error(&@1, "set name too long (> %d", IFNAMSIZ),
+						   state->msgs);
+					YYERROR;
+				}
+
 				memset(&$$, 0, sizeof($$));
 				$$.set		= $1;
 			}
-- 
1.8.3.2

[PATCH] nftables: fix length of set name

[Giuseppe Longo]

This patch permits to copy the last char of the set name
that's currently excluded.

nft add table ip test
nft add chain ip test filter {type filter hook input priority 0 \; }
nft add set ip test thisnamewith0016 { type ipv4_address\;}
nft list table ip test
table ip test {
        set thisnamewith0016 {
                type ipv4_address
        }

        chain filter {
                 type filter hook input priority 0;
        }
}

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index adce01e..a0da542 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2018,7 +2018,7 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 		free_page((unsigned long)inuse);
 	}
 
-	snprintf(set->name, sizeof(set->name), name, n);
+	snprintf(set->name, sizeof(set->name)+1, name, n);
 	list_for_each_entry(i, &ctx->table->sets, list) {
 		if (!strcmp(set->name, i->name))
 			return -ENFILE;
@@ -2399,7 +2399,7 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
 	if (set == NULL)
 		goto err1;
 
-	nla_strlcpy(name, nla[NFTA_SET_NAME], sizeof(set->name));
+	nla_strlcpy(name, nla[NFTA_SET_NAME], sizeof(set->name)+1);
 	err = nf_tables_set_alloc_name(&ctx, set, name);
 	if (err < 0)
 		goto err2;
-- 
1.8.3.2

Re: [PATCH] nftables: fix length of set name

[Pablo Neira Ayuso]

On Fri, Mar 21, 2014 at 06:39:01PM +0100, Giuseppe Longo wrote:
> This patch permits to copy the last char of the set name
> that's currently excluded.
> 
> nft add table ip test
> nft add chain ip test filter {type filter hook input priority 0 \; }
> nft add set ip test thisnamewith0016 { type ipv4_address\;}
> nft list table ip test
> table ip test {
>         set thisnamewith0016 {
>                 type ipv4_address
>         }
> 
>         chain filter {
>                  type filter hook input priority 0;
>         }
> }
> 
> Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
> ---
>  net/netfilter/nf_tables_api.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index adce01e..a0da542 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2018,7 +2018,7 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
>  		free_page((unsigned long)inuse);
>  	}
>  
> -	snprintf(set->name, sizeof(set->name), name, n);
> +	snprintf(set->name, sizeof(set->name)+1, name, n);

This allows to write up to 16+1 bytes.

>  	list_for_each_entry(i, &ctx->table->sets, list) {
>  		if (!strcmp(set->name, i->name))
>  			return -ENFILE;
> @@ -2399,7 +2399,7 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
>  	if (set == NULL)
>  		goto err1;
>  
> -	nla_strlcpy(name, nla[NFTA_SET_NAME], sizeof(set->name));
> +	nla_strlcpy(name, nla[NFTA_SET_NAME], sizeof(set->name)+1);

Same thing here, but the name size is just 16 bytes long.

Re: [nft PATCH] src: check if the set name is too long

[Pablo Neira Ayuso]

On Fri, Mar 21, 2014 at 06:39:00PM +0100, Giuseppe Longo wrote:
> checks if the name of set is larger than 16 chars
> before to add it.
> If so, the set is not added and an error message is printed.
> 
> I didn't figure out why, but another error message is printed, see below:
> 
> nft add set ip test thenameofthissetistoolooong { type ipv4_address\; }
> <cmdline>:1:17-43: Error: set name is too long (> 16)
> add set ip test thenameofthissetistoolooong { type ipv4_address; }
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> <cmdline>:1:66-66: Error: syntax error, unexpected '}'
> add set ip test thenameofthissetistoolooong { type ipv4_address; }

I sent you a patch, I think it's better if we fix this from
kernel-space.

Re: [nft PATCH] src: check if the set name is too long

[Tomasz Bursztyka]

Hi Pablo,

> I sent you a patch, I think it's better if we fix this from
> kernel-space.

I think it's also good if we check the length when parsing, as Giuseppe did.
Then it reduce the overhead: the error is detected way before we process 
anything through netlink.

Of course here it should be IFNAMSIZ-1.
Giuseppe: could you resubmit your patch fixed ?

About the parsing error on unexpected '}' I believe it's another issue
the patch revealed somehow. (state->msgs issue?).

Tomasz

Re: [nft PATCH] src: check if the set name is too long

[Pablo Neira Ayuso]

On Tue, Mar 25, 2014 at 09:37:24AM +0200, Tomasz Bursztyka wrote:
> Hi Pablo,
> 
> >I sent you a patch, I think it's better if we fix this from
> >kernel-space.
> 
> I think it's also good if we check the length when parsing, as Giuseppe did.
> Then it reduce the overhead: the error is detected way before we
> process anything through netlink.

This is an error case, I don't think we should focus on reducing
overhead in those scenarios.

Re: [nft PATCH] src: check if the set name is too long

[Pablo Neira Ayuso]

On Tue, Mar 25, 2014 at 09:41:31AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 25, 2014 at 09:37:24AM +0200, Tomasz Bursztyka wrote:
> > Hi Pablo,
> > 
> > >I sent you a patch, I think it's better if we fix this from
> > >kernel-space.
> > 
> > I think it's also good if we check the length when parsing, as Giuseppe did.
> > Then it reduce the overhead: the error is detected way before we
> > process anything through netlink.
> 
> This is an error case, I don't think we should focus on reducing
> overhead in those scenarios.

Just to extend this. I prefer this limit is also set in kernelspace so
in case we ever remove it, we won't have to wait until a new nft
userspace tool version is released.

[nft PATCH] src: check if the set name is too long

[Giuseppe Longo]

checks if the name of set is larger than 15 chars
before to add it.
If so, the set is not added and an error message is printed.

I didn't figure out why, but another error message is printed, see below:

nft add set ip test thenameofthissetistoolooong { type ipv4_address\; }
<cmdline>:1:17-43: Error: set name is too long (> 16)
add set ip test thenameofthissetistoolooong { type ipv4_address; }
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
<cmdline>:1:66-66: Error: syntax error, unexpected '}'
add set ip test thenameofthissetistoolooong { type ipv4_address; }

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 src/parser.y | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/parser.y b/src/parser.y
index db6f493..17fbd5e 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -15,6 +15,7 @@
 #include <inttypes.h>
 #include <netinet/ip.h>
 #include <netinet/if_ether.h>
+#include <net/if.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nf_tables.h>
 #include <linux/netfilter/nf_conntrack_tuple_common.h>
@@ -986,6 +987,11 @@ chain_identifier	:	identifier
 
 set_spec		:	table_spec	identifier
 			{
+				if (strlen($2) > IFNAMSIZ) {
+					erec_queue(error(&@2, "set name too long (> %d)", IFNAMSIZ),
+						   state->msgs);
+					YYERROR;
+				}
 				$$		= $1;
 				$$.set		= $2;
 			}
@@ -993,6 +999,12 @@ set_spec		:	table_spec	identifier
 
 set_identifier		:	identifier
 			{
+				if (strlen($1) > IFNAMSIZ) {
+					erec_queue(error(&@1, "set name too long (> %d", IFNAMSIZ),
+						   state->msgs);
+					YYERROR;
+				}
+
 				memset(&$$, 0, sizeof($$));
 				$$.set		= $1;
 			}
-- 
1.8.3.2

Re: [nft PATCH] src: check if the set name is too long

[Tomasz Bursztyka]

Hi Giuseppe,

> checks if the name of set is larger than 15 chars

You mean 16 characters

Btw, have you tested 16 chars length name: when listing the set back,
is such name cut to 15 chars?

That sounds to be an issue from kernel side, at least looking quickly,
I could not find any code shortening this in  libmnl, libnftnl or nftables.

Tomasz

Re: [nft PATCH] src: check if the set name is too long

[Pablo Neira Ayuso]

On Fri, Mar 21, 2014 at 09:24:47AM +0200, Tomasz Bursztyka wrote:
> Hi Giuseppe,
> 
> >checks if the name of set is larger than 15 chars
> 
> You mean 16 characters
> 
> Btw, have you tested 16 chars length name: when listing the set back,
> is such name cut to 15 chars?
> 
> That sounds to be an issue from kernel side, at least looking quickly,
> I could not find any code shortening this in  libmnl, libnftnl or nftables.

The kernel is indeed limiting the name length in nf_tables_newset, see
nla_strcpy there.

Re: [nft PATCH] src: check if the set name is too long

[Tomasz Bursztyka]

Hi Wei,

I don't know how you ended up finding a relation between nftables API 
and genetlink, but it's bogus.

The set name has a fixed size of 16 characters (IFNAMSIZ), not 15.
And its netlink policy has nothing to do with genetlink.

Have a look at include/net/netfilter/nf_tables.h in kernel tree, there 
struct  nft_set is declared.
Its netlink related policy is in net/netfilter/nf_tables_api.c 
(nft_set_policy)

Br,

Tomasz