extensions: hashlimit: fix incorrect burst in translations

PATCH In review Pablo Neira Ayuso 2 participants 5 messages Jan 3, 2018 – Jan 4, 2018 netfilter

Share & tools

Pablo Neira Ayuso

author maintainer

2018-01-03 14:41 UTC #0 lore raw

PATCH 1/1 extensions: hashlimit: fix incorrect burst in translations

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 <[email protected]>

---
 extensions/libxt_hashlimit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

extensions/libxt_hashlimit.c +5 -3

index 472d8e7f6cc2..3fa5719127db 100644
@@ -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;

Signature · 1 line

2.11.0

Duncan Roe

2018-01-03 22:26 UTC #1 lore raw

Quoted 34 lines · 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 <[email protected]>
---
 extensions/libxt_hashlimit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
extensions/libxt_hashlimit.c +5 -4
index 472d8e7f6cc2..3fa5719127db 100644
@@ -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;
--

Quoted 2 lines

2.11.0

This still discards a timeout of 1s (1000ms):

Quoted 2 lines

$ 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:

Quoted 2 lines

$ 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.

Duncan Roe

2018-01-03 22:59 UTC #2 lore raw

And another thing

Quoted 11 lines · Duncan Roe

Quoted 10 lines · 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 <[email protected]>
---

Not actually related to the patch, but I happened to notice:

Quoted 10 lines

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))

Quoted 7 lines

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.

Pablo Neira Ayuso

author maintainer

2018-01-04 09:53 UTC #3 lore raw

In reply to Duncan Roe

Quoted 42 lines · Duncan Roe

Quoted 34 lines · 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 <[email protected]>
---
 extensions/libxt_hashlimit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
extensions/libxt_hashlimit.c +5 -4
index 472d8e7f6cc2..3fa5719127db 100644
@@ -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;
--
Quoted 2 lines
2.11.0
This still discards a timeout of 1s (1000ms):
Quoted 2 lines
$ 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

Quoted 8 lines

This is especially incorrect, since the code deliberately inserts a default timeout of 1m if no timeout was specified with a burst:
Quoted 2 lines
$ 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.

Duncan Roe

2018-01-04 11:24 UTC #4 lore raw

Quoted 49 lines · Pablo Neira Ayuso

Quoted 48 lines · Duncan Roe
Quoted 34 lines · 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 <[email protected]>
---
 extensions/libxt_hashlimit.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
extensions/libxt_hashlimit.c +5 -4
index 472d8e7f6cc2..3fa5719127db 100644
@@ -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;
--
Quoted 2 lines
2.11.0
This still discards a timeout of 1s (1000ms):
Quoted 2 lines
$ 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:
Quoted 2 lines
$ 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

Quoted 6 lines

Quoted 2 lines
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.