From owner-freebsd-ipfw@FreeBSD.ORG Wed Dec 22 15:10:12 2010 Return-Path: Delivered-To: freebsd-ipfw@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 84B8F106564A for ; Wed, 22 Dec 2010 15:10:12 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 4D9E78FC18 for ; Wed, 22 Dec 2010 15:10:12 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id oBMFACQ1043001 for ; Wed, 22 Dec 2010 15:10:12 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id oBMFACs1042998; Wed, 22 Dec 2010 15:10:12 GMT (envelope-from gnats) Date: Wed, 22 Dec 2010 15:10:12 GMT Message-Id: <201012221510.oBMFACs1042998@freefall.freebsd.org> To: freebsd-ipfw@FreeBSD.org From: "Alexander Verbod" Cc: Subject: Re: bin/153252: [ipfw][patch] ipfw lockdown system in subsequent call of "/etc/rc.d/ipfw start" X-BeenThere: freebsd-ipfw@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Alexander Verbod List-Id: IPFW Technical Discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Dec 2010 15:10:12 -0000 The following reply was made to PR bin/153252; it has been noted by GNATS. From: "Alexander Verbod" To: bug-followup@FreeBSD.org Cc: freebsd-ipfw@freebsd.org,"Ian Smith" ,"Eugene Grosbein" Subject: Re: bin/153252: [ipfw][patch] ipfw lockdown system in subsequent call of "/etc/rc.d/ipfw start" Date: Wed, 22 Dec 2010 10:03:26 -0500 Ian Smith 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 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 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 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 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 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 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 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 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 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 wrote: > Ian Smith 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.