[PATCH] fwhosts.cgi: Fix check to limit amount of ports in custom service groups.

[PATCH] fwhosts.cgi: Fix check to limit amount of ports in custom service groups.

[Stefan Schantl]

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

iptables multiport only supports up to 15 elements for each protocol (TCP or UDP).
That can be single ports or portranges (they count doubble).

This commit extends the check to calculate the amount of used TCP and/or
UDP ports of all existing entries in a group, by increasing the amount
for the service which should be added.

If the amount of ports for TCP or UDP ports become greater than the
limit of 15 the error message will be displayed.

Fixes #11323.

Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
---
 html/cgi-bin/fwhosts.cgi | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/html/cgi-bin/fwhosts.cgi b/html/cgi-bin/fwhosts.cgi
index 35611ac08..f6c7227ce 100644
--- a/html/cgi-bin/fwhosts.cgi
+++ b/html/cgi-bin/fwhosts.cgi
@@ -818,10 +818,28 @@ if ($fwhostsettings{'ACTION'} eq 'saveservicegrp')
 			}
 		}
 	}
-	if ($tcpcounter > 14){
+
+	# Loop through the hash of configured services.
+	foreach my $key (keys %customservice) {
+		# Assign nice human-readable values.
+		my $service_name = $customservice{$key}[0];
+		my $service_port = $customservice{$key}[1];
+		my $service_proto = $customservice{$key}[2];
+
+		# Skip services unless the processed one has found.
+		next unless $service_name eq $fwhostsettings{'CUST_SRV'};
+
+		# Increase the counters.
+		$tcpcounter++ if $service_proto eq 'TCP';
+		$tcpcounter++ if $service_proto eq 'TCP' && $service_port =~ m/:/i;
+		$udpcounter++ if $service_proto eq 'UDP';
+		$udpcounter++ if $service_proto eq 'UDP' && $service_port =~ m/:/i;
+	}
+
+	if ($tcpcounter > 15) {
 		$errormessage=$Lang::tr{'fwhost err maxservicetcp'};
 	}
-	if ($udpcounter > 14){
+	if ($udpcounter > 15) {
 		$errormessage=$Lang::tr{'fwhost err maxserviceudp'};
 	}
 	$tcpcounter=0;
-- 
2.30.2

Re: [PATCH] fwhosts.cgi: Fix check to limit amount of ports in custom service groups.

[Michael Tremer]

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

Hello,

I agree with the fix, but it would be better if we could hide this from the user.

I am not sure whether people have reported running into this, but the more sensible approach would have been to simply generate two iptables rules if there are more than the maximum number of ports being used in a group.

Since this has not been reported yet, I guess we will just leave it as it is.

Best,
-Michael

Reviewed-by: Michael Tremer <michael.tremer(a)ipfire.org>

> On 15 Jul 2021, at 11:07, Stefan Schantl <stefan.schantl(a)ipfire.org> wrote:
> 
> iptables multiport only supports up to 15 elements for each protocol (TCP or UDP).
> That can be single ports or portranges (they count doubble).
> 
> This commit extends the check to calculate the amount of used TCP and/or
> UDP ports of all existing entries in a group, by increasing the amount
> for the service which should be added.
> 
> If the amount of ports for TCP or UDP ports become greater than the
> limit of 15 the error message will be displayed.
> 
> Fixes #11323.
> 
> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> ---
> html/cgi-bin/fwhosts.cgi | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/html/cgi-bin/fwhosts.cgi b/html/cgi-bin/fwhosts.cgi
> index 35611ac08..f6c7227ce 100644
> --- a/html/cgi-bin/fwhosts.cgi
> +++ b/html/cgi-bin/fwhosts.cgi
> @@ -818,10 +818,28 @@ if ($fwhostsettings{'ACTION'} eq 'saveservicegrp')
> 			}
> 		}
> 	}
> -	if ($tcpcounter > 14){
> +
> +	# Loop through the hash of configured services.
> +	foreach my $key (keys %customservice) {
> +		# Assign nice human-readable values.
> +		my $service_name = $customservice{$key}[0];
> +		my $service_port = $customservice{$key}[1];
> +		my $service_proto = $customservice{$key}[2];
> +
> +		# Skip services unless the processed one has found.
> +		next unless $service_name eq $fwhostsettings{'CUST_SRV'};
> +
> +		# Increase the counters.
> +		$tcpcounter++ if $service_proto eq 'TCP';
> +		$tcpcounter++ if $service_proto eq 'TCP' && $service_port =~ m/:/i;
> +		$udpcounter++ if $service_proto eq 'UDP';
> +		$udpcounter++ if $service_proto eq 'UDP' && $service_port =~ m/:/i;
> +	}
> +
> +	if ($tcpcounter > 15) {
> 		$errormessage=$Lang::tr{'fwhost err maxservicetcp'};
> 	}
> -	if ($udpcounter > 14){
> +	if ($udpcounter > 15) {
> 		$errormessage=$Lang::tr{'fwhost err maxserviceudp'};
> 	}
> 	$tcpcounter=0;
> -- 
> 2.30.2
> 

Re: [PATCH] fwhosts.cgi: Fix check to limit amount of ports in custom service groups.

[Bernhard Bitsch]

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

I agree with Michael's statement.
A set of ports greater than 15 should be very unusual and should be 
structured logically further.
A error messages reminds to this case.

Best,
Bernhard

Reviewed-by: Bernhard Bitsch <bbitsch(a)ipfire.org>

Am 15.07.2021 um 17:45 schrieb Michael Tremer:
> Hello,
> 
> I agree with the fix, but it would be better if we could hide this from the user.
> 
> I am not sure whether people have reported running into this, but the more sensible approach would have been to simply generate two iptables rules if there are more than the maximum number of ports being used in a group.
> 
> Since this has not been reported yet, I guess we will just leave it as it is.
> 
> Best,
> -Michael
> 
> Reviewed-by: Michael Tremer <michael.tremer(a)ipfire.org>
> 
>> On 15 Jul 2021, at 11:07, Stefan Schantl <stefan.schantl(a)ipfire.org> wrote:
>>
>> iptables multiport only supports up to 15 elements for each protocol (TCP or UDP).
>> That can be single ports or portranges (they count doubble).
>>
>> This commit extends the check to calculate the amount of used TCP and/or
>> UDP ports of all existing entries in a group, by increasing the amount
>> for the service which should be added.
>>
>> If the amount of ports for TCP or UDP ports become greater than the
>> limit of 15 the error message will be displayed.
>>
>> Fixes #11323.
>>
>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>> ---
>> html/cgi-bin/fwhosts.cgi | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/html/cgi-bin/fwhosts.cgi b/html/cgi-bin/fwhosts.cgi
>> index 35611ac08..f6c7227ce 100644
>> --- a/html/cgi-bin/fwhosts.cgi
>> +++ b/html/cgi-bin/fwhosts.cgi
>> @@ -818,10 +818,28 @@ if ($fwhostsettings{'ACTION'} eq 'saveservicegrp')
>> 			}
>> 		}
>> 	}
>> -	if ($tcpcounter > 14){
>> +
>> +	# Loop through the hash of configured services.
>> +	foreach my $key (keys %customservice) {
>> +		# Assign nice human-readable values.
>> +		my $service_name = $customservice{$key}[0];
>> +		my $service_port = $customservice{$key}[1];
>> +		my $service_proto = $customservice{$key}[2];
>> +
>> +		# Skip services unless the processed one has found.
>> +		next unless $service_name eq $fwhostsettings{'CUST_SRV'};
>> +
>> +		# Increase the counters.
>> +		$tcpcounter++ if $service_proto eq 'TCP';
>> +		$tcpcounter++ if $service_proto eq 'TCP' && $service_port =~ m/:/i;
>> +		$udpcounter++ if $service_proto eq 'UDP';
>> +		$udpcounter++ if $service_proto eq 'UDP' && $service_port =~ m/:/i;
>> +	}
>> +
>> +	if ($tcpcounter > 15) {
>> 		$errormessage=$Lang::tr{'fwhost err maxservicetcp'};
>> 	}
>> -	if ($udpcounter > 14){
>> +	if ($udpcounter > 15) {
>> 		$errormessage=$Lang::tr{'fwhost err maxserviceudp'};
>> 	}
>> 	$tcpcounter=0;
>> -- 
>> 2.30.2
>>
>