Re: syspatch -c as non-root
Vadim Zhukov wrote: > But anyway, root-only requirement for listing available syspatches > seems a bit silly. as Antoine has replied, this is so the file retrieval occurs as a privsep user, so that a bug in that tooling is very much more difficult to exploit. Undoing that privsep feels unhealthy. The simplest form of check doesn't even need this much work: If a newer SHA256.sig file has been published, maybe there's something new.
Re: syspatch -c as non-root
сб, 14 дек. 2019 г. в 14:35, Antoine Jacoutot :
>
> 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.
Just to make sure I understand you correctly: are the "unsafe
commands" you mentioned the ftp(1) calls? The default case ("apply new
syspatches") and -R/-r cases do require running as root still, so no
changes in behavior here: the root check happens before any unpriv()
call. The -l case, which is already allowed for non-root, accesses
local files in /var/syspatch, and this already looks like more fragile
(accessing local data), than calling pledged ftp(1).
Or you mean that unpriv() function itself should be run as root only,
to clearly indicate intent?
> > 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 - 1.159
> > +++ syspatch.sh 14 Dec 2019 06:50:20 -
> > @@ -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.XX)
> > _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
--
WBR,
Vadim Zhukov
Re: syspatch -c as non-root
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 - 1.159
> +++ syspatch.sh 14 Dec 2019 06:50:20 -
> @@ -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.XX)
> _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
syspatch -c as non-root
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.
--
WBR,
Vadim Zhukov
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 - 1.159
+++ syspatch.sh 14 Dec 2019 06:50:20 -
@@ -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.XX)
_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
