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 [thread overview]
Message-ID: <A0586589-E52D-4454-855A-DBC1EDBD26C5@ipfire.org> (raw)
In-Reply-To: <110ce696-12a8-8265-7c51-265bb7ad2225@ipfire.org>
[-- Attachment #1: Type: text/plain, Size: 10835 bytes --]
Hello,
> On 20 Apr 2021, at 21:59, Peter Müller <peter.mueller(a)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(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
>> }
>>
>> # 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'} = '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;
>>
prev parent reply other threads:[~2021-04-22 16:38 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-30 13:07 Alexander Marx
2021-04-20 20:59 ` Peter Müller
2021-04-21 8:09 ` Adolf Belka
2021-04-22 16:38 ` Michael Tremer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A0586589-E52D-4454-855A-DBC1EDBD26C5@ipfire.org \
--to=michael.tremer@ipfire.org \
--cc=development@lists.ipfire.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox