From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] BUG12265: firewall: iptables rules are being created in
 the wrong chain
Date: Thu, 22 Apr 2021 17:38:59 +0100
Message-ID: <A0586589-E52D-4454-855A-DBC1EDBD26C5@ipfire.org>
In-Reply-To: <110ce696-12a8-8265-7c51-265bb7ad2225@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2040821590024365197=="
List-Id: <development.lists.ipfire.org>

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

Hello,

> On 20 Apr 2021, at 21:59, Peter M=C3=BCller <peter.mueller(a)ipfire.org> wr=
ote:
>=20
> Hello Alex,
>=20
> thank you for providing a second version of this. Sorry for my very late re=
ply.
>=20
> This patch works fine on my testing machine running Core Update 156, with o=
ne exception:
> Before, it was possible to create the following firewall rule - albeit it w=
as created in
> the wrong firewall chain -:
>=20
> - source: ORANGE network
> - destination: IPFire's ORANGE interface
> - arbitrary port and protocol (I used 53 and UDP for testing, but it does n=
ot matter)
>=20
> Such a rule is now rejected as it's source and destination belong to the sa=
me network,
> which is correct.

Why is this correct? ORANGE is closed by default and it should be possible to=
 expose services that are running on the firewall to the ORANGE network.

> (I assume the generated rule before did not work anyway.)

Yes, that is correct and what was supposed to be fixed in this patch.

> Since 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 conne=
ctions, as it
> might be better to fetch DNS and NTP data from the internet.
>=20
> Creating such a rule would now be impossible for ORANGE entirely, but could=
 still 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.
>=20
> @All: Is this side effect a show-stopper to you?

This is a show-stopper for me.

-Michael

>=20
> Thanks, and best regards,
> Peter M=C3=BCller
>=20
>> Fixes: #12265
>>=20
>> When creating a rule with a local network as source and a firewallinterfac=
e 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 =
is 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-li=
b.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{$fwdfwsetting=
s{'grp2'}} eq $netsettings{'GREEN_ADDRESS'}){
>> +		$fwdfwsettings{'grp2'} =3D 'ipfire_tgt';
>> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} =3D 'GREEN';
>> +	}
>> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsetting=
s{'grp2'}} eq $netsettings{'ORANGE_ADDRESS'}){
>> +		$fwdfwsettings{'grp2'} =3D 'ipfire_tgt';
>> +		$fwdfwsettings{$fwdfwsettings{'grp2'}} =3D 'ORANGE';
>> +	}
>> +	if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsetting=
s{'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$Lan=
g::tr{'fwdfw err notgt'}."<br>";}
>> +	if ($fwdfwsettings{$fwdfwsettings{'grp2'}} eq '' && !$fwdfwsettings{'grp=
2'}){ $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'sele=
cted';
>> 	$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'selec=
ted';
>> +				$selected{'ipfire_tgt'}{$fwdfwsettings{$fwdfwsettings{'grp2'}}} =3D's=
elected';
>> 				$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_a=
ddr'  checked></td><td width=3D'60%' nowrap=3D'nowrap'>$Lang::tr{'fwdfw targe=
tip'}<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_a=
ddr'  checked></td><td width=3D'60%' nowrap=3D'nowrap'>$Lang::tr{'fwdfw targe=
tip'}<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{'=
green'} ($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{'bl=
ue'} ($ifaces{'BLUE_ADDRESS'})</option>"if (&Header::blue_used());
>> -		print "<option value=3D'RED1' $selected{'ipfire'}{'RED1'}>$Lang::tr{'re=
d1'} ($redip)" if ($redip);
>> +		print"<td align=3D'right'><select name=3D'ipfire_tgt' style=3D'width:20=
0px;'>";
>> +		print "<option value=3D'ALL' $selected{'ipfire_tgt'}{'ALL'}>$Lang::tr{'=
all'}</option>";
>> +		print "<option value=3D'GREEN' $selected{'ipfire_tgt'}{'GREEN'}>$Lang::=
tr{'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 ($=
aliases{$alias}{'IPT'})</option>";
>> +				print "<option value=3D'$alias' $selected{'ipfire_tgt'}{$alias}>$alia=
s ($aliases{$alias}{'IPT'})</option>";
>> 			}
>> 		}
>> 		print<<END;
>>=20


--===============2040821590024365197==--