BUG12265: firewall: iptables rules are being created in the wrong chain Fixes: #12265

  When creating a rule like Source:Orange and Target:green IPfire Interface,
the rule was created in the forward instead of input chain.

This patch sets correct chain and additionally checks
if a single target ip (when set) is one of the ipfire interface ip addresses.
If this is the case, the target is automatically changed to IPFIRE interface instead of single target ip.
---
 html/cgi-bin/firewall.cgi | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)
  

Hello Alex,

thank you for submitting this one.

Unfortunately, this patch is not behaving correctly on my testing machine (running Core Update 155):

A firewall rule with source = ORANGE and destination = firewall | GREEN IP is still displayed in the
wrong section in the WebUI. Worse, the destination IP address is silently omitted, resulting in 
destination = any instead.

This faulty behaviour can be reproduced by creating and modifying firewall rules for arbitrary source
networks and arbitrary IP addresses of IPFire's interface as a destination.

Oddly enough, when accidentally leaving "destination IP" checked without entering one (why does the
WebUI even accept such rules?!), the rule is created in the correct section ("incoming firewall access"),
with it's target section displayed in blue and without an IP address - the IPFire machine, however, does
not even _have_ a BLUE interface.

Whatever goes wrong here seems to go seriously wrong.

Thanks, and best regards,
Peter Müller


> When creating a rule like Source:Orange and Target:green IPfire Interface,
> the rule was created in the forward instead of input chain.
> 
> This patch sets correct chain and additionally checks
> if a single target ip (when set) is one of the ipfire interface ip addresses.
> If this is the case, the target is automatically changed to IPFIRE interface instead of single target ip.
> ---
>  html/cgi-bin/firewall.cgi | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/html/cgi-bin/firewall.cgi b/html/cgi-bin/firewall.cgi
> index 532f99f91..c680eed1d 100644
> --- a/html/cgi-bin/firewall.cgi
> +++ b/html/cgi-bin/firewall.cgi
> @@ -2,7 +2,7 @@
>  ###############################################################################
>  #                                                                             #
>  # IPFire.org - A linux based firewall                                         #
> -# Copyright (C) 2013 Alexander Marx <amarx@ipfire.org>                        #
> +# Copyright (C) 2021 Alexander Marx <amarx@ipfire.org>                        #
>  #                                                                             #
>  # This program is free software: you can redistribute it and/or modify        #
>  # it under the terms of the GNU General Public License as published by        #
> @@ -213,6 +213,7 @@ if ($fwdfwsettings{'ACTION'} eq 'saverule')
>  	&General::readhasharray("$configfwdfw", \%configfwdfw);
>  	&General::readhasharray("$configinput", \%configinputfw);
>  	&General::readhasharray("$configoutgoing", \%configoutgoingfw);
> +	&General::readhash("/var/ipfire/ethernet/settings", \%netsettings);
>  	my $maxkey;
>  	#Set Variables according to the JQuery code in protocol section
>  	if ($fwdfwsettings{'PROT'} eq 'TCP' || $fwdfwsettings{'PROT'} eq 'UDP')
> @@ -231,6 +232,19 @@ if ($fwdfwsettings{'ACTION'} eq 'saverule')
>  	{
>  		$fwdfwsettings{'USESRV'} = 'ON';
>  	}
> +	#Check if manual targetip is one of IPFire addresses
> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings{'grp2'}} eq $netsettings{'GREEN_ADDRESS'}){
> +		$fwdfwsettings{'grp2'} = 'ipfire';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} = 'GREEN';
> +	}
> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings{'grp2'}} eq $netsettings{'ORANGE_ADDRESS'}){
> +		$fwdfwsettings{'grp2'} = 'ipfire';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} = 'ORANGE';
> +	}
> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings{'grp2'}} eq $netsettings{'BLUE_ADDRESS'}){
> +		$fwdfwsettings{'grp2'} = 'ipfire';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} = 'BLUE';
> +	}
>  	$errormessage=&checksource;
>  	if(!$errormessage){&checktarget;}
>  	if(!$errormessage){&checkrule;}
> @@ -247,7 +261,7 @@ if ($fwdfwsettings{'ACTION'} eq 'saverule')
>  		$errormessage=$Lang::tr{'fwdfw err same'};
>  	}
>  	# INPUT part
> -	if ($fwdfwsettings{'grp2'} eq 'ipfire' && $fwdfwsettings{$fwdfwsettings{'grp1'}} ne 'ORANGE'){
> +	if ($fwdfwsettings{'grp2'} eq 'ipfire'){
>  		$fwdfwsettings{'config'}=$configinput;
>  		$fwdfwsettings{'chain'} = 'INPUTFW';
>  		$maxkey=&General::findhasharraykey(\%configinputfw);
> @@ -1512,7 +1526,7 @@ sub newrule
>  	$checked{'USE_NAT'}{$fwdfwsettings{'USE_NAT'}} 			= 'CHECKED';
>  	$selected{'TIME_FROM'}{$fwdfwsettings{'TIME_FROM'}}		= 'selected';
>  	$selected{'TIME_TO'}{$fwdfwsettings{'TIME_TO'}}			= 'selected';
> -	$selected{'ipfire'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} ='selected';
> +	$selected{'ipfire tgt'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} ='selected';
>  	$selected{'ipfire_src'}{$fwdfwsettings{$fwdfwsettings{'grp1'}}} ='selected';
>  	#check if update and get values
>  	if($fwdfwsettings{'updatefwrule'} eq 'on' || $fwdfwsettings{'copyfwrule'} eq 'on' && !$errormessage){
> @@ -1526,7 +1540,7 @@ sub newrule
>  				$fwdfwsettings{'ACTIVE'}				= $hash{$key}[2];
>  				$fwdfwsettings{'grp1'}					= $hash{$key}[3];   
>  				$fwdfwsettings{$fwdfwsettings{'grp1'}}	= $hash{$key}[4];   
> -				$fwdfwsettings{'grp2'}					= $hash{$key}[5];   
> +				$fwdfwsettings{'grp2'}					= $hash{$key}[5];
>  				$fwdfwsettings{$fwdfwsettings{'grp2'}}	= $hash{$key}[6];   
>  				$fwdfwsettings{'USE_SRC_PORT'}			= $hash{$key}[7];
>  				$fwdfwsettings{'PROT'}					= $hash{$key}[8];
> @@ -1584,7 +1598,7 @@ sub newrule
>  				$checked{'RATE_LIMIT'}{$fwdfwsettings{'RATE_LIMIT'}}	= 'CHECKED';
>  				$selected{'TIME_FROM'}{$fwdfwsettings{'TIME_FROM'}}		= 'selected';
>  				$selected{'TIME_TO'}{$fwdfwsettings{'TIME_TO'}}			= 'selected';
> -				$selected{'ipfire'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} ='selected';
> +				$selected{'ipfire tgt'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} ='selected';
>  				$selected{'ipfire_src'}{$fwdfwsettings{$fwdfwsettings{'grp1'}}} ='selected';
>  				$selected{'dnat'}{$fwdfwsettings{'dnat'}}				='selected';
>  				$selected{'snat'}{$fwdfwsettings{'snat'}}				='selected';
> @@ -1753,16 +1767,16 @@ END
>  		<table width='100%' border='0'>	
>  		<tr><td width='1%'><input type='radio' name='grp2' value='tgt_addr'  checked></td><td width='60%' nowrap='nowrap'>$Lang::tr{'fwdfw targetip'}<input type='TEXT' name='tgt_addr' value='$fwdfwsettings{'tgt_addr'}' size='16' maxlength='18'><td width='1%'><input type='radio' name='grp2' id='ipfire' value='ipfire'  $checked{'grp2'}{'ipfire'}></td><td><b>Firewall</b></td>
>  END
> -		print"<td align='right'><select name='ipfire' style='width:200px;'>";
> -		print "<option value='ALL' $selected{'ipfire'}{'ALL'}>$Lang::tr{'all'}</option>";
> -		print "<option value='GREEN' $selected{'ipfire'}{'GREEN'}>$Lang::tr{'green'} ($ifaces{'GREEN_ADDRESS'})</option>" if $ifaces{'GREEN_ADDRESS'};
> -		print "<option value='ORANGE' $selected{'ipfire'}{'ORANGE'}>$Lang::tr{'orange'} ($ifaces{'ORANGE_ADDRESS'})</option>" if (&Header::orange_used());
> -		print "<option value='BLUE' $selected{'ipfire'}{'BLUE'}>$Lang::tr{'blue'} ($ifaces{'BLUE_ADDRESS'})</option>"if (&Header::blue_used());
> -		print "<option value='RED1' $selected{'ipfire'}{'RED1'}>$Lang::tr{'red1'} ($redip)" if ($redip);
> +		print"<td align='right'><select name='ipfire tgt' style='width:200px;'>";
> +		print "<option value='ALL' $selected{'ipfire tgt'}{'ALL'}>$Lang::tr{'all'}</option>";
> +		print "<option value='GREEN' $selected{'ipfire tgt'}{'GREEN'}>$Lang::tr{'green'} ($ifaces{'GREEN_ADDRESS'})</option>" if $ifaces{'GREEN_ADDRESS'};
> +		print "<option value='ORANGE' $selected{'ipfire tgt'}{'ORANGE'}>$Lang::tr{'orange'} ($ifaces{'ORANGE_ADDRESS'})</option>" if (&Header::orange_used());
> +		print "<option value='BLUE' $selected{'ipfire tgt'}{'BLUE'}>$Lang::tr{'blue'} ($ifaces{'BLUE_ADDRESS'})</option>"if (&Header::blue_used());
> +		print "<option value='RED1' $selected{'ipfire tgt'}{'RED1'}>$Lang::tr{'red1'} ($redip)" if ($redip);
>  		if (! -z "${General::swroot}/ethernet/aliases"){
>  			foreach my $alias (sort keys %aliases)
>  			{
> -				print "<option value='$alias' $selected{'ipfire'}{$alias}>$alias ($aliases{$alias}{'IPT'})</option>";
> +				print "<option value='$alias' $selected{'ipfire tgt'}{$alias}>$alias ($aliases{$alias}{'IPT'})</option>";
>  			}
>  		}
>  		print<<END;
>