On Sat, Dec 14, 2019 at 10:12:36AM +0300, Vadim Zhukov wrote:
> Hello all (long time no see!)
>
> TL;DR: Allow syspatch -c run under non-priviledged user.
>
> Reasoning: instead of putting syspatch -c in crontab, I've implemented
> a Zabbix trigger. Since the Zabbix agent runs as unpriviledged user,
> I had to add _zabbix line to doas.conf, allowing it to run syspatch -c.
> This way it works, but in the end I want this check either in stock
> Zabbix code, or in our packages. And requirement to modify doas.conf
> ruins all the fun.
>
> There is another way: writing some SUID script as simple as:
>
> #!/bin/sh
> case $1 in
> missing-syspatches) syspatch -c;;
> failed-services) rcctl ls failed;;
> *) echo "usage: ..." >&2; exit 1;;
> esac
>
> This way we could handle all possible future root-is-required cases
> at once. But adding SUID script... is it a Good Thing(TM)?
>
> But anyway, root-only requirement for listing available syspatches
> seems a bit silly.
>
> The patch was tested on 6.6. Well, it doesn't apply cleanly to 6.6,
> since "set +e" magic you see arrived in -CURRENT, but tweaked version
> runs smoothly.
>
> BTW, why do we ever call eval in unpriv()? Without eval, set +e isn't
> needed, FWIW.
I don't really like this.
It defeats the purpose of unpriv which guarantees you that a specific
unprivileged user will be used to run unsafe commands.
> Index: syspatch.sh
> ===================================================================
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
> retrieving revision 1.159
> diff -u -p -r1.159 syspatch.sh
> --- syspatch.sh 10 Dec 2019 17:11:06 -0000 1.159
> +++ syspatch.sh 14 Dec 2019 06:50:20 -0000
> @@ -179,7 +179,7 @@ ls_missing()
> ${_MIRROR}/syspatch${_OSrev}-${_p}.tgz"
> { unpriv ${_cmd} | tar tzf -; } 2>/dev/null | while read _f; do
> [[ -f /${_f} ]] || continue && echo ${_p} && pkill -u \
> - _syspatch -xf "${_cmd}" || true && break
> + $_USER -xf "${_cmd}" || true && break
> done
> done | sort -V
> }
> @@ -245,22 +245,26 @@ must be run manually to install the new
>
> unpriv()
> {
> - local _file=$2 _rc=0 _user=_syspatch
> + local _file=$2 _rc=0
>
> if [[ $1 == -f && -n ${_file} ]]; then
> >${_file}
> - chown "${_user}" "${_file}"
> + chown "${_USER}" "${_file}"
> chmod 0711 ${_TMP}
> shift 2
> fi
> (($# >= 1))
>
> - # XXX ksh(1) bug; send error code to the caller instead of failing hard
> - set +e
> - eval su -s /bin/sh ${_user} -c "'$@'" || _rc=$?
> - set -e
> -
> - [[ -n ${_file} ]] && chown root "${_file}"
> + if (($(id -u) == 0)); then
> + # XXX ksh(1) bug; send error code to the caller instead of
> failing hard
> + set +e
> + eval su -s /bin/sh ${_USER} -c "'$@'" || _rc=$?
> + set -e
> +
> + [[ -n ${_file} ]] && chown root "${_file}"
> + else
> + "$@" || _rc=$?
> + fi
>
> return ${_rc}
> }
> @@ -270,7 +274,7 @@ set -A _KERNV -- $(sysctl -n kern.versio
> sed 's/^OpenBSD \([1-9][0-9]*\.[0-9]\)\([^ ]*\).*/\1 \2/;q')
> ((${#_KERNV[*]} > 1)) && sp_err "Unsupported release:
> ${_KERNV[0]}${_KERNV[1]}"
>
> -[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(c|R|r)) ]] &&
> +[[ $@ == @(|-[[:alpha:]]) ]] || usage; [[ $@ == @(|-(R|r)) ]] &&
> (($(id -u) != 0)) && sp_err "need root privileges"
> [[ $@ == @(|-(R|r)) ]] && pgrep -qxf '/bin/ksh .*reorder_kernel' &&
> sp_err "cannot apply patches while reorder_kernel is running"
> @@ -290,7 +294,13 @@ _PDIR="/var/syspatch"
> _TMP=$(mktemp -d -p ${TMPDIR:-/tmp} syspatch.XXXXXXXXXX)
> _KARL=false
>
> -readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP
> +if (($(id -u) != 0)); then
> + _USER=$(id -nu)
> +else
> + _USER=_syspatch
> +fi
> +
> +readonly _BSDMP _KERNV _MIRROR _OSrev _PDIR _TMP _USER
>
> trap 'trap_handler' EXIT
> trap exit HUP INT TERM
>
--
Antoine
