| Message ID | 20190116184613.31698-1-phil@nwl.cc |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] src: Quote user-defined names | expand |
Hi Phil, On Wed, Jan 16, 2019 at 07:46:13PM +0100, Phil Sutter wrote: > Nftables claims to allow arbitrary names for ruleset elements (tables, > chains, objects) but suffers from the known problem of lex/yacc trying > to interpret those as keywords. As a workaround, users may quote their > names. Sadly this wasn't supported in most cases and this patch lifts > this restriction. > > In order to not print rulesets which are not accepted anymore by 'nft > -f' command, unconditionally quote all names on output. > > Note that the same problem existed for interface names. I've tested for > those to work in both netdev family chains and flowtable definitions, > though automatic testing is troublesome since they must exist (and I'm > not sure if test scripts should call iproute2 to add an interface with a > crafted name). This is what we are supporting natively, probably not well documented: commit 57ecffc9d1e551ecc0546806ca9c008e93c2ecf3 Author: Pablo Neira Ayuso <pablo@netfilter.org> Date: Tue Aug 16 23:22:51 2016 +0200 scanner: allow strings starting by underscores and dots POSIX.1-2008 (which is simultaneously IEEE Std 1003.1-2008) says: "The set of characters from which portable filenames are constructed. A B C D E F G H I J K L M N O P Q R S T U V W X Y Z a b c d e f g h i j k l m n o p q r s t u v w x y z 0 1 2 3 4 5 6 7 8 9 . _ -" I think we can just document this or you need this sort of flexibility. We can also allow for keywords to be used as names, which is what is left behind... We can of course decide to go for quotes as you propose, this was so far the only exception since all other user-defined values from rules are always assumed to enclosed in quotes.
On Wed, Jan 16, 2019 at 08:19:00PM +0100, Pablo Neira Ayuso wrote: > Hi Phil, > > On Wed, Jan 16, 2019 at 07:46:13PM +0100, Phil Sutter wrote: > > Nftables claims to allow arbitrary names for ruleset elements (tables, > > chains, objects) but suffers from the known problem of lex/yacc trying > > to interpret those as keywords. As a workaround, users may quote their > > names. Sadly this wasn't supported in most cases and this patch lifts > > this restriction. > > > > In order to not print rulesets which are not accepted anymore by 'nft > > -f' command, unconditionally quote all names on output. > > > > Note that the same problem existed for interface names. I've tested for > > those to work in both netdev family chains and flowtable definitions, > > though automatic testing is troublesome since they must exist (and I'm > > not sure if test scripts should call iproute2 to add an interface with a > > crafted name). > > This is what we are supporting natively, probably not well documented: > > commit 57ecffc9d1e551ecc0546806ca9c008e93c2ecf3 > Author: Pablo Neira Ayuso <pablo@netfilter.org> > Date: Tue Aug 16 23:22:51 2016 +0200 > > scanner: allow strings starting by underscores and dots > > POSIX.1-2008 (which is simultaneously IEEE Std 1003.1-2008) says: > "The set of characters from which portable filenames are constructed. > > A B C D E F G H I J K L M N O P Q R S T U V W X Y Z > a b c d e f g h i j k l m n o p q r s t u v w x y z > 0 1 2 3 4 5 6 7 8 9 . _ -" > > I think we can just document this or you need this sort of > flexibility. We can also allow for keywords to be used as names, which > is what is left behind... > > We can of course decide to go for quotes as you propose, this was so > far the only exception since all other user-defined values from rules > are always assumed to enclosed in quotes. My point is that users don't necessarily know what names are forbidden (i.e., keywords) and which are not. Suggesting to prefix names by underscore "just in case" or "if you get a weird error message" is not the best option in my opinion. That said, my solution of "quote names unless you know what you're doing" is not much better to be fair. :) I really don't know what's the best way to handle this, given that we can't work around this quirk in lex/yacc without turning all into a mess. The problem I'm facing is simply that users file tickets because they are not aware of the problem and hence think that nft not accepting certain names is simply a bug one should fix. Cheers, Phil
Phil Sutter <phil@nwl.cc> wrote: > Nftables claims to allow arbitrary names for ruleset elements (tables, > chains, objects) but suffers from the known problem of lex/yacc trying > to interpret those as keywords. As a workaround, users may quote their > names. Sadly this wasn't supported in most cases and this patch lifts > this restriction. > > In order to not print rulesets which are not accepted anymore by 'nft > -f' command, unconditionally quote all names on output. Acked-by: Florian Westphal <fw@strlen.de> I think we should also enforce QUOTED_STRING on where possible in future patches (i mean, when new features are added, can't force it for existing cases).
Hi Phil,
Another spin on this, let's try to make a final decision on this asap.
In this case, this patch passes the quoted string to the kernel, so
the listing shows it again.
Still, problem here is that the shell is stripping off the quotes
unless I escape them, ie.
nft add chain "x" "y"
enters the unquoted path from the scannner. So I have to use:
nft add chain \"x\" \"y\"
or:
nft add chain x '"y"'
I think your patch fixes the problem with using keywords as object
names, which we could also fix via a rule that deals with this.
The problem with using any arbitrary name would be still there, unless
the user escapes the quotes.
On the other hand, if we quote the string in the listing by default,
we clearly show that these are user-defined, which is not a bad idea.
However, we don't get much from showing quotes by default on listings,
I mean, this is not solving the arbitrary name problem from the input
path, which I think it the real problem.
Then, enforcing quotes since the beginning would not have helped
either, because of the shell behaviour.
Exploring another patch here to allow to use keywords without quotes
as object names, it won't look nice in bison, since we will need
something similar to what we do in primary_rhs_expr for TCP, UDP...
but it will work.
diff --git a/src/parser_bison.y b/src/parser_bison.y
index b20be3a896b0..e5143c40c794 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -550,6 +550,9 @@ int nft_lex(void *, void *, void *);
%type <string> extended_prio_name
%destructor { xfree($$); } extended_prio_name
+%type <string> generic_identifier
+%destructor { xfree($$); } generic_identifier
+
%type <string> dev_spec quota_unit
%destructor { xfree($$); } dev_spec quota_unit
@@ -2041,7 +2044,7 @@ tableid_spec : family_spec HANDLE NUM
}
;
-chain_spec : table_spec identifier
+chain_spec : table_spec generic_identifier
{
$$ = $1;
$$.chain.name = $2;
@@ -2057,7 +2060,19 @@ chainid_spec : table_spec HANDLE NUM
}
;
-chain_identifier : identifier
+generic_identifier : STRING { $$ = $1; }
+ | QUOTED_STRING
+ {
+ size_t len;
+
+ len = strlen($1) + 3;
+ $$ = xmalloc(len);
+ snprintf($$, len, "\"%s\"", $1);
+ xfree($1);
+ }
+ ;
+
+chain_identifier : generic_identifier
{
memset(&$$, 0, sizeof($$));
$$.chain.name = $1;
On Thu, Feb 14, 2019 at 12:10:54PM +0100, Pablo Neira Ayuso wrote: > On the other hand, if we quote the string in the listing by default, > we clearly show that these are user-defined, which is not a bad idea. > However, we don't get much from showing quotes by default on listings, > I mean, this is not solving the arbitrary name problem from the input > path, which I think it the real problem. Well, from nft -f yes, but not from command line.
Hi Pablo, On Thu, Feb 14, 2019 at 12:10:54PM +0100, Pablo Neira Ayuso wrote: > Another spin on this, let's try to make a final decision on this asap. > > In this case, this patch passes the quoted string to the kernel, so > the listing shows it again. > > Still, problem here is that the shell is stripping off the quotes > unless I escape them, ie. > > nft add chain "x" "y" > > enters the unquoted path from the scannner. So I have to use: > > nft add chain \"x\" \"y\" > > or: > > nft add chain x '"y"' > > I think your patch fixes the problem with using keywords as object > names, which we could also fix via a rule that deals with this. > > The problem with using any arbitrary name would be still there, unless > the user escapes the quotes. > > On the other hand, if we quote the string in the listing by default, > we clearly show that these are user-defined, which is not a bad idea. > However, we don't get much from showing quotes by default on listings, > I mean, this is not solving the arbitrary name problem from the input > path, which I think it the real problem. > > Then, enforcing quotes since the beginning would not have helped > either, because of the shell behaviour. > > Exploring another patch here to allow to use keywords without quotes > as object names, it won't look nice in bison, since we will need > something similar to what we do in primary_rhs_expr for TCP, UDP... > but it will work. Are you sure about that? Flex would still recognize the keyword as such and you won't get STRING type in Bison. Or am I missing the point? Personally, I'm totally fine with people having to escape the quotes. This is how shells work, and we have the same problem in other situation requiring the quotes, too. My shell for instance catches the curly braces and semi-colons, as well if not escaped. Quoting all user-defined names on output merely helps with the case where a user *really* wanted to produce a confusing ruleset and to avoid ruleset restore after dump from failing miserably because the names are not quoted in output. HTH, Phil
Hi again, On Thu, Feb 14, 2019 at 06:43:03PM +0100, Phil Sutter wrote: > On Thu, Feb 14, 2019 at 12:10:54PM +0100, Pablo Neira Ayuso wrote: > > Another spin on this, let's try to make a final decision on this asap. > > > > In this case, this patch passes the quoted string to the kernel, so > > the listing shows it again. > > > > Still, problem here is that the shell is stripping off the quotes > > unless I escape them, ie. > > > > nft add chain "x" "y" > > > > enters the unquoted path from the scannner. So I have to use: > > > > nft add chain \"x\" \"y\" > > > > or: > > > > nft add chain x '"y"' > > > > I think your patch fixes the problem with using keywords as object > > names, which we could also fix via a rule that deals with this. > > > > The problem with using any arbitrary name would be still there, unless > > the user escapes the quotes. > > > > On the other hand, if we quote the string in the listing by default, > > we clearly show that these are user-defined, which is not a bad idea. > > However, we don't get much from showing quotes by default on listings, > > I mean, this is not solving the arbitrary name problem from the input > > path, which I think it the real problem. > > > > Then, enforcing quotes since the beginning would not have helped > > either, because of the shell behaviour. > > > > Exploring another patch here to allow to use keywords without quotes > > as object names, it won't look nice in bison, since we will need > > something similar to what we do in primary_rhs_expr for TCP, UDP... > > but it will work. > > Are you sure about that? Flex would still recognize the keyword as such > and you won't get STRING type in Bison. Or am I missing the point? > > Personally, I'm totally fine with people having to escape the quotes. > This is how shells work, and we have the same problem in other situation > requiring the quotes, too. My shell for instance catches the curly > braces and semi-colons, as well if not escaped. > > Quoting all user-defined names on output merely helps with the case > where a user *really* wanted to produce a confusing ruleset and to avoid > ruleset restore after dump from failing miserably because the names are > not quoted in output. Getting back to this dusty topic again: I played with extending 'identifier' in parser_bison.c like so: | @@ -2183,6 +2183,7 @@ chain_policy : ACCEPT { $$ = NF_ACCEPT; } | ; | | identifier : STRING | + | HOUR { $$ = strdup("hour"); } | ; | | string : STRING I am able to create a table named 'hour'. The approach has two problems though AFAICT: 1) When adding TABLE as identifier, bison spews shift/reduce conflict warnings. 2) In order to allow for really arbitrary names, we would have to add all defined keywords to identifier. This is tedious, ugly and (most importantly) a moving target. Do you think it is possible to address these problems? Cheers, Phil
Hi, On Thu, Jan 23, 2020 at 11:45:48PM +0100, Phil Sutter wrote: > On Thu, Feb 14, 2019 at 06:43:03PM +0100, Phil Sutter wrote: > > On Thu, Feb 14, 2019 at 12:10:54PM +0100, Pablo Neira Ayuso wrote: > > > Another spin on this, let's try to make a final decision on this asap. > > > > > > In this case, this patch passes the quoted string to the kernel, so > > > the listing shows it again. > > > > > > Still, problem here is that the shell is stripping off the quotes > > > unless I escape them, ie. > > > > > > nft add chain "x" "y" > > > > > > enters the unquoted path from the scannner. So I have to use: > > > > > > nft add chain \"x\" \"y\" > > > > > > or: > > > > > > nft add chain x '"y"' > > > > > > I think your patch fixes the problem with using keywords as object > > > names, which we could also fix via a rule that deals with this. > > > > > > The problem with using any arbitrary name would be still there, unless > > > the user escapes the quotes. > > > > > > On the other hand, if we quote the string in the listing by default, > > > we clearly show that these are user-defined, which is not a bad idea. > > > However, we don't get much from showing quotes by default on listings, > > > I mean, this is not solving the arbitrary name problem from the input > > > path, which I think it the real problem. > > > > > > Then, enforcing quotes since the beginning would not have helped > > > either, because of the shell behaviour. > > > > > > Exploring another patch here to allow to use keywords without quotes > > > as object names, it won't look nice in bison, since we will need > > > something similar to what we do in primary_rhs_expr for TCP, UDP... > > > but it will work. > > > > Are you sure about that? Flex would still recognize the keyword as such > > and you won't get STRING type in Bison. Or am I missing the point? > > > > Personally, I'm totally fine with people having to escape the quotes. > > This is how shells work, and we have the same problem in other situation > > requiring the quotes, too. My shell for instance catches the curly > > braces and semi-colons, as well if not escaped. > > > > Quoting all user-defined names on output merely helps with the case > > where a user *really* wanted to produce a confusing ruleset and to avoid > > ruleset restore after dump from failing miserably because the names are > > not quoted in output. > > Getting back to this dusty topic again: I played with extending > 'identifier' in parser_bison.c like so: > > | @@ -2183,6 +2183,7 @@ chain_policy : ACCEPT { $$ = NF_ACCEPT; } > | ; > | > | identifier : STRING > | + | HOUR { $$ = strdup("hour"); } > | ; > | > | string : STRING > > I am able to create a table named 'hour'. The approach has two problems > though AFAICT: > > 1) When adding TABLE as identifier, bison spews shift/reduce conflict > warnings. > > 2) In order to allow for really arbitrary names, we would have to add > all defined keywords to identifier. This is tedious, ugly and (most > importantly) a moving target. I had another idea, namely teaching flex to recognize a bit of context by defining e.g. 'table <something>' as a keyword. But this turns into a mess when it comes to optional family specifiers, line breaks, etc. Also this is probably not how lex/yacc was meant to be used. The reason why I keep getting back to this is the increasing impact on users over time: Whenever we introduce a new keyword, we potentially break someone's working ruleset. With bison not even accepting quoted names everywhere, there is no way out even for cautious users. Can we please restart discussions on how to solve this issue? Cheers, Phil
diff --git a/src/parser_bison.y b/src/parser_bison.y index 02a373cb2289a..fb9b0fcf89baf 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -1782,7 +1782,7 @@ flowtable_list_expr : flowtable_expr_member | flowtable_list_expr COMMA opt_newline ; -flowtable_expr_member : STRING +flowtable_expr_member : string { $$ = symbol_expr_alloc(&@$, SYMBOL_VALUE, current_scope(state), @@ -1989,6 +1989,7 @@ chain_policy : ACCEPT { $$ = NF_ACCEPT; } ; identifier : STRING + | QUOTED_STRING ; string : STRING diff --git a/src/rule.c b/src/rule.c index 73b78c75a267a..f845b5d097c3a 100644 --- a/src/rule.c +++ b/src/rule.c @@ -433,9 +433,9 @@ static void set_print_declaration(const struct set *set, nft_print(octx, " %s", opts->family); if (opts->table != NULL) - nft_print(octx, " %s", opts->table); + nft_print(octx, " \"%s\"", opts->table); - nft_print(octx, " %s {", set->handle.set.name); + nft_print(octx, " \"%s\" {", set->handle.set.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, set->handle.handle.id); @@ -1062,7 +1062,7 @@ static void chain_print_declaration(const struct chain *chain, { char priobuf[STD_PRIO_BUFSIZE]; - nft_print(octx, "\tchain %s {", chain->handle.chain.name); + nft_print(octx, "\tchain \"%s\" {", chain->handle.chain.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id); nft_print(octx, "\n"); @@ -1222,7 +1222,7 @@ static void table_print(const struct table *table, struct output_ctx *octx) const char *delim = ""; const char *family = family2str(table->handle.family); - nft_print(octx, "table %s %s {", family, table->handle.table.name); + nft_print(octx, "table %s \"%s\" {", family, table->handle.table.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, table->handle.handle.id); nft_print(octx, "\n"); @@ -1748,7 +1748,7 @@ static void obj_print_data(const struct obj *obj, { switch (obj->type) { case NFT_OBJECT_COUNTER: - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab); @@ -1763,7 +1763,7 @@ static void obj_print_data(const struct obj *obj, const char *data_unit; uint64_t bytes; - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab); @@ -1780,14 +1780,14 @@ static void obj_print_data(const struct obj *obj, } break; case NFT_OBJECT_SECMARK: - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab); nft_print(octx, "%s", obj->secmark.ctx); break; case NFT_OBJECT_CT_HELPER: - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s", opts->nl); @@ -1801,7 +1801,7 @@ static void obj_print_data(const struct obj *obj, opts->stmt_separator); break; case NFT_OBJECT_CT_TIMEOUT: - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s", opts->nl); @@ -1820,7 +1820,7 @@ static void obj_print_data(const struct obj *obj, const char *data_unit; uint64_t rate; - nft_print(octx, " %s {", obj->handle.obj.name); + nft_print(octx, " \"%s\" {", obj->handle.obj.name); if (nft_output_handle(octx)) nft_print(octx, " # handle %" PRIu64, obj->handle.handle.id); nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab); @@ -1899,7 +1899,7 @@ static void obj_print_declaration(const struct obj *obj, nft_print(octx, " %s", opts->family); if (opts->table != NULL) - nft_print(octx, " %s", opts->table); + nft_print(octx, " \"%s\"", opts->table); obj_print_data(obj, opts, octx); @@ -2015,9 +2015,9 @@ static void flowtable_print_declaration(const struct flowtable *flowtable, nft_print(octx, " %s", opts->family); if (opts->table != NULL) - nft_print(octx, " %s", opts->table); + nft_print(octx, " \"%s\"", opts->table); - nft_print(octx, " %s {%s", flowtable->handle.flowtable, opts->nl); + nft_print(octx, " \"%s\" {%s", flowtable->handle.flowtable, opts->nl); nft_print(octx, "%s%shook %s priority %s%s", opts->tab, opts->tab, @@ -2028,7 +2028,7 @@ static void flowtable_print_declaration(const struct flowtable *flowtable, nft_print(octx, "%s%sdevices = { ", opts->tab, opts->tab); for (i = 0; i < flowtable->dev_array_len; i++) { - nft_print(octx, "%s", flowtable->dev_array[i]); + nft_print(octx, "\"%s\"", flowtable->dev_array[i]); if (i + 1 != flowtable->dev_array_len) nft_print(octx, ", "); } diff --git a/tests/shell/testcases/nft-f/0018quoted-names_0 b/tests/shell/testcases/nft-f/0018quoted-names_0 new file mode 100755 index 0000000000000..9655a48d492c0 --- /dev/null +++ b/tests/shell/testcases/nft-f/0018quoted-names_0 @@ -0,0 +1,19 @@ +#!/bin/bash + +# Test if keywords are allowed as names if quoted + +set -e + +# XXX: interface names are arbitrary, too (flowtable, chain) +RULESET=' +table inet "day" { + chain "minute" {} + set "hour" { type inet_service; } + flowtable "second" { hook ingress priority 0; devices = { lo }; } + counter "table" { packets 0 bytes 0 } + quota "chain" { 10 bytes } +}' + +$NFT -f - <<< "$RULESET" +OUTPUT=$($NFT list ruleset) +$NFT -f - <<< "$OUTPUT"
Nftables claims to allow arbitrary names for ruleset elements (tables, chains, objects) but suffers from the known problem of lex/yacc trying to interpret those as keywords. As a workaround, users may quote their names. Sadly this wasn't supported in most cases and this patch lifts this restriction. In order to not print rulesets which are not accepted anymore by 'nft -f' command, unconditionally quote all names on output. Note that the same problem existed for interface names. I've tested for those to work in both netdev family chains and flowtable definitions, though automatic testing is troublesome since they must exist (and I'm not sure if test scripts should call iproute2 to add an interface with a crafted name). Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/parser_bison.y | 3 +- src/rule.c | 28 +++++++++---------- .../shell/testcases/nft-f/0018quoted-names_0 | 19 +++++++++++++ 3 files changed, 35 insertions(+), 15 deletions(-) create mode 100755 tests/shell/testcases/nft-f/0018quoted-names_0