[PATCH nft,v2 1/2] datatype: reject rate in quota statement

[PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Pablo Neira Ayuso]

Bail out if rate are used:

 ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
 add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
                                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

improve error reporting while at this.

Fixes: 6615676d825e ("src: add per-bytes limit")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: - change patch subject
    - use strndup() to fetch units in rate_parse() so limit rate does not break.

 src/datatype.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index d398a9c8c618..297c5d0409d5 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
 struct error_record *data_unit_parse(const struct location *loc,
 				     const char *str, uint64_t *rate)
 {
-	if (strncmp(str, "bytes", strlen("bytes")) == 0)
+	if (strcmp(str, "bytes") == 0)
 		*rate = 1ULL;
-	else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
+	else if (strcmp(str, "kbytes") == 0)
 		*rate = 1024;
-	else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
+	else if (strcmp(str, "mbytes") == 0)
 		*rate = 1024 * 1024;
 	else
-		return error(loc, "Wrong rate format");
+		return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
 
 	return NULL;
 }
@@ -1500,14 +1500,20 @@ struct error_record *data_unit_parse(const struct location *loc,
 struct error_record *rate_parse(const struct location *loc, const char *str,
 				uint64_t *rate, uint64_t *unit)
 {
+	const char *slash, *rate_str;
 	struct error_record *erec;
-	const char *slash;
 
 	slash = strchr(str, '/');
 	if (!slash)
-		return error(loc, "wrong rate format");
+		return error(loc, "wrong rate format, expecting {bytes,kbytes,mbytes}/{second,minute,hour,day,week}");
+
+	rate_str = strndup(str, slash - str);
+	if (!rate_str)
+		memory_allocation_error();
+
+	erec = data_unit_parse(loc, rate_str, rate);
+	free_const(rate_str);
 
-	erec = data_unit_parse(loc, str, rate);
 	if (erec != NULL)
 		return erec;
 
-- 
2.30.2

[PATCH nft,v2 2/2] datatype: improve error reporting when time unit is not correct

[Pablo Neira Ayuso]

Display:

  Wrong unit format, expecting bytes or kbytes or mbytes

instead of:

  Wrong rate format

Fixes: 6615676d825e ("src: add per-bytes limit")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/datatype.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/datatype.c b/src/datatype.c
index 297c5d0409d5..33fe471048ef 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1477,7 +1477,7 @@ static struct error_record *time_unit_parse(const struct location *loc,
 	else if (strcmp(str, "week") == 0)
 		*unit = 1ULL * 60 * 60 * 24 * 7;
 	else
-		return error(loc, "Wrong rate format");
+		return error(loc, "Wrong time format, expecting second or minute or hour or day or week");
 
 	return NULL;
 }
-- 
2.30.2

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Phil Sutter]

On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> Bail out if rate are used:
> 
>  ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
>  add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
>                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> improve error reporting while at this.
> 
> Fixes: 6615676d825e ("src: add per-bytes limit")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v2: - change patch subject
>     - use strndup() to fetch units in rate_parse() so limit rate does not break.
> 
>  src/datatype.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index d398a9c8c618..297c5d0409d5 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
>  struct error_record *data_unit_parse(const struct location *loc,
>  				     const char *str, uint64_t *rate)
>  {
> -	if (strncmp(str, "bytes", strlen("bytes")) == 0)
> +	if (strcmp(str, "bytes") == 0)
>  		*rate = 1ULL;
> -	else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> +	else if (strcmp(str, "kbytes") == 0)
>  		*rate = 1024;
> -	else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> +	else if (strcmp(str, "mbytes") == 0)
>  		*rate = 1024 * 1024;
>  	else
> -		return error(loc, "Wrong rate format");
> +		return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
>  
>  	return NULL;
>  }

I have local commits which introduce KBYTES and MBYTES keywords and
thereby kill the need for quota_unit and limit_bytes cases in
parser_bison.y. It still needs testing and is surely not solving all
issues there are, but I find it nicer than the partially redundant code
we have right now.

My motivation for this was to maybe improve parser's ability to handle
lack of spaces in input. I still see the scanner fall into the generic
"string" token case which requires manual dissection in the parser.

What is your motivation for the above changes? Maybe we could collect
parser limitations around these units and see what helps "the most"?

Cheers, Phil

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Pablo Neira Ayuso]

On Wed, Aug 14, 2024 at 06:00:55PM +0200, Phil Sutter wrote:
> On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> > Bail out if rate are used:
> > 
> >  ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
> >  add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
> >                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > improve error reporting while at this.
> > 
> > Fixes: 6615676d825e ("src: add per-bytes limit")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v2: - change patch subject
> >     - use strndup() to fetch units in rate_parse() so limit rate does not break.
> > 
> >  src/datatype.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/datatype.c b/src/datatype.c
> > index d398a9c8c618..297c5d0409d5 100644
> > --- a/src/datatype.c
> > +++ b/src/datatype.c
> > @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
> >  struct error_record *data_unit_parse(const struct location *loc,
> >  				     const char *str, uint64_t *rate)
> >  {
> > -	if (strncmp(str, "bytes", strlen("bytes")) == 0)
> > +	if (strcmp(str, "bytes") == 0)
> >  		*rate = 1ULL;
> > -	else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> > +	else if (strcmp(str, "kbytes") == 0)
> >  		*rate = 1024;
> > -	else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> > +	else if (strcmp(str, "mbytes") == 0)
> >  		*rate = 1024 * 1024;
> >  	else
> > -		return error(loc, "Wrong rate format");
> > +		return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
> >  
> >  	return NULL;
> >  }
> 
> I have local commits which introduce KBYTES and MBYTES keywords and
> thereby kill the need for quota_unit and limit_bytes cases in
> parser_bison.y. It still needs testing and is surely not solving all
> issues there are, but I find it nicer than the partially redundant code
> we have right now.

Is this allowing for compact representation? ie. kbytes/second,
because I remember this was the issue to follow this poor man
approach.

> My motivation for this was to maybe improve parser's ability to handle
> lack of spaces in input. I still see the scanner fall into the generic
> "string" token case which requires manual dissection in the parser.
> 
> What is your motivation for the above changes?

A user that accidentally used:

        quota over 10mbytes/second

which is currently accepted, the /second part is misleading, as the
commit described, this is now rejected after this patch.

> Maybe we could collect parser limitations around these units and see
> what helps "the most"?

I am fine with handling this from the parser instead, there is now
flex start conditions that can help to enable these tokens on demand.

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Phil Sutter]

On Wed, Aug 14, 2024 at 06:09:09PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 14, 2024 at 06:00:55PM +0200, Phil Sutter wrote:
> > On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> > > Bail out if rate are used:
> > > 
> > >  ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
> > >  add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
> > >                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > improve error reporting while at this.
> > > 
> > > Fixes: 6615676d825e ("src: add per-bytes limit")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > > v2: - change patch subject
> > >     - use strndup() to fetch units in rate_parse() so limit rate does not break.
> > > 
> > >  src/datatype.c | 20 +++++++++++++-------
> > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/datatype.c b/src/datatype.c
> > > index d398a9c8c618..297c5d0409d5 100644
> > > --- a/src/datatype.c
> > > +++ b/src/datatype.c
> > > @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
> > >  struct error_record *data_unit_parse(const struct location *loc,
> > >  				     const char *str, uint64_t *rate)
> > >  {
> > > -	if (strncmp(str, "bytes", strlen("bytes")) == 0)
> > > +	if (strcmp(str, "bytes") == 0)
> > >  		*rate = 1ULL;
> > > -	else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> > > +	else if (strcmp(str, "kbytes") == 0)
> > >  		*rate = 1024;
> > > -	else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> > > +	else if (strcmp(str, "mbytes") == 0)
> > >  		*rate = 1024 * 1024;
> > >  	else
> > > -		return error(loc, "Wrong rate format");
> > > +		return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
> > >  
> > >  	return NULL;
> > >  }
> > 
> > I have local commits which introduce KBYTES and MBYTES keywords and
> > thereby kill the need for quota_unit and limit_bytes cases in
> > parser_bison.y. It still needs testing and is surely not solving all
> > issues there are, but I find it nicer than the partially redundant code
> > we have right now.
> 
> Is this allowing for compact representation? ie. kbytes/second,
> because I remember this was the issue to follow this poor man
> approach.

You're right, I just checked and noticed my changes actually make things
worse, because the parser falls into the string case more often than
not. I wish there was a way to exclude string tokens via start
conditions.

For testing, I just removed the '{string}' case from scanner.l and
updated 'identifier' in parser_bison.y to accept QUOTED_STRING as well.
With that in place, all these parse correctly:

| nft add rule \"t\" \"c\" limit rate 1 kbytes/second
| nft add rule \"t\" \"c\" limit rate 1 kbytes/ second
| nft add rule \"t\" \"c\" limit rate 1 kbytes / second
| nft add rule \"t\" \"c\" limit rate 1 kbytes /second
| nft add rule \"t\" \"c\" limit rate 1kbytes/second
| nft add rule \"t\" \"c\" limit rate 1kbytes /second
| nft add rule \"t\" \"c\" limit rate 1kbytes/ second

> > My motivation for this was to maybe improve parser's ability to handle
> > lack of spaces in input. I still see the scanner fall into the generic
> > "string" token case which requires manual dissection in the parser.
> > 
> > What is your motivation for the above changes?
> 
> A user that accidentally used:
> 
>         quota over 10mbytes/second
> 
> which is currently accepted, the /second part is misleading, as the
> commit described, this is now rejected after this patch.

I see. Similar, but distinct bug. :)

> > Maybe we could collect parser limitations around these units and see
> > what helps "the most"?
> 
> I am fine with handling this from the parser instead, there is now
> flex start conditions that can help to enable these tokens on demand.

Maybe one could introduce a start condition which allows strings, but
it might turn into a mess given the wide use of them. I'll give it a try
and let you know.

Thanks, Phil

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Phil Sutter]

On Wed, Aug 14, 2024 at 09:26:36PM +0200, Phil Sutter wrote:
[...]
> Maybe one could introduce a start condition which allows strings, but
> it might turn into a mess given the wide use of them. I'll give it a try
> and let you know.

Looks like I hit a dead end there: For expressions like 'iif', we have
to accept STRING on RHS and since I need a token to push SC_STRING, I
can't just enable it for all relational expressions. The alternative is
to enable it for the whole rule but I can't disable it selectively (as I
had to enable it again afterwards without knowing what's next. :(

Cheers, Phil

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Aug 16, 2024 at 02:25:12PM +0200, Phil Sutter wrote:
> On Wed, Aug 14, 2024 at 09:26:36PM +0200, Phil Sutter wrote:
> [...]
> > Maybe one could introduce a start condition which allows strings, but
> > it might turn into a mess given the wide use of them. I'll give it a try
> > and let you know.
> 
> Looks like I hit a dead end there: For expressions like 'iif', we have
> to accept STRING on RHS and since I need a token to push SC_STRING, I
> can't just enable it for all relational expressions. The alternative is
> to enable it for the whole rule but I can't disable it selectively (as I
> had to enable it again afterwards without knowing what's next. :(

flex rules also tells what to find first (order implies priority)
maybe a combination of start conditions to carefully placing. I can
take my poor man fix by now so this can be revisited later :)

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Phil Sutter]

On Mon, Aug 19, 2024 at 12:47:02PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Aug 16, 2024 at 02:25:12PM +0200, Phil Sutter wrote:
> > On Wed, Aug 14, 2024 at 09:26:36PM +0200, Phil Sutter wrote:
> > [...]
> > > Maybe one could introduce a start condition which allows strings, but
> > > it might turn into a mess given the wide use of them. I'll give it a try
> > > and let you know.
> > 
> > Looks like I hit a dead end there: For expressions like 'iif', we have
> > to accept STRING on RHS and since I need a token to push SC_STRING, I
> > can't just enable it for all relational expressions. The alternative is
> > to enable it for the whole rule but I can't disable it selectively (as I
> > had to enable it again afterwards without knowing what's next. :(
> 
> flex rules also tells what to find first (order implies priority)
> maybe a combination of start conditions to carefully placing. I can
> take my poor man fix by now so this can be revisited later :)

I have a working draft using an exclusive start condition (Florian
pointed me to that). It passes the testsuite, might need more work
though (the first token after the limit statement is still parsed in the
exclusive condition, so all statement openers must potentially be valid
in all conditions.

Anyway, it's not a simple fix from my side so please go ahead and we'll
discuss my patches later as the "academic project" they are.

Cheers, Phil

Re: [PATCH nft,v2 1/2] datatype: reject rate in quota statement

[Pablo Neira Ayuso]

On Mon, Aug 19, 2024 at 05:18:55PM +0200, Phil Sutter wrote:
> On Mon, Aug 19, 2024 at 12:47:02PM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Fri, Aug 16, 2024 at 02:25:12PM +0200, Phil Sutter wrote:
> > > On Wed, Aug 14, 2024 at 09:26:36PM +0200, Phil Sutter wrote:
> > > [...]
> > > > Maybe one could introduce a start condition which allows strings, but
> > > > it might turn into a mess given the wide use of them. I'll give it a try
> > > > and let you know.
> > > 
> > > Looks like I hit a dead end there: For expressions like 'iif', we have
> > > to accept STRING on RHS and since I need a token to push SC_STRING, I
> > > can't just enable it for all relational expressions. The alternative is
> > > to enable it for the whole rule but I can't disable it selectively (as I
> > > had to enable it again afterwards without knowing what's next. :(
> > 
> > flex rules also tells what to find first (order implies priority)
> > maybe a combination of start conditions to carefully placing. I can
> > take my poor man fix by now so this can be revisited later :)
> 
> I have a working draft using an exclusive start condition (Florian
> pointed me to that). It passes the testsuite, might need more work
> though (the first token after the limit statement is still parsed in the
> exclusive condition, so all statement openers must potentially be valid
> in all conditions.
> 
> Anyway, it's not a simple fix from my side so please go ahead and we'll
> discuss my patches later as the "academic project" they are.

ok, pushed it out, thanks