From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter =?utf-8?q?M=C3=BCller?= <peter.mueller@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] BUG12265: firewall: iptables rules are being created in
 the wrong chain
Date: Tue, 20 Apr 2021 22:59:32 +0200
Message-ID: <110ce696-12a8-8265-7c51-265bb7ad2225@ipfire.org>
In-Reply-To: <20210330130737.27113-1-alexander.marx@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0786960690435220102=="
List-Id: <development.lists.ipfire.org>

--===============0786960690435220102==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello Alex,

thank you for providing a second version of this. Sorry for my very late repl=
y.

This patch works fine on my testing machine running Core Update 156, with one=
 exception:
Before, it was possible to create the following firewall rule - albeit it was=
 created in
the wrong firewall chain -:

- source: ORANGE network
- destination: IPFire's ORANGE interface
- arbitrary port and protocol (I used 53 and UDP for testing, but it does not=
 matter)

Such a rule is now rejected as it's source and destination belong to the same=
 network,
which is correct. (I assume the generated rule before did not work anyway.) S=
ince the
current firewall default policy states no services are available for ORANGE, =
not even
DNS or NTP, I guess some firewall operators wrote rules allowing such connect=
ions, as it
might be better to fetch DNS and NTP data from the internet.

Creating such a rule would now be impossible for ORANGE entirely, but could s=
till be
created for individual IP addresses within ORANGE. This is fine to me, but I =
am not sure
if the rest of the list agrees.

@All: Is this side effect a show-stopper to you?

Thanks, and best regards,
Peter M=C3=BCller

> Fixes: #12265
>=20
> When creating a rule with a local network as source and a firewallinterface=
 as target, the rule was created in the FORWARD instead of INPUT chain.
> This one fixes that and additionally checks if a manual target ip address i=
s one of IPFire's interfaces and changes the rule accordingly.
> ---
>  config/firewall/firewall-lib.pl |  4 ++--
>  html/cgi-bin/firewall.cgi       | 42 ++++++++++++++++++++++-----------
>  2 files changed, 30 insertions(+), 16 deletions(-)
>=20
> diff --git a/config/firewall/firewall-lib.pl b/config/firewall/firewall-lib=
.pl
> index bc0b30ca5..26d357ea2 100644
> --- a/config/firewall/firewall-lib.pl
> +++ b/config/firewall/firewall-lib.pl
> @@ -2,7 +2,7 @@
>  ##########################################################################=
#####
>  #                                                                         =
    #
>  # IPFire.org - A linux based firewall                                     =
    #
> -# Copyright (C) 2013 Alexander Marx <amarx(a)ipfire.org>                  =
      #
> +# Copyright (C) 2021 Alexander Marx <amarx(a)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    =
    #
> @@ -427,7 +427,7 @@ sub get_address
>  		}
> =20
>  	# The firewall's own IP addresses.
> -	} elsif ($key ~~ ["ipfire", "ipfire_src"]) {
> +	} elsif ($key ~~ ["ipfire_tgt", "ipfire_src"]) {
>  		# ALL
>  		if ($value eq "ALL") {
>  			push(@ret, ["0/0", ""]);
> diff --git a/html/cgi-bin/firewall.cgi b/html/cgi-bin/firewall.cgi
> index 532f99f91..6c9b7e9a7 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(a)ipfire.org>                  =
      #
> +# Copyright (C) 2021 Alexander Marx <amarx(a)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'} =3D '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'} =3D 'ipfire_tgt';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} =3D 'GREEN';
> +	}
> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings=
{'grp2'}} eq $netsettings{'ORANGE_ADDRESS'}){
> +		$fwdfwsettings{'grp2'} =3D 'ipfire_tgt';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} =3D 'ORANGE';
> +	}
> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings=
{'grp2'}} eq $netsettings{'BLUE_ADDRESS'}){
> +		$fwdfwsettings{'grp2'} =3D 'ipfire_tgt';
> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} =3D 'BLUE';
> +	}
>  	$errormessage=3D&checksource;
>  	if(!$errormessage){&checktarget;}
>  	if(!$errormessage){&checkrule;}
> @@ -243,11 +257,11 @@ if ($fwdfwsettings{'ACTION'} eq 'saverule')
>  		}
>  	}
>  	#check if we try to break rules
> -	if(	$fwdfwsettings{'grp1'} eq 'ipfire_src' && $fwdfwsettings{'grp2'} eq '=
ipfire'){
> +	if(	$fwdfwsettings{'grp1'} eq 'ipfire_src' && $fwdfwsettings{'grp2'} eq '=
ipfire_tgt'){
>  		$errormessage=3D$Lang::tr{'fwdfw err same'};
>  	}
>  	# INPUT part
> -	if ($fwdfwsettings{'grp2'} eq 'ipfire' && $fwdfwsettings{$fwdfwsettings{'=
grp1'}} ne 'ORANGE'){
> +	if ($fwdfwsettings{'grp2'} eq 'ipfire_tgt'){
>  		$fwdfwsettings{'config'}=3D$configinput;
>  		$fwdfwsettings{'chain'} =3D 'INPUTFW';
>  		$maxkey=3D&General::findhasharraykey(\%configinputfw);
> @@ -600,7 +614,7 @@ sub checktarget
>  		}
>  	}
>  	#check empty fields
> -	if ($fwdfwsettings{$fwdfwsettings{'grp2'}} eq ''){ $errormessage.=3D$Lang=
::tr{'fwdfw err notgt'}."<br>";}
> +	if ($fwdfwsettings{$fwdfwsettings{'grp2'}} eq '' && !$fwdfwsettings{'grp2=
'}){ $errormessage.=3D$Lang::tr{'fwdfw err notgt'}."<br>";}
>  	#check tgt services
>  	if ($fwdfwsettings{'USESRV'} eq 'ON'){
>  		if ($fwdfwsettings{'grp3'} eq 'cust_srv'){
> @@ -1512,7 +1526,7 @@ sub newrule
>  	$checked{'USE_NAT'}{$fwdfwsettings{'USE_NAT'}} 			=3D 'CHECKED';
>  	$selected{'TIME_FROM'}{$fwdfwsettings{'TIME_FROM'}}		=3D 'selected';
>  	$selected{'TIME_TO'}{$fwdfwsettings{'TIME_TO'}}			=3D 'selected';
> -	$selected{'ipfire'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} =3D'selected';
> +	$selected{'ipfire_tgt'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} =3D'selec=
ted';
>  	$selected{'ipfire_src'}{$fwdfwsettings{$fwdfwsettings{'grp1'}}} =3D'selec=
ted';
>  	#check if update and get values
>  	if($fwdfwsettings{'updatefwrule'} eq 'on' || $fwdfwsettings{'copyfwrule'}=
 eq 'on' && !$errormessage){
> @@ -1584,7 +1598,7 @@ sub newrule
>  				$checked{'RATE_LIMIT'}{$fwdfwsettings{'RATE_LIMIT'}}	=3D 'CHECKED';
>  				$selected{'TIME_FROM'}{$fwdfwsettings{'TIME_FROM'}}		=3D 'selected';
>  				$selected{'TIME_TO'}{$fwdfwsettings{'TIME_TO'}}			=3D 'selected';
> -				$selected{'ipfire'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} =3D'select=
ed';
> +				$selected{'ipfire_tgt'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} =3D'se=
lected';
>  				$selected{'ipfire_src'}{$fwdfwsettings{$fwdfwsettings{'grp1'}}} =3D'se=
lected';
>  				$selected{'dnat'}{$fwdfwsettings{'dnat'}}				=3D'selected';
>  				$selected{'snat'}{$fwdfwsettings{'snat'}}				=3D'selected';
> @@ -1751,18 +1765,18 @@ END
>  		&Header::openbox('100%', 'left', $Lang::tr{'fwdfw target'});
>  		print<<END;
>  		<table width=3D'100%' border=3D'0'>=09
> -		<tr><td width=3D'1%'><input type=3D'radio' name=3D'grp2' value=3D'tgt_ad=
dr'  checked></td><td width=3D'60%' nowrap=3D'nowrap'>$Lang::tr{'fwdfw target=
ip'}<input type=3D'TEXT' name=3D'tgt_addr' value=3D'$fwdfwsettings{'tgt_addr'=
}' size=3D'16' maxlength=3D'18'><td width=3D'1%'><input type=3D'radio' name=
=3D'grp2' id=3D'ipfire' value=3D'ipfire'  $checked{'grp2'}{'ipfire'}></td><td=
><b>Firewall</b></td>
> +		<tr><td width=3D'1%'><input type=3D'radio' name=3D'grp2' value=3D'tgt_ad=
dr'  checked></td><td width=3D'60%' nowrap=3D'nowrap'>$Lang::tr{'fwdfw target=
ip'}<input type=3D'TEXT' name=3D'tgt_addr' value=3D'$fwdfwsettings{'tgt_addr'=
}' size=3D'16' maxlength=3D'18'><td width=3D'1%'><input type=3D'radio' name=
=3D'grp2' id=3D'ipfire_tgt' value=3D'ipfire_tgt'  $checked{'grp2'}{'ipfire_tg=
t'}></td><td><b>Firewall</b></td>
>  END
> -		print"<td align=3D'right'><select name=3D'ipfire' style=3D'width:200px;'=
>";
> -		print "<option value=3D'ALL' $selected{'ipfire'}{'ALL'}>$Lang::tr{'all'}=
</option>";
> -		print "<option value=3D'GREEN' $selected{'ipfire'}{'GREEN'}>$Lang::tr{'g=
reen'} ($ifaces{'GREEN_ADDRESS'})</option>" if $ifaces{'GREEN_ADDRESS'};
> -		print "<option value=3D'ORANGE' $selected{'ipfire'}{'ORANGE'}>$Lang::tr{=
'orange'} ($ifaces{'ORANGE_ADDRESS'})</option>" if (&Header::orange_used());
> -		print "<option value=3D'BLUE' $selected{'ipfire'}{'BLUE'}>$Lang::tr{'blu=
e'} ($ifaces{'BLUE_ADDRESS'})</option>"if (&Header::blue_used());
> -		print "<option value=3D'RED1' $selected{'ipfire'}{'RED1'}>$Lang::tr{'red=
1'} ($redip)" if ($redip);
> +		print"<td align=3D'right'><select name=3D'ipfire_tgt' style=3D'width:200=
px;'>";
> +		print "<option value=3D'ALL' $selected{'ipfire_tgt'}{'ALL'}>$Lang::tr{'a=
ll'}</option>";
> +		print "<option value=3D'GREEN' $selected{'ipfire_tgt'}{'GREEN'}>$Lang::t=
r{'green'} ($ifaces{'GREEN_ADDRESS'})</option>" if $ifaces{'GREEN_ADDRESS'};
> +		print "<option value=3D'ORANGE' $selected{'ipfire_tgt'}{'ORANGE'}>$Lang:=
:tr{'orange'} ($ifaces{'ORANGE_ADDRESS'})</option>" if (&Header::orange_used(=
));
> +		print "<option value=3D'BLUE' $selected{'ipfire_tgt'}{'BLUE'}>$Lang::tr{=
'blue'} ($ifaces{'BLUE_ADDRESS'})</option>"if (&Header::blue_used());
> +		print "<option value=3D'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=3D'$alias' $selected{'ipfire'}{$alias}>$alias ($a=
liases{$alias}{'IPT'})</option>";
> +				print "<option value=3D'$alias' $selected{'ipfire_tgt'}{$alias}>$alias=
 ($aliases{$alias}{'IPT'})</option>";
>  			}
>  		}
>  		print<<END;
>=20

--===============0786960690435220102==--