src: check if the set name is too long

PATCH In review Giuseppe Longo 3 participants 7 messages Mar 21, 2014 – Mar 25, 2014

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Giuseppe Longo

author

2014-03-21 17:39 UTC #0 lore raw

PATCH 1/1 src: check if the set name is too long

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 <[email protected]>

---
 src/parser.y | 12 ++++++++++++
 1 file changed, 12 insertions(+)

src/parser.y +12 -0

index db6f493..17fbd5e 100644
@@ -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;
 			}

Signature · 1 line

1.8.3.2

Giuseppe Longo

author

2014-03-21 17:39 UTC #1 lore raw

PATCH 1/1 nftables: fix length of set name

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 <[email protected]>

---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/netfilter/nf_tables_api.c +2 -2

index adce01e..a0da542 100644
@@ -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;

Signature · 1 line

1.8.3.2

Pablo Neira Ayuso

maintainer

2014-03-24 14:57 UTC #2 lore raw

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

Quoted 27 lines · 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 <[email protected]>
---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/netfilter/nf_tables_api.c +0 -0
index adce01e..a0da542 100644

net/netfilter/nf_tables_api.c static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set, · line 2021

Quoted 5 lines

net/netfilter/nf_tables_api.c +1 -1

@@ -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.

net/netfilter/nf_tables_api.c static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb, · line 2402

Quoted 9 lines

net/netfilter/nf_tables_api.c +1 -1

 	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.

Pablo Neira Ayuso

maintainer

2014-03-24 14:57 UTC #3 lore raw

In reply to Giuseppe Longo

Quoted 4 lines · 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.

Quoted 8 lines

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.

Tomasz Bursztyka

2014-03-25 07:37 UTC #4 lore raw

Hi Pablo,

Quoted 2 lines

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

Pablo Neira Ayuso

maintainer

2014-03-25 08:41 UTC #5 lore raw

Quoted 8 lines · Tomasz Bursztyka

Hi Pablo,
Quoted 2 lines
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.

Pablo Neira Ayuso

maintainer

2014-03-25 08:47 UTC #6 lore raw

Quoted 6 lines · Pablo Neira Ayuso

Quoted 5 lines · Tomasz Bursztyka
Hi Pablo,
Quoted 2 lines
I sent you a patch, I think it's better if we fix this from kernel-space.

Quoted 6 lines

Quoted 3 lines
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.