Re: Unable to create a chain called "trace"

Re: Unable to create a chain called "trace"

[Florian Westphal]

Martin Gignac <martin.gignac@gmail.com> wrote:

[ cc devel ]

> Out of curiosity, is there a reason why calling a chain "trace"
> results in an error?
> 
> This configuration:
> 
>   chain trace {
>     type filter hook prerouting priority -301;
>     ip daddr 24.153.88.9 ip protocol icmp meta nftrace set 1
>   }
> 
> Results in the following error when I try loading the ruleset:
> 
>   /etc/firewall/rules.nft:40:9-13: Error: syntax error, unexpected
> trace, expecting string
>   chain trace {
>         ^^^^^

grammar bug.

Pablo, Phil, others, can you remind me why we never did:

diff --git a/src/monitor.c b/src/monitor.c
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -254,7 +254,7 @@ static int netlink_events_chain_cb(const struct nlmsghdr *nlh, int type,
 			chain_print_plain(c, &monh->ctx->nft->output);
 			break;
 		case NFT_MSG_DELCHAIN:
-			nft_mon_print(monh, "chain %s %s %s",
+			nft_mon_print(monh, "chain %s \"%s\" \"%s\"",
 				      family2str(c->handle.family),
 				      c->handle.table.name,
 				      c->handle.chain.name);
diff --git a/src/parser_bison.y b/src/parser_bison.y
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2395,6 +2395,7 @@ chain_policy		:	ACCEPT		{ $$ = NF_ACCEPT; }
 			;
 
 identifier		:	STRING
+			|	QUOTED_STRING
 			;
 
 string			:	STRING
diff --git a/src/rule.c b/src/rule.c
index e4bb6bae276a..77477e535f2e 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1236,7 +1236,7 @@ static void chain_print_declaration(const struct chain *chain,
 	if (chain->flags & CHAIN_F_BINDING)
 		return;
 
-	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);
 	if (chain->comment)
@@ -1297,7 +1297,7 @@ void chain_print_plain(const struct chain *chain, struct output_ctx *octx)
 	char priobuf[STD_PRIO_BUFSIZE];
 	int policy;
 
-	nft_print(octx, "chain %s %s %s", family2str(chain->handle.family),
+	nft_print(octx, "chain %s \"%s\" \"%s\"", family2str(chain->handle.family),
 		  chain->handle.table.name, chain->handle.chain.name);
 
 	if (chain->flags & CHAIN_F_BASECHAIN) {

?

Re: Unable to create a chain called "trace"

[Phil Sutter]

Hi,

On Mon, Feb 08, 2021 at 04:49:15PM +0100, Florian Westphal wrote:
> Martin Gignac <martin.gignac@gmail.com> wrote:
> 
> [ cc devel ]
> 
> > Out of curiosity, is there a reason why calling a chain "trace"
> > results in an error?
> > 
> > This configuration:
> > 
> >   chain trace {
> >     type filter hook prerouting priority -301;
> >     ip daddr 24.153.88.9 ip protocol icmp meta nftrace set 1
> >   }
> > 
> > Results in the following error when I try loading the ruleset:
> > 
> >   /etc/firewall/rules.nft:40:9-13: Error: syntax error, unexpected
> > trace, expecting string
> >   chain trace {
> >         ^^^^^
> 
> grammar bug.
> 
> Pablo, Phil, others, can you remind me why we never did:

Because this would be followed up by:

| Subject: Unable to create a table called "trace"

Jokes aside:

I think Pablo didn't like the obvious consequence of having to quote
*all* string types which are user-defined in output. He played with
keeping the quotes as part of the name, so they are sent to kernel and
in listing they would automatically appear quoted. I don't quite
remember why this was problematic, though.

In general, shells eating the quotes is problematic and users may not be
aware of it. This includes scripts that mangle ruleset dumps by
accident, etc. (Not sure if it is really a problem as we quote some
strings already).

Using JSON, there are no such limits, BTW. I really wonder if there's
really no fix for bison parser to make it "context aware".

Cheers, Phil

Re: Unable to create a chain called "trace"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> In general, shells eating the quotes is problematic and users may not be
> aware of it. This includes scripts that mangle ruleset dumps by
> accident, etc. (Not sure if it is really a problem as we quote some
> strings already).
> 
> Using JSON, there are no such limits, BTW. I really wonder if there's
> really no fix for bison parser to make it "context aware".

Right.  We can probably make lots of keywords available for table/chain names
by only recognizing them while parsing rules, i.e. via 'start conditions'
in flex.  But I don't think there is anyone with the time to do the
needed scanner changes.

Re: Unable to create a chain called "trace"

[Phil Sutter]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

Hi,

On Mon, Feb 08, 2021 at 06:14:44PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > In general, shells eating the quotes is problematic and users may not be
> > aware of it. This includes scripts that mangle ruleset dumps by
> > accident, etc. (Not sure if it is really a problem as we quote some
> > strings already).
> > 
> > Using JSON, there are no such limits, BTW. I really wonder if there's
> > really no fix for bison parser to make it "context aware".
> 
> Right.  We can probably make lots of keywords available for table/chain names
> by only recognizing them while parsing rules, i.e. via 'start conditions'
> in flex.  But I don't think there is anyone with the time to do the
> needed scanner changes.

Oh, I wasn't aware of start conditions at all, thanks for the pointer.
Instead of reducing most keyword's scope to rule context, I tried a less
intrusive approach, namely recognizing "only strings plus some extra" in
certain conditions. See attached patch for reference. With it in place,
I was at least able to:

# nft add table inet table
# nft add chain inet table chain
# nft add rule inet table chain iifname rule

Cheers, Phil

[-- Attachment #2: nftables_start_condition.diff --]
[-- Type: text/plain, Size: 3954 bytes --]

diff --git a/src/scanner.l b/src/scanner.l
index 8bde1fbe912d8..c873cb7c1d226 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -107,6 +107,8 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 extern int	yyget_column(yyscan_t);
 extern void	yyset_column(int, yyscan_t);
 
+static int nspec;
+
 %}
 
 space		[ ]
@@ -194,6 +196,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 %option nodefault
 %option warn
 
+%x spec
+
 %%
 
 "=="			{ return EQ; }
@@ -250,19 +254,19 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "hook"			{ return HOOK; }
 "device"		{ return DEVICE; }
 "devices"		{ return DEVICES; }
-"table"			{ return TABLE; }
+"table"			{ BEGIN(spec); nspec = 1; return TABLE; }
 "tables"		{ return TABLES; }
-"chain"			{ return CHAIN; }
+"chain"			{ BEGIN(spec); nspec = 2; return CHAIN; }
 "chains"		{ return CHAINS; }
-"rule"			{ return RULE; }
+"rule"			{ BEGIN(spec); nspec = 2; return RULE; }
 "rules"			{ return RULES; }
 "sets"			{ return SETS; }
-"set"			{ return SET; }
+"set"			{ BEGIN(spec); nspec = 2; return SET; }
 "element"		{ return ELEMENT; }
-"map"			{ return MAP; }
+"map"			{ BEGIN(spec); nspec = 2; return MAP; }
 "maps"			{ return MAPS; }
 "flowtable"		{ return FLOWTABLE; }
-"handle"		{ return HANDLE; }
+<*>"handle"		{ return HANDLE; }
 "ruleset"		{ return RULESET; }
 "trace"			{ return TRACE; }
 
@@ -280,8 +284,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "return"		{ return RETURN; }
 "to"			{ return TO; }
 
-"inet"			{ return INET; }
-"netdev"		{ return NETDEV; }
+<*>"inet"		{ return INET; }
+<*>"netdev"		{ return NETDEV; }
 
 "add"			{ return ADD; }
 "replace"		{ return REPLACE; }
@@ -380,7 +384,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "nh"			{ return NETWORK_HDR; }
 "th"			{ return TRANSPORT_HDR; }
 
-"bridge"		{ return BRIDGE; }
+<*>"bridge"		{ return BRIDGE; }
 
 "ether"			{ return ETHER; }
 "saddr"			{ return SADDR; }
@@ -400,7 +404,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "plen"			{ return PLEN; }
 "operation"		{ return OPERATION; }
 
-"ip"			{ return IP; }
+<*>"ip"			{ return IP; }
 "version"		{ return HDRVERSION; }
 "hdrlength"		{ return HDRLENGTH; }
 "dscp"			{ return DSCP; }
@@ -451,7 +455,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "igmp"			{ return IGMP; }
 "mrt"			{ return MRT; }
 
-"ip6"			{ return IP6; }
+<*>"ip6"		{ return IP6; }
 "priority"		{ return PRIORITY; }
 "flowlabel"		{ return FLOWLABEL; }
 "nexthdr"		{ return NEXTHDR; }
@@ -512,10 +516,10 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "meta"			{ return META; }
 "mark"			{ return MARK; }
 "iif"			{ return IIF; }
-"iifname"		{ return IIFNAME; }
+"iifname"		{ BEGIN(spec); nspec = 1; return IIFNAME; }
 "iiftype"		{ return IIFTYPE; }
 "oif"			{ return OIF; }
-"oifname"		{ return OIFNAME; }
+"oifname"		{ BEGIN(spec); nspec = 1; return OIFNAME; }
 "oiftype"		{ return OIFTYPE; }
 "skuid"			{ return SKUID; }
 "skgid"			{ return SKGID; }
@@ -613,7 +617,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				return STRING;
 			}
 
-{numberstring}		{
+<*>{numberstring}	{
+				if (nspec && !--nspec)
+					BEGIN(0);
 				errno = 0;
 				yylval->val = strtoull(yytext, NULL, 0);
 				if (errno != 0) {
@@ -639,7 +645,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				return ASTERISK_STRING;
 			}
 
-{string}		{
+<*>{string}		{
+				if (nspec && !--nspec)
+					BEGIN(0);
 				yylval->string = xstrdup(yytext);
 				return STRING;
 			}
@@ -648,23 +656,23 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				reset_pos(yyget_extra(yyscanner), yylloc);
 			}
 
-{newline}		{
+<*>{newline}		{
 				reset_pos(yyget_extra(yyscanner), yylloc);
 				return NEWLINE;
 			}
 
-{tab}+
-{space}+
-{comment}
+<*>{tab}+
+<*>{space}+
+<*>{comment}
 
-<<EOF>> 		{
+<*><<EOF>> 		{
 				update_pos(yyget_extra(yyscanner), yylloc, 1);
 				scanner_pop_buffer(yyscanner);
 				if (YY_CURRENT_BUFFER == NULL)
 					return TOKEN_EOF;
 			}
 
-.			{ return JUNK; }
+<*>.			{ return JUNK; }
 
 %%
 

Re: Unable to create a chain called "trace"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Mon, Feb 08, 2021 at 06:14:44PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > In general, shells eating the quotes is problematic and users may not be
> > > aware of it. This includes scripts that mangle ruleset dumps by
> > > accident, etc. (Not sure if it is really a problem as we quote some
> > > strings already).
> > > 
> > > Using JSON, there are no such limits, BTW. I really wonder if there's
> > > really no fix for bison parser to make it "context aware".
> > 
> > Right.  We can probably make lots of keywords available for table/chain names
> > by only recognizing them while parsing rules, i.e. via 'start conditions'
> > in flex.  But I don't think there is anyone with the time to do the
> > needed scanner changes.
> 
> Oh, I wasn't aware of start conditions at all, thanks for the pointer.
> Instead of reducing most keyword's scope to rule context, I tried a less
> intrusive approach, namely recognizing "only strings plus some extra" in
> certain conditions. See attached patch for reference. With it in place,
> I was at least able to:
> 
> # nft add table inet table
> # nft add chain inet table chain
> # nft add rule inet table chain iifname rule

Thanks.  Another bug report about this problem arrived moments ago,
this time 'add rule filter dynamic'

Whats worse is that 3rd party tool created a chain with that name.

So I fear we can't really release a new nft version without a new
scanner that passes 'string' outside of rule context.

Phils patch makes this work but breaks the test suite.

>  "=="			{ return EQ; }
>  
> -{numberstring}		{
> +<*>{numberstring}	{
> +				if (nspec && !--nspec)
> +					BEGIN(0);
>  				errno = 0;
>  				yylval->val = strtoull(yytext, NULL, 0);
>  				if (errno != 0) {
> @@ -639,7 +645,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  				return ASTERISK_STRING;
>  			}
>  
> -{string}		{
> +<*>{string}		{
> +				if (nspec && !--nspec)
> +					BEGIN(0);

Not sure this is a good way to go, it looks rather fragile.

Seems we need allow "{" for "*" and then count the {} nests so
we can pop off a scanner state stack once we make it back to the
same } level that we had at the last state switch.

Re: Unable to create a chain called "trace"

[Phil Sutter]

On Fri, Feb 12, 2021 at 01:05:07AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Mon, Feb 08, 2021 at 06:14:44PM +0100, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > In general, shells eating the quotes is problematic and users may not be
> > > > aware of it. This includes scripts that mangle ruleset dumps by
> > > > accident, etc. (Not sure if it is really a problem as we quote some
> > > > strings already).
> > > > 
> > > > Using JSON, there are no such limits, BTW. I really wonder if there's
> > > > really no fix for bison parser to make it "context aware".
> > > 
> > > Right.  We can probably make lots of keywords available for table/chain names
> > > by only recognizing them while parsing rules, i.e. via 'start conditions'
> > > in flex.  But I don't think there is anyone with the time to do the
> > > needed scanner changes.
> > 
> > Oh, I wasn't aware of start conditions at all, thanks for the pointer.
> > Instead of reducing most keyword's scope to rule context, I tried a less
> > intrusive approach, namely recognizing "only strings plus some extra" in
> > certain conditions. See attached patch for reference. With it in place,
> > I was at least able to:
> > 
> > # nft add table inet table
> > # nft add chain inet table chain
> > # nft add rule inet table chain iifname rule
> 
> Thanks.  Another bug report about this problem arrived moments ago,
> this time 'add rule filter dynamic'
> 
> Whats worse is that 3rd party tool created a chain with that name.

This is easily possible using JSON API alone as that doesn't have the
naming limitations. Depending on personal interpretation, this could
mean the problem is far worse than "some exotic tool is able to disturb
nft" or it is actually pretty common and "just another case of 'nft list
ruleset | nft -f -' being broken". I tend towards the latter, but agree
that it is a serious issue.

> So I fear we can't really release a new nft version without a new
> scanner that passes 'string' outside of rule context.
> 
> Phils patch makes this work but breaks the test suite.

It was merely a quick hack. :)

> >  "=="			{ return EQ; }
> >  
> > -{numberstring}		{
> > +<*>{numberstring}	{
> > +				if (nspec && !--nspec)
> > +					BEGIN(0);
> >  				errno = 0;
> >  				yylval->val = strtoull(yytext, NULL, 0);
> >  				if (errno != 0) {
> > @@ -639,7 +645,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
> >  				return ASTERISK_STRING;
> >  			}
> >  
> > -{string}		{
> > +<*>{string}		{
> > +				if (nspec && !--nspec)
> > +					BEGIN(0);
> 
> Not sure this is a good way to go, it looks rather fragile.

I didn't find a better way to conditionally parse two following args as
strings instead of just a single one. Basically I miss an explicit end
condition from which to call BEGIN(0).

> Seems we need allow "{" for "*" and then count the {} nests so
> we can pop off a scanner state stack once we make it back to the
> same } level that we had at the last state switch.

What is the problem?

Thanks, Phil

Re: Unable to create a chain called "trace"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> I didn't find a better way to conditionally parse two following args as
> strings instead of just a single one. Basically I miss an explicit end
> condition from which to call BEGIN(0).

Yes, thats part of the problem.

> > Seems we need allow "{" for "*" and then count the {} nests so
> > we can pop off a scanner state stack once we make it back to the
> > same } level that we had at the last state switch.
> 
> What is the problem?

Detect when we need to exit the current start condition.

We may not even be able to do BEGIN(0) if we have multiple, nested
start conditionals. flex supports start condition stacks, but that
still leaves the exit/closure issue.

Example:

table chain {
 chain bla {  /* should start to recognize rules, but
		 we did not see 'rule' keyword */
	ip saddr { ... } /* can't exit rule start condition on } ... */
	ip daddr { ... }
 }  /* should disable rule keywords again */

 chain dynamic { /* so 'dynamic' is a string here ... */
 }
}

I don't see a solution, perhaps add dummy bison rule(s)
to explicitly signal closure of e.g. a rule context?

Re: Unable to create a chain called "trace"

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > I didn't find a better way to conditionally parse two following args as
> > strings instead of just a single one. Basically I miss an explicit end
> > condition from which to call BEGIN(0).
> 
> Yes, thats part of the problem.
> 
> > > Seems we need allow "{" for "*" and then count the {} nests so
> > > we can pop off a scanner state stack once we make it back to the
> > > same } level that we had at the last state switch.
> > 
> > What is the problem?
> 
> Detect when we need to exit the current start condition.
> 
> We may not even be able to do BEGIN(0) if we have multiple, nested
> start conditionals. flex supports start condition stacks, but that
> still leaves the exit/closure issue.
> 
> Example:
> 
> table chain {
>  chain bla {  /* should start to recognize rules, but
> 		 we did not see 'rule' keyword */
> 	ip saddr { ... } /* can't exit rule start condition on } ... */
> 	ip daddr { ... }
>  }  /* should disable rule keywords again */
> 
>  chain dynamic { /* so 'dynamic' is a string here ... */
>  }
> }
> 
> I don't see a solution, perhaps add dummy bison rule(s)
> to explicitly signal closure of e.g. a rule context?

It should also be possible to add an explicit rule to allow for
keywords to be used as table/chain/... identifier.

It should be possible to add a test script in the infrastructure to
create table/chain/... using keywords, to make sure this does not
break.

It's not nice, but it's simple and we don't mingle with flex.

I have attached an example patchset (see patch 2/2), it's incomplete.
I could also have a look at adding such regression test.

[-- Attachment #2: 0001-parser_bison-rename-chain_identifier-to-chain_block_.patch --]
[-- Type: text/x-diff, Size: 2549 bytes --]

From 84ee11474385fe67f551486c9bbcc94e387ba927 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 12 Feb 2021 17:59:29 +0100
Subject: [PATCH 1/2] parser_bison: rename chain_identifier to
 chain_block_identifier

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/parser_bison.y | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 11e899ff2f20..825f134c33ff 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -588,8 +588,8 @@ int nft_lex(void *, void *, void *);
 %type <cmd>			base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd
 %destructor { cmd_free($$); }	base_cmd add_cmd replace_cmd create_cmd insert_cmd delete_cmd get_cmd list_cmd reset_cmd flush_cmd rename_cmd export_cmd monitor_cmd describe_cmd import_cmd
 
-%type <handle>			table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
-%destructor { handle_free(&$$); } table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
+%type <handle>			table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_block_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
+%destructor { handle_free(&$$); } table_spec tableid_spec chain_spec chainid_spec flowtable_spec chain_block_identifier ruleid_spec handle_spec position_spec rule_position ruleset_spec index_spec
 %type <handle>			set_spec setid_spec set_identifier flowtableid_spec flowtable_identifier obj_spec objid_spec obj_identifier
 %destructor { handle_free(&$$); } set_spec setid_spec set_identifier flowtableid_spec obj_spec objid_spec obj_identifier
 %type <val>			family_spec family_spec_explicit
@@ -1576,7 +1576,7 @@ table_block		:	/* empty */	{ $$ = $<table>-1; }
 			|	table_block	common_block
 			|	table_block	stmt_separator
 			|	table_block	table_options	stmt_separator
-			|	table_block	CHAIN		chain_identifier
+			|	table_block	CHAIN		chain_block_identifier
 					chain_block_alloc	'{' 	chain_block	'}'
 					stmt_separator
 			{
@@ -2463,7 +2463,7 @@ chainid_spec 		: 	table_spec 	HANDLE NUM
 			}
 			;
 
-chain_identifier	:	identifier
+chain_block_identifier	:	identifier
 			{
 				memset(&$$, 0, sizeof($$));
 				$$.chain.name		= $1;
-- 
2.20.1


[-- Attachment #3: 0002-parser_bison-allow-for-keywords-to-be-used-as-table-.patch --]
[-- Type: text/x-diff, Size: 1965 bytes --]

From f77efb5f662d24c03bf2ef5fd0bca0345dd3054c Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 12 Feb 2021 18:02:04 +0100
Subject: [PATCH 2/2] parser_bison: allow for keywords to be used as table and
 chain identifiers

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/parser_bison.y | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 825f134c33ff..9937bd511c6e 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -574,8 +574,8 @@ int nft_lex(void *, void *, void *);
 %token IN			"in"
 %token OUT			"out"
 
-%type <string>			identifier type_identifier string comment_spec
-%destructor { xfree($$); }	identifier type_identifier string comment_spec
+%type <string>			identifier type_identifier string comment_spec table_identifier chain_identifier keyword_identifier
+%destructor { xfree($$); }	identifier type_identifier string comment_spec table_identifier chain_identifier keyword_identifier
 
 %type <val>			time_spec quota_used
 
@@ -2429,7 +2429,14 @@ family_spec_explicit	:	IP		{ $$ = NFPROTO_IPV4; }
 			|	NETDEV		{ $$ = NFPROTO_NETDEV; }
 			;
 
-table_spec		:	family_spec	identifier
+keyword_identifier	:	DYNAMIC		{ $$ = xstrdup("dynamic"); }
+			;
+
+table_identifier	:	STRING
+			|	keyword_identifier
+			;
+
+table_spec		:	family_spec	table_identifier
 			{
 				memset(&$$, 0, sizeof($$));
 				$$.family	= $1;
@@ -2447,7 +2454,7 @@ tableid_spec 		: 	family_spec 	HANDLE NUM
 			}
 			;
 
-chain_spec		:	table_spec	identifier
+chain_spec		:	table_spec	chain_identifier
 			{
 				$$		= $1;
 				$$.chain.name	= $2;
@@ -2463,7 +2470,11 @@ chainid_spec 		: 	table_spec 	HANDLE NUM
 			}
 			;
 
-chain_block_identifier	:	identifier
+chain_identifier	:	STRING
+			|	keyword_identifier
+			;
+
+chain_block_identifier	:	chain_identifier
 			{
 				memset(&$$, 0, sizeof($$));
 				$$.chain.name		= $1;
-- 
2.20.1

Re: Unable to create a chain called "trace"

[Phil Sutter]

Hi,

On Fri, Feb 12, 2021 at 06:09:21PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > I didn't find a better way to conditionally parse two following args as
> > > strings instead of just a single one. Basically I miss an explicit end
> > > condition from which to call BEGIN(0).
> > 
> > Yes, thats part of the problem.
> > 
> > > > Seems we need allow "{" for "*" and then count the {} nests so
> > > > we can pop off a scanner state stack once we make it back to the
> > > > same } level that we had at the last state switch.
> > > 
> > > What is the problem?
> > 
> > Detect when we need to exit the current start condition.
> > 
> > We may not even be able to do BEGIN(0) if we have multiple, nested
> > start conditionals. flex supports start condition stacks, but that
> > still leaves the exit/closure issue.
> > 
> > Example:
> > 
> > table chain {
> >  chain bla {  /* should start to recognize rules, but
> > 		 we did not see 'rule' keyword */
> > 	ip saddr { ... } /* can't exit rule start condition on } ... */
> > 	ip daddr { ... }
> >  }  /* should disable rule keywords again */
> > 
> >  chain dynamic { /* so 'dynamic' is a string here ... */
> >  }
> > }
> > 
> > I don't see a solution, perhaps add dummy bison rule(s)
> > to explicitly signal closure of e.g. a rule context?
> 
> It should also be possible to add an explicit rule to allow for
> keywords to be used as table/chain/... identifier.

Which means we have to collect and maintain a list of all known keywords
which is at least error-prone.

> It should be possible to add a test script in the infrastructure to
> create table/chain/... using keywords, to make sure this does not
> break.

You mean something that auto-generates the list of keywords to try?

> It's not nice, but it's simple and we don't mingle with flex.
> 
> I have attached an example patchset (see patch 2/2), it's incomplete.
> I could also have a look at adding such regression test.

Ah, I tried that path but always ended with shift/reduce conflicts. They
appear when replacing DYNAMIC with e.g. TABLE, CHAIN or RULE in your
patch. Of course we may declare that none of those is a sane name for a
table, but I wonder if we'll discover less obvious cases later.

Cheers, Phil

Re: Unable to create a chain called "trace"

[Pablo Neira Ayuso]

On Fri, Feb 12, 2021 at 06:32:01PM +0100, Phil Sutter wrote:
> Hi,
> 
> On Fri, Feb 12, 2021 at 06:09:21PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > I didn't find a better way to conditionally parse two following args as
> > > > strings instead of just a single one. Basically I miss an explicit end
> > > > condition from which to call BEGIN(0).
> > > 
> > > Yes, thats part of the problem.
> > > 
> > > > > Seems we need allow "{" for "*" and then count the {} nests so
> > > > > we can pop off a scanner state stack once we make it back to the
> > > > > same } level that we had at the last state switch.
> > > > 
> > > > What is the problem?
> > > 
> > > Detect when we need to exit the current start condition.
> > > 
> > > We may not even be able to do BEGIN(0) if we have multiple, nested
> > > start conditionals. flex supports start condition stacks, but that
> > > still leaves the exit/closure issue.
> > > 
> > > Example:
> > > 
> > > table chain {
> > >  chain bla {  /* should start to recognize rules, but
> > > 		 we did not see 'rule' keyword */
> > > 	ip saddr { ... } /* can't exit rule start condition on } ... */
> > > 	ip daddr { ... }
> > >  }  /* should disable rule keywords again */
> > > 
> > >  chain dynamic { /* so 'dynamic' is a string here ... */
> > >  }
> > > }
> > > 
> > > I don't see a solution, perhaps add dummy bison rule(s)
> > > to explicitly signal closure of e.g. a rule context?
> > 
> > It should also be possible to add an explicit rule to allow for
> > keywords to be used as table/chain/... identifier.
> 
> Which means we have to collect and maintain a list of all known keywords
> which is at least error-prone.

You mean, someone might forget to update the list of keywords.

That's right.

> > It should be possible to add a test script in the infrastructure to
> > create table/chain/... using keywords, to make sure this does not
> > break.
> 
> You mean something that auto-generates the list of keywords to try?

Autogenerating this list would be good, I didn't good that far in
exploring this.

Or just making a shell script that extracts the %token lines to try to
create table with a keyword as a name.

The shell script would just have a "list of unallowed keyword" to
filter out the %tokens that are not allowed, for those tokens that are
really reserved keywords.

> > It's not nice, but it's simple and we don't mingle with flex.
> > 
> > I have attached an example patchset (see patch 2/2), it's incomplete.
> > I could also have a look at adding such regression test.
> 
> Ah, I tried that path but always ended with shift/reduce conflicts. They
> appear when replacing DYNAMIC with e.g. TABLE, CHAIN or RULE in your
> patch.

Probably we have to set some explicit restrictions, like table, chain,
rule, set, map and flowtable are reserved keywords. For example, not
allowing to call a table '>'. That was not possible since the
beginning anyway.

The concern is to add a new token and break backward as it happened
with 'dynamic' as Florian reported I think.

> Of course we may declare that none of those is a sane name for a
> table, but I wonder if we'll discover less obvious cases later.

BTW, Florian mentioned your patch makes unhappy the tests infra?
What's the issue?

Re: Unable to create a chain called "trace"

[Phil Sutter]

On Fri, Feb 12, 2021 at 06:54:23PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 12, 2021 at 06:32:01PM +0100, Phil Sutter wrote:
> > On Fri, Feb 12, 2021 at 06:09:21PM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> > > > Phil Sutter <phil@nwl.cc> wrote:
> > > > > I didn't find a better way to conditionally parse two following args as
> > > > > strings instead of just a single one. Basically I miss an explicit end
> > > > > condition from which to call BEGIN(0).
> > > > 
> > > > Yes, thats part of the problem.
> > > > 
> > > > > > Seems we need allow "{" for "*" and then count the {} nests so
> > > > > > we can pop off a scanner state stack once we make it back to the
> > > > > > same } level that we had at the last state switch.
> > > > > 
> > > > > What is the problem?
> > > > 
> > > > Detect when we need to exit the current start condition.
> > > > 
> > > > We may not even be able to do BEGIN(0) if we have multiple, nested
> > > > start conditionals. flex supports start condition stacks, but that
> > > > still leaves the exit/closure issue.
> > > > 
> > > > Example:
> > > > 
> > > > table chain {
> > > >  chain bla {  /* should start to recognize rules, but
> > > > 		 we did not see 'rule' keyword */
> > > > 	ip saddr { ... } /* can't exit rule start condition on } ... */
> > > > 	ip daddr { ... }
> > > >  }  /* should disable rule keywords again */
> > > > 
> > > >  chain dynamic { /* so 'dynamic' is a string here ... */
> > > >  }
> > > > }
> > > > 
> > > > I don't see a solution, perhaps add dummy bison rule(s)
> > > > to explicitly signal closure of e.g. a rule context?
> > > 
> > > It should also be possible to add an explicit rule to allow for
> > > keywords to be used as table/chain/... identifier.
> > 
> > Which means we have to collect and maintain a list of all known keywords
> > which is at least error-prone.
> 
> You mean, someone might forget to update the list of keywords.

Yes, every time a new keyword is introduced that list has to be updated.
Right now each introduced keyword may break someone's ruleset. This is
only avoided if that keyword list you propose is constantly kept up to
date.

This is the reason why I prefer to have a more intelligent parser which
just knows where something user-defined is supposed to be and not even
tries to parse it as something it knows.

> That's right.
> 
> > > It should be possible to add a test script in the infrastructure to
> > > create table/chain/... using keywords, to make sure this does not
> > > break.
> > 
> > You mean something that auto-generates the list of keywords to try?
> 
> Autogenerating this list would be good, I didn't good that far in
> exploring this.

Ah, I thought that's implied by your mention of a script. If it is
possible, it would at least help keep that list from above up to date.

> Or just making a shell script that extracts the %token lines to try to
> create table with a keyword as a name.
> 
> The shell script would just have a "list of unallowed keyword" to
> filter out the %tokens that are not allowed, for those tokens that are
> really reserved keywords.

sed -n 's/^"\([^"]*\)".*/\1/p' src/scanner.l | exclude_unwanted

> > > It's not nice, but it's simple and we don't mingle with flex.
> > > 
> > > I have attached an example patchset (see patch 2/2), it's incomplete.
> > > I could also have a look at adding such regression test.
> > 
> > Ah, I tried that path but always ended with shift/reduce conflicts. They
> > appear when replacing DYNAMIC with e.g. TABLE, CHAIN or RULE in your
> > patch.
> 
> Probably we have to set some explicit restrictions, like table, chain,
> rule, set, map and flowtable are reserved keywords. For example, not
> allowing to call a table '>'. That was not possible since the
> beginning anyway.

This topic constantly reminds me of C objects named like their type
'struct foo foo'. That's my personal proof that it must be possible! :)

> The concern is to add a new token and break backward as it happened
> with 'dynamic' as Florian reported I think.
> 
> > Of course we may declare that none of those is a sane name for a
> > table, but I wonder if we'll discover less obvious cases later.
> 
> BTW, Florian mentioned your patch makes unhappy the tests infra?
> What's the issue?

I didn't check. But I guess the nested syntax (chain within table) is a
problem with my simple "parse two strings now" approach. I would like to
play a bit more with that start condition approach. IMO teaching flex
how to interpret tokens based on earlier ones is a smart way of fixing
it.

Cheers, Phil

Re: Unable to create a chain called "trace"

[Balazs Scheidler]

On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > I didn't find a better way to conditionally parse two following args as
> > strings instead of just a single one. Basically I miss an explicit end
> > condition from which to call BEGIN(0).
> 
> Yes, thats part of the problem.
> 
> > > Seems we need allow "{" for "*" and then count the {} nests so
> > > we can pop off a scanner state stack once we make it back to the
> > > same } level that we had at the last state switch.
> > 
> > What is the problem?
> 
> Detect when we need to exit the current start condition.
> 
> We may not even be able to do BEGIN(0) if we have multiple, nested
> start conditionals. flex supports start condition stacks, but that
> still leaves the exit/closure issue.
> 
> Example:
> 
> table chain {
>  chain bla {  /* should start to recognize rules, but
> 		 we did not see 'rule' keyword */
> 	ip saddr { ... } /* can't exit rule start condition on } ... */
> 	ip daddr { ... }
>  }  /* should disable rule keywords again */
> 
>  chain dynamic { /* so 'dynamic' is a string here ... */
>  }
> }
> 
> I don't see a solution, perhaps add dummy bison rule(s)
> to explicitly signal closure of e.g. a rule context?

You can always push/pop the flexer state from bison code blocks, maybe
that's what you mean on "dummy bison rules".

Trigger the state from bison and make sure it ends.

Something like this:

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 11e899ff..d8107181 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2397,7 +2397,10 @@ chain_policy             :       ACCEPT          { $$ = NF_ACCEPT; }
 identifier             :       STRING
                        ;
 
-string                 :       STRING
+string                 :       { yy_push_state(scanner, STRING); } __string { yy_pop_state(scanner); }
+                       ;
+
+__string               :       STRING
                        |       QUOTED_STRING
                        |       ASTERISK_STRING
                        ;

-- 
Bazsi

Re: Unable to create a chain called "trace"

[Phil Sutter]

Hi,

On Fri, Feb 12, 2021 at 01:20:07PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > I didn't find a better way to conditionally parse two following args as
> > strings instead of just a single one. Basically I miss an explicit end
> > condition from which to call BEGIN(0).
> 
> Yes, thats part of the problem.
> 
> > > Seems we need allow "{" for "*" and then count the {} nests so
> > > we can pop off a scanner state stack once we make it back to the
> > > same } level that we had at the last state switch.
> > 
> > What is the problem?
> 
> Detect when we need to exit the current start condition.

I explored my approach further but ended up in an ugly situation due to
the use of 'set' keyword in rules: My code is not context-aware, so upon
recognizing 'set' keyword it switches to spec-condition. I can't simply
detect preceding command-keywords due to them being implicit in nested
notation.

> We may not even be able to do BEGIN(0) if we have multiple, nested
> start conditionals. flex supports start condition stacks, but that
> still leaves the exit/closure issue.
> 
> Example:
> 
> table chain {
>  chain bla {  /* should start to recognize rules, but
> 		 we did not see 'rule' keyword */

My code worked with this after enabling detection of '{' in all
conditions and making it call BEGIN(0) (regardless of nspec value).

> 	ip saddr { ... } /* can't exit rule start condition on } ... */

Maybe we could track nesting block depth in a simple counter?

> 	ip daddr { ... }
>  }  /* should disable rule keywords again */
> 
>  chain dynamic { /* so 'dynamic' is a string here ... */
>  }
> }
> 
> I don't see a solution, perhaps add dummy bison rule(s)
> to explicitly signal closure of e.g. a rule context?

We can't influence start conditions from within bison (if that's what
you had in mind). All we can do is try make flex aware of the current
input context. For instance, detect 'table' followed by '{' to open
"table definition context" and start tracking braces to detect its end.
Though after all I think your assessment of all this being "fragile" is
appropriate. :/

Cheers, Phil

Re: Unable to create a chain called "trace"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > I don't see a solution, perhaps add dummy bison rule(s)
> > to explicitly signal closure of e.g. a rule context?
> 
> We can't influence start conditions from within bison (if that's what
> you had in mind).

Why not?  in scanner.l:

void scanner_pop_start_cond(void *scanner)
{
        yy_pop_state(scanner);
}

And then call it from bison as 'scanner_pop_start_cond(nft->scanner)'.

Needs %stack in scanner.l so yy_pop_state() is defined.

bison has more clue as to when a expression/rule/set block etc
has finished parsing than the scanner.

Not sure tracking { } nesting depth is enough with a pure scanner.l
approach.

Re: Unable to create a chain called "trace"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > grammar bug.
> > 
> > Pablo, Phil, others, can you remind me why we never did:
> 
> Because this would be followed up by:
> 
> | Subject: Unable to create a table called "trace"
> 
> Jokes aside:
> 
> I think Pablo didn't like the obvious consequence of having to quote
> *all* string types which are user-defined in output. He played with
> keeping the quotes as part of the name, so they are sent to kernel and
> in listing they would automatically appear quoted. I don't quite
> remember why this was problematic, though.
>
> In general, shells eating the quotes is problematic and users may not be
> aware of it. This includes scripts that mangle ruleset dumps by
> accident, etc. (Not sure if it is really a problem as we quote some
> strings already).

Ok, but what if we just allow use of quotes in input?
That would at least allow to use nft to delete/add to chains created
by other tools.

Re: Unable to create a chain called "trace"

[Phil Sutter]

On Fri, Feb 12, 2021 at 01:29:23PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > grammar bug.
> > > 
> > > Pablo, Phil, others, can you remind me why we never did:
> > 
> > Because this would be followed up by:
> > 
> > | Subject: Unable to create a table called "trace"
> > 
> > Jokes aside:
> > 
> > I think Pablo didn't like the obvious consequence of having to quote
> > *all* string types which are user-defined in output. He played with
> > keeping the quotes as part of the name, so they are sent to kernel and
> > in listing they would automatically appear quoted. I don't quite
> > remember why this was problematic, though.
> >
> > In general, shells eating the quotes is problematic and users may not be
> > aware of it. This includes scripts that mangle ruleset dumps by
> > accident, etc. (Not sure if it is really a problem as we quote some
> > strings already).
> 
> Ok, but what if we just allow use of quotes in input?
> That would at least allow to use nft to delete/add to chains created
> by other tools.

IIRC, this was deemed to make things worse as people may more easily
create rulesets which break with 'nft list ruleset | nft -f -'. But that
point won't hold anymore now, I guess. :D

Extracting the changes to parser_bison.y from my patch in
| Message-Id: <20190116184613.31698-1-phil@nwl.cc>
might suffice already.

Cheers, Phil