[PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[Pablo Neira Ayuso]

iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP

shows:

nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop

which prints burst twice, this is not correct.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 extensions/libxt_hashlimit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
index 472d8e7f6cc2..3fa5719127db 100644
--- a/extensions/libxt_hashlimit.c
+++ b/extensions/libxt_hashlimit.c
@@ -1350,10 +1350,12 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
 
 	if (cfg->mode & XT_HASHLIMIT_BYTES)
 		print_bytes_rate_xlate(xl, cfg);
-	else
+	else {
 		print_packets_rate_xlate(xl, cfg->avg, revision);
-	if (cfg->burst != 5)
-		xt_xlate_add(xl, " burst %lu packets", cfg->burst);
+		if (cfg->burst != XT_HASHLIMIT_BURST)
+			xt_xlate_add(xl, " burst %lu packets", cfg->burst);
+
+	}
 	xt_xlate_add(xl, "}");
 
 	return ret;
-- 
2.11.0

Re: [PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[Duncan Roe]

On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote:
> iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP
>
> shows:
>
> nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop
>
> which prints burst twice, this is not correct.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  extensions/libxt_hashlimit.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
> index 472d8e7f6cc2..3fa5719127db 100644
> --- a/extensions/libxt_hashlimit.c
> +++ b/extensions/libxt_hashlimit.c
> @@ -1350,10 +1350,12 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
>
>  	if (cfg->mode & XT_HASHLIMIT_BYTES)
>  		print_bytes_rate_xlate(xl, cfg);
> -	else
> +	else {
>  		print_packets_rate_xlate(xl, cfg->avg, revision);
> -	if (cfg->burst != 5)
> -		xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> +		if (cfg->burst != XT_HASHLIMIT_BURST)
> +			xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> +
> +	}
>  	xt_xlate_add(xl, "}");
>
>  	return ret;
> --
> 2.11.0
>
This still discards a timeout of 1s (1000ms):

> $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr limit rate over 200 kbytes/second burst 1 mbytes} counter drop

This is especially incorrect, since the code deliberately inserts a default
timeout of 1m if no timeout was specified with a burst:

> $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP
> nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop

The patch I suggested doesn't have that problem, because of forcing defaults to
zero. Can doing that have any adverse side-effects?

Cheers ... Duncan.

And another thing

[Duncan Roe]

On Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote:
> On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote:
> > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP
> >
> > shows:
> >
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop
> >
> > which prints burst twice, this is not correct.
> >
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
Not actually related to the patch, but I happened to notice:

> 769   if (info->cfg.mode & XT_HASHLIMIT_BYTES) {
> 770     uint32_t burst = 0;
> 771     if (cb->xflags & F_BURST) {
> 772       if (info->cfg.burst < cost_to_bytes(info->cfg.avg))
> 773         xtables_error(PARAMETER_PROBLEM,
> 774           "burst cannot be smaller than %lub", cost_to_bytes(info->cfg.avg));
> 775
> 776       burst = info->cfg.burst;
> 777       burst /= cost_to_bytes(info->cfg.avg);
> 778       if (info->cfg.burst % cost_to_bytes(info->cfg.avg))
> 779         burst++;
> 780       if (!(cb->xflags & F_HTABLE_EXPIRE))
> 781         info->cfg.expire = XT_HASHLIMIT_BYTE_EXPIRE_BURST * 1000;
> 782     }
> 783     info->cfg.burst = burst;
> 784   } else if (info->cfg.burst > XT_HASHLIMIT_BURST_MAX)
> 785     burst_error();

What is that final "else" claues there for? No hashlimit was specified so why
check its value?

Cheers ... Duncan.

Re: [PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[Pablo Neira Ayuso]

On Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote:
> On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote:
> > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP
> >
> > shows:
> >
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop
> >
> > which prints burst twice, this is not correct.
> >
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  extensions/libxt_hashlimit.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
> > index 472d8e7f6cc2..3fa5719127db 100644
> > --- a/extensions/libxt_hashlimit.c
> > +++ b/extensions/libxt_hashlimit.c
> > @@ -1350,10 +1350,12 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> >
> >  	if (cfg->mode & XT_HASHLIMIT_BYTES)
> >  		print_bytes_rate_xlate(xl, cfg);
> > -	else
> > +	else {
> >  		print_packets_rate_xlate(xl, cfg->avg, revision);
> > -	if (cfg->burst != 5)
> > -		xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> > +		if (cfg->burst != XT_HASHLIMIT_BURST)
> > +			xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> > +
> > +	}
> >  	xt_xlate_add(xl, "}");
> >
> >  	return ret;
> > --
> > 2.11.0
> >
> This still discards a timeout of 1s (1000ms):
> 
> > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr limit rate over 200 kbytes/second burst 1 mbytes} counter drop
> 
> This is especially incorrect, since the code deliberately inserts a default
> timeout of 1m if no timeout was specified with a burst:
> 
> > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop
> 
> The patch I suggested doesn't have that problem, because of forcing defaults to
> zero. Can doing that have any adverse side-effects?

Yes. Problem is that we cannot assume that hashlimit_mt_check() is
called. If you compile nftables with --with-xtables, listing of rules
that are added via iptables-compat will be translated to nftables.

Re: [PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[Duncan Roe]

On Thu, Jan 04, 2018 at 10:53:28AM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote:
> > On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote:
> > > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP
> > >
> > > shows:
> > >
> > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop
> > >
> > > which prints burst twice, this is not correct.
> > >
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > >  extensions/libxt_hashlimit.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
> > > index 472d8e7f6cc2..3fa5719127db 100644
> > > --- a/extensions/libxt_hashlimit.c
> > > +++ b/extensions/libxt_hashlimit.c
> > > @@ -1350,10 +1350,12 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> > >
> > >  	if (cfg->mode & XT_HASHLIMIT_BYTES)
> > >  		print_bytes_rate_xlate(xl, cfg);
> > > -	else
> > > +	else {
> > >  		print_packets_rate_xlate(xl, cfg->avg, revision);
> > > -	if (cfg->burst != 5)
> > > -		xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> > > +		if (cfg->burst != XT_HASHLIMIT_BURST)
> > > +			xt_xlate_add(xl, " burst %lu packets", cfg->burst);
> > > +
> > > +	}
> > >  	xt_xlate_add(xl, "}");
> > >
> > >  	return ret;
> > > --
> > > 2.11.0
> > >
> > This still discards a timeout of 1s (1000ms):
> >
> > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr limit rate over 200 kbytes/second burst 1 mbytes} counter drop
> >
> > This is especially incorrect, since the code deliberately inserts a default
> > timeout of 1m if no timeout was specified with a burst:
> >
> > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP
> > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop
> >
> > The patch I suggested doesn't have that problem, because of forcing defaults to
> > zero. Can doing that have any adverse side-effects?
>
> Yes. Problem is that we cannot assume that hashlimit_mt_check() is
> called. If you compile nftables with --with-xtables, listing of rules
> that are added via iptables-compat will be translated to nftables.

OK on that. But see my comments to your latest patch. I guess it should be
harmless to introduce a flags byte that no other code is aware of?

Cheers ... Duncan.