BUG12265: firewall: iptables rules are being created in the wrong chain
Commit Message
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(-)
Comments
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;
>
Hi All,
This is not a problem for me.
Regards,
Adolf
On 20/04/2021 22:59, Peter Müller wrote:
> 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;
>>
Hello,
> On 20 Apr 2021, at 21:59, Peter Müller <peter.mueller@ipfire.org> wrote:
>
> 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.
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 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?
This is a show-stopper for me.
-Michael
>
> 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;
>>
@@ -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", ""]);
@@ -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;