Re: [PATCH] BUG12265: firewall: iptables rules are being created in the wrong chain

Hello Alex,

thank you for providing a second version of this. Sorry for my very late reply.

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.) 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 connections, 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 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.

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

Thanks, and best regards, Peter Müller

Fixes: #12265

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

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@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 # @@ -427,7 +427,7 @@ sub get_address }

# 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@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_tgt';
  

• $fwdfwsettings{$fwdfwsettings{'grp2'}} = 'GREEN';
  

• }
• if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings{'grp2'}} eq $netsettings{'ORANGE_ADDRESS'}){

• $fwdfwsettings{'grp2'} = 'ipfire_tgt';
  

• $fwdfwsettings{$fwdfwsettings{'grp2'}} = 'ORANGE';
  

• }
• if ($fwdfwsettings{'grp2'} eq 'tgt_addr' && $fwdfwsettings{$fwdfwsettings{'grp2'}} eq $netsettings{'BLUE_ADDRESS'}){

• $fwdfwsettings{'grp2'} = 'ipfire_tgt';
  

• $fwdfwsettings{$fwdfwsettings{'grp2'}} = 'BLUE';
  

• } $errormessage=&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=$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'}=$configinput; $fwdfwsettings{'chain'} = 'INPUTFW'; $maxkey=&General::findhasharraykey(%configinputfw);

@@ -600,7 +614,7 @@ sub checktarget } } #check empty fields

• if ($fwdfwsettings{$fwdfwsettings{'grp2'}} eq ''){ $errormessage.=$Lang::tr{'fwdfw err notgt'}."<br>";}

• if ($fwdfwsettings{$fwdfwsettings{'grp2'}} eq '' && !$fwdfwsettings{'grp2'}){ $errormessage.=$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'}} = '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){

@@ -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';
  

@@ -1751,18 +1765,18 @@ END &Header::openbox('100%', 'left', $Lang::tr{'fwdfw target'}); print<<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>
  

• <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_tgt' value='ipfire_tgt'  $checked{'grp2'}{'ipfire_tgt'}></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;