On Fri, Feb 12, 2021 at 01:05:07AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> wrote:
> > On Mon, Feb 08, 2021 at 06:14:44PM +0100, Florian Westphal wrote:
> > > Phil Sutter <phil@xxxxxx> 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
