[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 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

[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

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.