From owner-freebsd-rc@FreeBSD.ORG Sun Mar 5 03:00:26 2006 Return-Path: X-Original-To: freebsd-rc@hub.freebsd.org Delivered-To: freebsd-rc@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6A66B16A422 for ; Sun, 5 Mar 2006 03:00:26 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id C833D43D45 for ; Sun, 5 Mar 2006 03:00:25 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id k2530P48069973 for ; Sun, 5 Mar 2006 03:00:25 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id k2530PCU069972; Sun, 5 Mar 2006 03:00:25 GMT (envelope-from gnats) Date: Sun, 5 Mar 2006 03:00:25 GMT Message-Id: <200603050300.k2530PCU069972@freefall.freebsd.org> To: freebsd-rc@FreeBSD.org From: Giorgos Keramidas Cc: Subject: Re: conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw and rc.d/ip6fw. X-BeenThere: freebsd-rc@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Giorgos Keramidas List-Id: "Discussion related to /etc/rc.d design and implementation." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Mar 2006 03:00:26 -0000 The following reply was made to PR conf/93815; it has been noted by GNATS. From: Giorgos Keramidas To: Vulpes Velox Cc: bug-followup@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 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 > > They can be recalled like this... > /etc/rc.d/ipfw restart 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 :)