From e4d452b37b363307c629e8a260cb66f423354abb Mon Sep 17 00:00:00 2001
From: Franco Fichtner <franco@opnsense.org>
Date: Fri, 29 Nov 2024 14:52:52 +0100
Subject: [PATCH] firmware: make output_cmd safter regarding arguments passed

As a side effect it removes the spurious quoting around most
static arguemnts that do not need to be vetted for safety
anyway (but still are if a mistake is made).

Some arguments are passed unquoted for one of the two reasons:

1. It's a global variable pointing to a binary or directory most
   likely, especially for first argument which is the command.
2. It's an argument that is set in the script, but may be empty
   when the command runs, i.e. '-f' option.
---
 src/opnsense/scripts/firmware/check.sh      | 14 +++++++-------
 src/opnsense/scripts/firmware/config.sh     | 14 +++++++++++++-
 src/opnsense/scripts/firmware/connection.sh | 10 +++++-----
 src/opnsense/scripts/firmware/health.sh     |  6 +++---
 src/opnsense/scripts/firmware/install.sh    |  6 +++---
 src/opnsense/scripts/firmware/lock.sh       |  6 +++---
 src/opnsense/scripts/firmware/reinstall.sh  | 14 +++++++-------
 src/opnsense/scripts/firmware/remove.sh     |  6 +++---
 src/opnsense/scripts/firmware/resync.sh     |  2 +-
 src/opnsense/scripts/firmware/security.sh   |  2 +-
 src/opnsense/scripts/firmware/sync.subr.sh  |  6 +++---
 src/opnsense/scripts/firmware/unlock.sh     |  6 +++---
 src/opnsense/scripts/firmware/update.sh     |  6 +++---
 src/opnsense/scripts/firmware/upgrade.sh    |  8 ++++----
 14 files changed, 59 insertions(+), 47 deletions(-)

diff --git a/src/opnsense/scripts/firmware/check.sh b/src/opnsense/scripts/firmware/check.sh
index e3d95715d..63a106792 100755
--- a/src/opnsense/scripts/firmware/check.sh
+++ b/src/opnsense/scripts/firmware/check.sh
@@ -92,7 +92,7 @@ fi
 # business subscriptions come with additional license metadata
 if [ -n "$(opnsense-update -x)" ]; then
     output_text -n "Fetching subscription information, please wait... "
-    if output_cmd "fetch -qT 30 -o '${LICENSEFILE}' '$(opnsense-update -M)/subscription'"; then
+    if output_cmd fetch -qT 30 -o "${LICENSEFILE}" "$(opnsense-update -M)/subscription"; then
         output_text "done"
     fi
 else
@@ -100,15 +100,15 @@ else
 fi
 
 output_text -n "Fetching changelog information, please wait... "
-if output_cmd "${BASEDIR}/changelog.sh fetch"; then
+if output_cmd ${BASEDIR}/changelog.sh fetch; then
     output_text "done"
 fi
 
 : > ${OUTFILE}
-output_cmd -o ${OUTFILE} "${PKG} update -f"
+output_cmd -o ${OUTFILE} ${PKG} update -f
 
 # always update the package manager so we can see the real updates directly
-output_cmd "${PKG} upgrade -r '${product_repo}' -Uy 'pkg'"
+output_cmd ${PKG} upgrade -r "${product_repo}" -Uy pkg
 
 # parse early errors
 if grep -q 'No address record' ${OUTFILE}; then
@@ -146,11 +146,11 @@ else
     : > ${OUTFILE}
 
     # now check what happens when we would go ahead
-    output_cmd -o ${OUTFILE} "${PKG} upgrade ${force_all} -Un"
+    output_cmd -o ${OUTFILE} ${PKG} upgrade ${force_all} -Un
     if  [ -n "${CUSTOMPKG}" ]; then
-        output_cmd -o ${OUTFILE} "${PKG} install -Un '${CUSTOMPKG}'"
+        output_cmd -o ${OUTFILE} ${PKG} install -Un "${CUSTOMPKG}"
     elif [ "${product_id}" != "${product_target}" ]; then
-        output_cmd -o ${OUTFILE} "${PKG} install -r '${product_repo}' -Un '${product_target}'"
+        output_cmd -o ${OUTFILE} ${PKG} install -r "${product_repo}" -Un "${product_target}"
     elif [ -z "$(${PKG} rquery %n ${product_id})" ]; then
         # although this should say "to update matching" we emulate for
         # check below as the package manager does not catch this
diff --git a/src/opnsense/scripts/firmware/config.sh b/src/opnsense/scripts/firmware/config.sh
index 7d14063fd..f8ea07e33 100755
--- a/src/opnsense/scripts/firmware/config.sh
+++ b/src/opnsense/scripts/firmware/config.sh
@@ -95,6 +95,7 @@ output_text()
 
 output_cmd()
 {
+	DO_CMD=
 	DO_OUT=
 
 	while getopts o: OPT; do
@@ -110,11 +111,22 @@ output_cmd()
 
 	shift $((OPTIND - 1))
 
+	for ARG in "${@}"; do
+		# single quote will not execute for safety
+		if [ -z "${ARG##*"'"*}" ]; then
+			output_text "firmware: safety violation in argument during ${REQUEST}"
+			return 1
+		fi
+
+		# append safely to argument in single quotes
+		DO_CMD="${DO_CMD} '$(echo ${ARG})'"
+	done
+
 	# pipe needed for grabbing the command return value
 	${TEE} ${LOCKFILE} ${DO_OUT} < ${PIPEFILE} &
 
 	# also capture stderr in this case
-	eval "(${1}) 2>&1" > ${PIPEFILE}
+	eval "(${DO_CMD}) 2>&1" > ${PIPEFILE}
 }
 
 output_done()
diff --git a/src/opnsense/scripts/firmware/connection.sh b/src/opnsense/scripts/firmware/connection.sh
index 0913af9fd..bd73322b7 100755
--- a/src/opnsense/scripts/firmware/connection.sh
+++ b/src/opnsense/scripts/firmware/connection.sh
@@ -44,18 +44,18 @@ mkdir -p ${PKG_DBDIR}
 
 if [ -n "${IPV4}" -a -z "${IPV4%%*.*}" ]; then
 	output_text "Checking connectivity for host: ${HOST} -> ${IPV4}"
-	output_cmd "ping -4 ${POPT} ${IPV4}"
+	output_cmd ping -4 ${POPT} "${IPV4}"
 	output_text "Checking connectivity for repository (IPv4): ${URL}"
-	output_cmd "${PKG} -4 update -f"
+	output_cmd ${PKG} -4 update -f
 else
 	output_text "No IPv4 address could be found for host: ${HOST}"
 fi
 
 if [ -n "${IPV6}" -a -z "${IPV6%%*:*}" ]; then
 	output_text "Checking connectivity for host: ${HOST} -> ${IPV6}"
-	output_cmd "ping -6 ${POPT} ${IPV6}"
+	output_cmd ping -6 ${POPT} "${IPV6}"
 	output_text "Checking connectivity for repository (IPv6): ${URL}"
-	output_cmd "${PKG} -6 update -f"
+	output_cmd ${PKG} -6 update -f
 else
 	output_text "No IPv6 address could be found for host: ${HOST}"
 fi
@@ -63,7 +63,7 @@ fi
 for HOST in $(/usr/local/opnsense/scripts/firmware/hostnames.sh); do
 	output_text "Checking server certificate for host: ${HOST}"
 	# XXX -crl_check and -crl_check_all are possible but -CRL pass is not working
-	output_cmd "echo | openssl s_client -quiet -no_ign_eof ${HOST}:443"
+	echo | output_cmd openssl s_client -quiet -no_ign_eof "${HOST}:443"
 done
 
 output_done
diff --git a/src/opnsense/scripts/firmware/health.sh b/src/opnsense/scripts/firmware/health.sh
index 2e3c8d1e9..a8a552177 100755
--- a/src/opnsense/scripts/firmware/health.sh
+++ b/src/opnsense/scripts/firmware/health.sh
@@ -232,7 +232,7 @@ fi
 
 if [ -z "${CMD}" -o "${CMD}" = "repos" ]; then
 	output_text ">>> Check installed repositories"
-	output_cmd "opnsense-verify -l"
+	output_cmd opnsense-verify -l
 fi
 
 if [ -z "${CMD}" -o "${CMD}" = "plugins" ]; then
@@ -257,10 +257,10 @@ fi
 
 if [ -z "${CMD}" -o "${CMD}" = "packages" ]; then
 	output_text ">>> Check for missing package dependencies"
-	output_cmd "${PKG} check -dan"
+	output_cmd ${PKG} check -dan
 
 	output_text ">>> Check for missing or altered package files"
-	output_cmd "${PKG} check -sa"
+	output_cmd ${PKG} check -sa
 fi
 
 if [ -z "${CMD}" -o "${CMD}" = "core" ]; then
diff --git a/src/opnsense/scripts/firmware/install.sh b/src/opnsense/scripts/firmware/install.sh
index 6395578aa..5dde148d8 100755
--- a/src/opnsense/scripts/firmware/install.sh
+++ b/src/opnsense/scripts/firmware/install.sh
@@ -43,8 +43,8 @@ if [ "${PACKAGE#os-}" != "${PACKAGE}" ]; then
 	fi
 fi
 
-output_cmd "${PKG} install -y ${PACKAGE}"
-output_cmd "${BASEDIR}/register.php install ${PACKAGE}"
-output_cmd "${PKG} autoremove -y"
+output_cmd ${PKG} install -y "${PACKAGE}"
+output_cmd ${BASEDIR}/register.php install "${PACKAGE}"
+output_cmd ${PKG} autoremove -y
 
 output_done
diff --git a/src/opnsense/scripts/firmware/lock.sh b/src/opnsense/scripts/firmware/lock.sh
index ee783269c..2d1d7a9fa 100755
--- a/src/opnsense/scripts/firmware/lock.sh
+++ b/src/opnsense/scripts/firmware/lock.sh
@@ -33,12 +33,12 @@ PACKAGE=${1}
 
 if [ "${PACKAGE}" = "base" ]; then
 	output_text "Locking base set"
-	output_cmd "opnsense-update -bL"
+	output_cmd opnsense-update -bL
 elif [ "${PACKAGE}" = "kernel" ]; then
 	output_text "Locking kernel set"
-	output_cmd "opnsense-update -kL"
+	output_cmd opnsense-update -kL
 else
-	output_cmd "${PKG} lock -y ${PACKAGE}"
+	output_cmd ${PKG} lock -y "${PACKAGE}"
 fi
 
 output_done
diff --git a/src/opnsense/scripts/firmware/reinstall.sh b/src/opnsense/scripts/firmware/reinstall.sh
index 757596d00..f16bb0886 100755
--- a/src/opnsense/scripts/firmware/reinstall.sh
+++ b/src/opnsense/scripts/firmware/reinstall.sh
@@ -34,27 +34,27 @@ PACKAGE=${1}
 if [ "${PACKAGE}" = "base" ]; then
 	if opnsense-update -Tb; then
 		# force reinstall intended
-		if output_cmd "opnsense-update -bf"; then
+		if output_cmd opnsense-update -bf; then
 			output_reboot
 		fi
 	else
 		# for locked message only
-		output_cmd "opnsense-update -b"
+		output_cmd opnsense-update -b
 	fi
 elif [ "${PACKAGE}" = "kernel" ]; then
 	if opnsense-update -Tk; then
 		# force reinstall intended
-		if output_cmd "opnsense-update -kf"; then
+		if output_cmd opnsense-update -kf; then
 			output_reboot
 		fi
 	else
 		# for locked message only
-		output_cmd "opnsense-update -k"
+		output_cmd opnsense-update -k
 	fi
 else
-	output_cmd "opnsense-revert -l ${PACKAGE}"
-	output_cmd "${BASEDIR}/register.php install ${PACKAGE}"
-	output_cmd "${PKG} autoremove -y"
+	output_cmd opnsense-revert -l "${PACKAGE}"
+	output_cmd ${BASEDIR}/register.php install "${PACKAGE}"
+	output_cmd ${PKG} autoremove -y
 fi
 
 output_done
diff --git a/src/opnsense/scripts/firmware/remove.sh b/src/opnsense/scripts/firmware/remove.sh
index 015d921c2..60465f91c 100755
--- a/src/opnsense/scripts/firmware/remove.sh
+++ b/src/opnsense/scripts/firmware/remove.sh
@@ -31,8 +31,8 @@ REQUEST="REMOVE"
 
 PACKAGE=${1}
 
-output_cmd "${PKG} remove -y ${PACKAGE}"
-output_cmd "${BASEDIR}/register.php remove ${PACKAGE}"
-output_cmd "${PKG} autoremove -y"
+output_cmd ${PKG} remove -y "${PACKAGE}"
+output_cmd ${BASEDIR}/register.php remove "${PACKAGE}"
+output_cmd ${PKG} autoremove -y
 
 output_done
diff --git a/src/opnsense/scripts/firmware/resync.sh b/src/opnsense/scripts/firmware/resync.sh
index a79a4775a..bcf951753 100755
--- a/src/opnsense/scripts/firmware/resync.sh
+++ b/src/opnsense/scripts/firmware/resync.sh
@@ -28,6 +28,6 @@ REQUEST="RESYNC"
 
 . /usr/local/opnsense/scripts/firmware/config.sh
 
-output_cmd "${BASEDIR}/register.php resync"
+output_cmd ${BASEDIR}/register.php resync
 
 output_done
diff --git a/src/opnsense/scripts/firmware/security.sh b/src/opnsense/scripts/firmware/security.sh
index ab5cdb92c..887cd06fb 100755
--- a/src/opnsense/scripts/firmware/security.sh
+++ b/src/opnsense/scripts/firmware/security.sh
@@ -28,6 +28,6 @@ REQUEST="AUDIT SECURITY"
 
 . /usr/local/opnsense/scripts/firmware/config.sh
 
-output_cmd "${PKG} audit -F"
+output_cmd ${PKG} audit -F
 
 output_done
diff --git a/src/opnsense/scripts/firmware/sync.subr.sh b/src/opnsense/scripts/firmware/sync.subr.sh
index 4555e5f14..7debc6917 100755
--- a/src/opnsense/scripts/firmware/sync.subr.sh
+++ b/src/opnsense/scripts/firmware/sync.subr.sh
@@ -45,11 +45,11 @@ for PACKAGE in $(/usr/local/sbin/pluginctl -g system.firmware.plugins | \
 			MUSTCHECK=
 		fi
 
-		output_cmd "${PKG} install -y ${PACKAGE}"
-		output_cmd "${BASEDIR}/register.php install ${PACKAGE}"
+		output_cmd ${PKG} install -y "${PACKAGE}"
+		output_cmd ${BASEDIR}/register.php install "${PACKAGE}"
 	fi
 done
 
 if [ -z "${MUSTCHECK}" ]; then
-	output_cmd "${PKG} autoremove -y"
+	output_cmd ${PKG} autoremove -y
 fi
diff --git a/src/opnsense/scripts/firmware/unlock.sh b/src/opnsense/scripts/firmware/unlock.sh
index 48297e744..026e2c11b 100755
--- a/src/opnsense/scripts/firmware/unlock.sh
+++ b/src/opnsense/scripts/firmware/unlock.sh
@@ -33,12 +33,12 @@ PACKAGE=${1}
 
 if [ "${PACKAGE}" = "base" ]; then
 	output_text "Unlocking base set"
-	output_cmd "opnsense-update -bU"
+	output_cmd opnsense-update -bU
 elif [ "${PACKAGE}" = "kernel" ]; then
 	output_text "Unlocking kernel set"
-	output_cmd "opnsense-update -kU"
+	output_cmd opnsense-update -kU
 else
-	output_cmd "${PKG} unlock -y ${PACKAGE}"
+	output_cmd ${PKG} unlock -y "${PACKAGE}"
 fi
 
 output_done
diff --git a/src/opnsense/scripts/firmware/update.sh b/src/opnsense/scripts/firmware/update.sh
index 958be1725..d5ff04e04 100755
--- a/src/opnsense/scripts/firmware/update.sh
+++ b/src/opnsense/scripts/firmware/update.sh
@@ -48,10 +48,10 @@ ALWAYS_REBOOT=$(/usr/local/sbin/pluginctl -g system.firmware.reboot)
 PKGS_HASH=$(${PKG} query %n-%v 2> /dev/null | sha256)
 
 # upgrade all packages if possible
-output_cmd "opnsense-update ${FORCE} -pt 'opnsense${SUFFIX}'"
+output_cmd opnsense-update ${FORCE} -pt "opnsense${SUFFIX}"
 
 # restart the web server
-output_cmd "/usr/local/etc/rc.restart_webgui"
+output_cmd /usr/local/etc/rc.restart_webgui
 
 # run plugin resolver if requested
 if [ "${CMD}" = "sync" ]; then
@@ -60,7 +60,7 @@ fi
 
 # if we can update base, we'll do that as well
 if opnsense-update ${FORCE} -bk -c; then
-	if output_cmd "opnsense-update ${FORCE} -bk"; then
+	if output_cmd opnsense-update ${FORCE} -bk; then
 		output_reboot
 	fi
 fi
diff --git a/src/opnsense/scripts/firmware/upgrade.sh b/src/opnsense/scripts/firmware/upgrade.sh
index 3d3125086..d4b87eaef 100755
--- a/src/opnsense/scripts/firmware/upgrade.sh
+++ b/src/opnsense/scripts/firmware/upgrade.sh
@@ -29,15 +29,15 @@ REQUEST="UPGRADE"
 
 . /usr/local/opnsense/scripts/firmware/config.sh
 
-if output_cmd "opnsense-update -u"; then
-	if output_cmd "/usr/local/etc/rc.syshook upgrade"; then
-		if output_cmd "opnsense-update -K"; then
+if output_cmd opnsense-update -u; then
+	if output_cmd /usr/local/etc/rc.syshook upgrade; then
+		if output_cmd opnsense-update -K; then
 			output_reboot
 		fi
 	fi
 
 	# abort pending upgrades
-	output_cmd "opnsense-update -es"
+	output_cmd opnsense-update -es
 fi
 
 output_done