[PATCH] 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(-)
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;
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);
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>"; }
-
Alexander Marx -
Peter Müller