bin/153252: [ipfw][patch] ipfw lockdown system in subsequent call of "/etc/rc.d/ipfw start"

Wed Dec 22 15:04:01 UTC 2010

Ian Smith <smithi at nimnet.asn.au> wrote:

> Please treat this as a general discussion of issues, as well as your PR.
> Try not taking critique too defensively.  Perhaps a language problem;

I'd always like to learn from people, but if someone can't see
difference between /sbin/ipfw and /etc/rc.d/ipfw and point wrongly to manual
page of /sbin/ipfw(8) while talking many times about some special secret
'right way' how to run _START UP_ script - I just simply trying to explain
that is wrong in my opinion, so no any defense or aggression :)

Ian Smith <smithi at nimnet.asn.au> wrote:
> There may be a easier solution to this problem than having start fail if 
> the firewall is already enabled .. that is, simply disable the firewall 
> in ipfw_prestart(); it's enabled again in ipfw_poststart() and as you 
> see there, it'd be necessary to test for and disable both IPv4 and IPv6 
> firewalls anyway.

I don't think that using ipfw_prestart()/ipfw_poststart() mechanism is 
a good way to resolve this issue. As Eugene already point it out - it will 
disable firewall for a while that will create window when there will be no 
firewall protection at all.

The simplest way to satisfy everybody is to add on the 
top of "/etc/rc.d/ipfw" and "/etc/rc.firewall" this string:
trap ':' 1 2 5 15;
It will guaranty that script doesn't stop in a middle when 
${fwcmd} -f flush
command will be executed that broke connection anyway but at least 
it will allow to finish apply firewall rules and then will be possible to
relogin again, but I don't think it is a nice solution.

IMHO reloading firewall's rules should be handled by additional command such
as "reload" or similar but not in the "start" action because "start" must mean 
"start" only.
Does anybody can start running again for example without prior stopping?
Start must be start. Official documentation doesn't provide any 
explanation how to handle in a special way some particular _start up_ scripts.
Right way to applying firewall's rules over network explained very well for
/sbin/ipfw but not for /etc/rc.d/ipfw and I believe it is right.

I think it should be kept simple - start up script load initial firewall's
rule and after that one should handle reloading of firewall's rules with
own scripts by calling /sbin/ipfw that is intended exactly to do this kind
of actions.

Running command "start" twice is meaningless. If one want to reload 
firewall's rules with 
/etc/rc.d/ipfw then there should be implemented safe command "reload"
or something else, but I don't think it is a good idea.
/etc/rc.d/ipfw is a mechanism that generally 
control service - enable it or not.
If one need just reload firewall's rules it could be done by running
/etc/rc.firewall script enclosed in guarding script instead of 
"/etc/rc.d/ipfw start", something like this:

/bin/sh -T -c "trap 'exit 1;' 1 2 ; /bin/sh /etc/rc.firewall;"

or simply

/usr/bin/nohup /bin/sh /etc/rc.firewall

in case if /usr already mounted.

Ian Smith <smithi at nimnet.asn.au> wrote:
> One of the reasons that people might want to run 'start' again when it's 
> already running is described in (at least) both of:
I'm sorry, but running twice '/etc/rc.d/ipfw start' isn't solution in that case.
I think the right solution is to fix logical bug and run natd first before 
loading firewall rules instead of running "start" command twice.
(BTW, it's impossible to run '/etc/rc.d/ipfw start' on boot twice,
 so it could be a solution)

Ian Smith <smithi at nimnet.asn.au> wrote:
> Yes, because you're running /etc/rc.d/ipfw start over the network, which 
> I think disabling the firewall in ipfw_prestart() should fix.  Comments?
Well, actually it wasn't me who "running '/etc/rc.d/ipfw start' over 
the network", it was my students who learning to manage FreeBSD. They 
wrote some script that accidentally called twice "/etc/rc.d/ipfw start" and
I explained them that they couldn't be born twice, so the ipfw service 
couldn't be logically started twice too. 

Add code below to /etc/rc.d/ipfw

#--------------
extra_commands='reload'
reload_cmd='ipfw_reload'

ipfw_reload() {
 /bin/sh -T -c "trap 'exit 1;' 1 2 ; /bin/sh /etc/rc.firewall;"
}
#--------------

add 

#--------------
trap ':' 1 2
#--------------

on top of /etc/rc.d/ipfw and /etc/rc.firewall

and you'll have safe way to reload firewall's rules if you want.

Disabling the firewall in ipfw_prestart() IMHO isn't right.

Ian Smith <smithi at nimnet.asn.au> wrote:
> Nonetheless, Eugene's advice is worthwhile, ./ in PATH is never a good 
> idea when discussing security, which is what a firewall is all about.
Well, I completely agree with you and Eugene on that if it come to a 
general security. This is kind of topic, but you forget that some 
projects don't need to be opened to the wild internet, it could be an 
automatic stations in closed environment where using ./ in the PATH can 
simplify things a lot without any security issues.
The point is that OS should protected itself from tricks that could be 
done with ./ in the PATH. Providing full path to the programs on an 
OS level is always a good idea.

Ian Smith <smithi at nimnet.asn.au> wrote:
> There's another part of your patch that Eugene didn't comment on that 
> caught my eye:
>
> -firewall_coscripts="/etc/rc.d/natd ${firewall_coscripts}"
> +
> +if checkyesno firewall_nat_enable; then
> +        firewall_coscripts="/etc/rc.d/natd ${firewall_coscripts}"
> +elif checkyesno natd_enable; then
> +        firewall_coscripts="/etc/rc.d/natd ${firewall_coscripts}"
> +fi
>
> Firstly, there's no problem with running /etc/rc.d/natd in any case, as 
> it won't do anything (ie is a NOP) unless natd_enable is set in rc.conf.

If it won't to do anything then why OS need to execute additional useless
operations? 
/etc/rc.d/ipfw is already checked in ipfw_prestart() function
- is nat needed or not, so no any reason to complicate it.
I prefer in source code clear logic - if it isn't needed then it 
shouldn't be run and this logic IMHO should be handled in one 
place to guaranty simplicity and independence. It just a couple of 
strings that wouldn't bloat OS but increase speed of boot process by
excluding useless operations.

Anyway all this code block could be excluded if nat will be loaded 
in ipfw_prestart() function that will fix issue shown in PR's:

http://www.freebsd.org/cgi/query-pr.cgi?pr=153155
http://www.freebsd.org/cgi/query-pr.cgi?pr=148137

and exclude useless operation such as 
firewall_coscripts="/etc/rc.d/natd ${firewall_coscripts}"
in the end of /etc/rc.d/ipfw

Eugene Grosbein <egrosbein at rdtc.ru> wrote:
> "unconditionally disable ability of reloading ipfw rules using
> '/etc/rc.d/ipfw start' command" - will this way of reloading ipfw rules
> work with your patch applied? It seems, no.
Eugene, don't take my words as offense please, lets talk about technical
aspects only.
Do you reload MySql with '/usr/local/etc/rc.d/mysql-server start'?
I guess - no. The same with other services because meaning of word "start"
is to born new instance of something.
"/etc/rc.d/ipfw start" isn't exclusion, it should follow human's logic.
If one want to reload firewall's rules he/she need to implement command "reload".

Eugene Grosbein <egrosbein at rdtc.ru> wrote:
> It does not cause lockdown if used in right way, I do this all the time.
Instead of repeating many times about magic "right way" could you point me
to documentation where is this "right way" explained?
Don't point me to ipfw(8) manual page again please because it describe 
"/sbin/ipfw" but isn't "/etc/rc.d/ipfw".
There is a big difference between them.

I didn't saw in the documentation a special 'right way' of using rc.d start up
scripts. May be I miss something, may be you.
You can execute anyone scripts without any parameters in rc.d directory and it 
will show all available arguments that can be passed to scripts.
I didn't found anyone rc scripts that says it need to be used in a special
"right way".
There no and shouldn't be any special tricks to run start up scripts.

Eugene Grosbein <egrosbein at rdtc.ru> wrote:
> Disabling firewall is not an option.
> There must be a way to reload rules without passing packets by meantime.
> This way now is "/etc/rc.d/ipfw start" command.
Absolutely agree with you on that - "Disabling firewall is not an option" 
but reloading firewall's rules by employ "/etc/rc.d/ipfw start" isn't
an option too. I guess all our disagreement is because there no command 
"reload" for ipfw service.

Eugene Grosbein <egrosbein at rdtc.ru> wrote:
>  >  Did you try all steps described in the "How-To-Repeat" section before replying?
> Yes. No problems here.
I bet you did it from system console or by enclosing "/etc/rc.d/ipfw start"
to surrounding guarding script, otherwise if you don't protect 
'/etc/rc.d/ipfw start' from interruption then first 
"ipfw -f flash" command will broke connection to the box and script
"/etc/rc.d/ipfw" will stop execution in a middle by leaving box inaccessible
over network.

Eugene Grosbein <egrosbein at rdtc.ru> wrote:
> Ian Smith <smithi at nimnet.asn.au> wrote:
> > Firstly, there's no problem with running /etc/rc.d/natd in any case, as 
> > it won't do anything (ie is a NOP) unless natd_enable is set in rc.conf.
> > 
> > Secondly, having firewall_nat_enable set has no need or use for loading 
> > natd, they're quite separate methods of performing NAT.
>
> Nice catch ;-) I've overlooked this.

I don't agree with you guys here. 
There no any reason to execute /etc/rc.d/natd if it will not be used.
Pointless operation. 
You create multiple dependency that is hard to track it down. 
Each script should be independent as much as possible and don't relay
on assumption that /etc/rc.d/natd wouldn't start never.
Why not to handle all logic in one place instead of creating 
possible holes. It just three additional lines that whouldn't bloat OS to the
hell but ensure clear logic of operations and exclude useless running
of /etc/rc.d/natd if it disabled. 
There already exist condition statements in the ipfw_prestart()
function that tried to figure out if nat will be used or not, so IMHO
better to handle it exactly in that place where test condition was done instead of
cheese it up across multiple scripts.