conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw and rc.d/ip6fw.

[Giorgos Keramidas]

The following reply was made to PR conf/93815; it has been noted by GNATS.

From: Giorgos Keramidas <keramida at FreeBSD.org>
To: Vulpes Velox <v.velox at vvelox.net>
Cc: bug-followup at FreeBSD.org
Subject: Re: conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw and rc.d/ip6fw.
Date: Sun, 5 Mar 2006 04:54:55 +0200

 On 2006-02-25 04:19, Vulpes Velox <v.velox at vvelox.net> wrote:
 > This allows ipfw rules to be saved. /var/db/ipfw is used for that. If
 > a name for the save is not specified, last will be used.
 >
 > They can be saved like this...
 > /etc/rc.d/ipfw save <name>
 >
 > They can be recalled like this...
 > /etc/rc.d/ipfw restart <name>
 
 I feel a bit worried about allowing unquoted user-supplied names to a
 shell script and then using them as filenames.
 
 > --- rc.d_ipfw.patch begins here ---
 > 18a19,29
 > > extra_commands="save"
 > > save_cmd="ipfw_save"
 > >
 > >
 > > #gets the name of the save to use
 > > if [ ! -z $2 ]; then
 > > 	savename="$2"
 > > 	usingsave="yes"
 > > else
 > > 	savename="last"
 > > fi
 
 Please don't.  This should be written at least with a proper quote set
 around $2 like this:
 
     if [ -z "$2" ]; then
 	savename="last"
     else
 	savename="$2"
         usingsave="yes"
     fi
 
 > 31a43,49
 > > ipfw_save()
 > > {
 > >         # Saves the firewall rules to /var/db/ipfw/$savename
 > > 	[ ! -d /var/db/ipfw ] && mkdir /var/db/ipfw && chmod go-rwx /var/db/ipfw
 > > 	ipfw list | awk '{print "${fwcmd} add " $0 }' > /var/db/ipfw/$savename
 > > }
 
 The style sucks a bit here, but it's mostly ok.  I'd probably avoid
 constructs that have the potential to end up using really-very-long
 lines, like cmd && cmd && cmd a bit and make the directory of the saved
 firewalls tunable through rc.conf:
 
     ipfw_save()
     {
         # set the firewall save directory if none was specified
         [ -z "${firewall_savedir}" ] && firewall_savedir=/var/db/ipfw
 
 	if [ ! -d "${firewall_savedir}" ]; then
 	    mkdir -p "${firewall_savedir}" || return 1
 	fi
 
         ipfw list | sed -e 's/^/add /' > "${firewall_savedir}/${savename}"
     }
 
 Also, in my opinion, loading saved rulesets shouldn't be overloaded with
 the special 'last' savename, but supported by a similar ipfw_load()
 function.  Then 'last' could be used as a valid savename too :)