From: Lucas Gabriel Vuotto <lucas@sexy.is>
Subject: sh unpriv: remove eval
To: tech@openbsd.org
Date: Sat, 30 Nov 2024 11:16:49 +0000

A post in ports@ from kn made me look at this again. I'd like to remove
eval from sh's unprivs in our tree. I think it makes it quite difficult
to reason about, while there are better and more clear constructions.

I tested it in sysupgrade. This is how ps looks while it runs:

_syspatc 12124  2.3  0.0  1240  4132 p2  S+p    10:47AM    0:00.55 ftp -N sysupgrade -Vmo base76.tgz http://cdn.openbsd.org/pub/OpenBSD/snapshots/amd64/base76.tgz
root     68356  0.0  0.0  1360  1040 p2  S+p    10:47AM    0:00.03 /bin/ksh /usr/sbin/sysupgrade -ns
_syspatc 57878  0.0  0.0  1352   968 p2  S+p    10:47AM    0:00.01 sh -c "$@" ftp ftp -N sysupgrade -Vmo base76.tgz http://cdn.openbsd.org/pub/OpenBSD/snapshots/amd64/base76.tgz

_argv0 was introduced only for clarity. Alternatively, it's a single
line diff that looks like

-	eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
+	su -s /bin/sh ${_user} -c '"$@"' "$1" "$@" || _rc=$?

Comments? OKs?

	Lucas


diff refs/heads/master e9307bee0d2860276e938474c70e59c6737c9c22
commit - db835d32417ebae1708add715bd52627db5c420c
commit + e9307bee0d2860276e938474c70e59c6737c9c22
blob - b825d96b39398a24777a3fb9e211451645e732b0
blob + 50a155786a30ff0cac065a1805b6b06a50b2e511
--- usr.sbin/syspatch/syspatch.sh
+++ usr.sbin/syspatch/syspatch.sh
@@ -254,7 +254,7 @@ must be run manually to install the new kernel"
 
 unpriv()
 {
-	local _file=$2 _rc=0 _user=_syspatch
+	local _argv0 _file=$2 _rc=0 _user=_syspatch
 
 	if [[ $1 == -f && -n ${_file} ]]; then
 		>${_file}
@@ -263,8 +263,9 @@ unpriv()
 		shift 2
 	fi
 	(($# >= 1))
+	_argv0=$1
 
-	eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
+	su -s /bin/sh ${_user} -c '"$@"' "$_argv0" "$@" || _rc=$?
 
 	[[ -n ${_file} ]] && chown root "${_file}"
 
blob - b01ccd3878d13305b42871dda308cecb708378a5
blob + 3dccd36872673f3a7ca9e230bf138e5201ea4800
--- usr.sbin/sysupgrade/sysupgrade.sh
+++ usr.sbin/sysupgrade/sysupgrade.sh
@@ -41,7 +41,7 @@ usage()
 
 unpriv()
 {
-	local _file _rc=0 _user=_syspatch
+	local _argv0 _file _rc=0 _user=_syspatch
 
 	if [[ $1 == -f ]]; then
 		_file=$2
@@ -52,8 +52,9 @@ unpriv()
 		chown "${_user}" "${_file}"
 	fi
 	(($# >= 1))
+	_argv0=$1
 
-	eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
+	su -s /bin/sh "${_user}" -c '"$@"' "$_argv0" "$@" || _rc=$?
 
 	[[ -n ${_file} ]] && chown root "${_file}"