The init script for collectd gives ownership of the PID file directory to the same user that the daemon runs as: checkpath --directory --mode 0770 --owner ${COLLECTD_USER}:${COLLECTD_GROUP} "$(dirname "${COLLECTD_PIDFILE}")" As a result, the $COLLECTD_USER can write whatever he wants into the PID file. Later, that may be exploitable: when the service is stopped, root will call "kill" on the contents of that file. The init script is doing that because start-stop-daemon starts collectd as the ${COLLECTD_USER} user. Thus the PID file created by the daemon is created by the ${COLLECTD_USER} user, who needs permission to write it. The usual way that this problem is avoided is that the daemon itself drops privileges, rather than start-stop-daemon doing the work. However, collectd doesn't have any such user/group options. Fortunately, collectd *does* have the ability to run in the foreground. As a workaround, you can pass the "-f" flag to collectd to run it in the foreground. Then command_background=true can be used to tell start-stop-daemon that it should both force the process into the background, and manage the PID file itself. That will require no special privileges on the PID directory, thus avoiding the vulnerability. To summarize the changes that I would make: 1. add "-f" to command_args, and drop -P "${COLLECTD_PIDFILE}" from it. 2. set command_background=true 3. use /run/collectd.pid (writable only by root) as the COLLECTD_PIDFILE. 4. eliminate the (now redundant) stop_post function 5. utilize command_user to set the user/group, rather than start_stop_daemon_args
(In reply to Michael Orlitzky from comment #0) > The init script for collectd gives ownership of the PID file directory to > the same user that the daemon runs as: > > checkpath --directory > --mode 0770 > --owner ${COLLECTD_USER}:${COLLECTD_GROUP} > "$(dirname "${COLLECTD_PIDFILE}")" > > As a result, the $COLLECTD_USER can write whatever he wants into the PID > file. Later, that may be exploitable: when the service is stopped, root will > call "kill" on the contents of that file. I maybe miss a thing but collectd uses OpenRC's default stop function: > ssd_stop() > { > local _progress= > local startcommand="$(service_get_value "command")" > local startchroot="$(service_get_value "chroot")" > local startpidfile="$(service_get_value "pidfile")" > local startprocname="$(service_get_value "procname")" > command="${startcommand:-$command}" > chroot="${startchroot:-$chroot}" > pidfile="${startpidfile:-$pidfile}" > procname="${startprocname:-$procname}" > [ -n "$command" -o -n "$procname" -o -n "$pidfile" ] || return 0 > yesno "${command_progress}" && _progress=--progress > ebegin "Stopping ${name:-$RC_SVCNAME}" > start-stop-daemon --stop \ > ${retry:+--retry} $retry \ > ${command:+--exec} $command \ > ${procname:+--name} $procname \ > ${pidfile:+--pidfile} $chroot$pidfile \ > ${stopsig:+--signal} $stopsig \ > ${_progress} > > eend $? "Failed to stop ${name:-$RC_SVCNAME}" > } (https://github.com/OpenRC/openrc/blob/master/sh/start-stop-daemon.sh) And the runscript defines "command": > command="/usr/sbin/collectd" (https://gitweb.gentoo.org/repo/gentoo.git/tree/app-admin/collectd/files/collectd.initd-r1#n14) ...so I don't understand how a user able to control the content of "/run/collectd/collectd.pid" (and you are right, per default, the collectd user is able to control this file) could abuse this file to escalate privileges: Start-stop-daemon should detect that the PID doesn't match the command.
(In reply to Thomas Deutschmann from comment #1) > Start-stop-daemon should detect that the PID doesn't match the > command. A reasonable assumption that turns out to be wrong if you try it =)
It would be nice if we could fix issues like these wholesale, but it's not so clear-cut; e.g. https://github.com/OpenRC/openrc/pull/156 In the meantime, it's still a good idea to update the init scripts because people copy around stop() and reload() functions without any regard for what they're doing. Even if OpenRC was able to check the command name against the PID, there are plenty of copy/pasted stop() functions that don't pass the command name to s-s-d. Having bulletproof examples in the tree provides another layer of defense against the issue.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=9a70b58bd58ff19395c55abbf0a2e620a5a56f3a commit 9a70b58bd58ff19395c55abbf0a2e620a5a56f3a Author: Thomas Deutschmann <whissi@gentoo.org> AuthorDate: 2018-01-25 22:34:18 +0000 Commit: Thomas Deutschmann <whissi@gentoo.org> CommitDate: 2018-01-25 22:34:34 +0000 app-admin/collectd: bump, fixes CVE-2017-16820 & #628540 Ebuild changes: =============== - To address bug 628540, we no longer run collectd in daemon mode, instead we will run collectd everywhere in foreground and let the init system handle the PID file. - /run/collectd/ (default location for collectd's UNIX socket) is now maintained using tmpfiles service. Bug: https://bugs.gentoo.org/628540 Bug: https://bugs.gentoo.org/637538 Package-Manager: Portage-2.3.20, Repoman-2.3.6 app-admin/collectd/collectd-5.7.2-r1.ebuild | 541 +++++++++++++++++++++ .../files/collectd-5.7.2-CVE-2017-16820.patch | 39 ++ app-admin/collectd/files/collectd.confd-r2 | 49 ++ app-admin/collectd/files/collectd.initd-r2 | 70 +++ app-admin/collectd/files/collectd.tmpfile | 1 + 5 files changed, 700 insertions(+)}
@ Michael: Could you please review my changes if they address all the problems?
Looks good, thanks!
Thanks for the review and the report :) @ Arches, please test and mark stable: =app-admin/collectd-5.7.2-r1
x86 stable
amd64 stable
arm stable, all arches done.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=c148c069385caf68d0bc5609ce13db4ba9e71415 commit c148c069385caf68d0bc5609ce13db4ba9e71415 Author: Thomas Deutschmann <whissi@gentoo.org> AuthorDate: 2018-03-03 16:41:59 +0000 Commit: Thomas Deutschmann <whissi@gentoo.org> CommitDate: 2018-03-03 16:43:51 +0000 app-admin/collectd: Security cleanup Bug: https://bugs.gentoo.org/628540 Package-Manager: Portage-2.3.24, Repoman-2.3.6 app-admin/collectd/Manifest | 2 - app-admin/collectd/collectd-5.6.2-r4.ebuild | 532 -------------------- app-admin/collectd/collectd-5.7.1.ebuild | 537 --------------------- app-admin/collectd/collectd-5.7.2.ebuild | 536 -------------------- .../files/collectd-5.6.2-CVE-2017-7401.patch | 56 --- .../collectd/files/collectd-5.6.2-issue2303.patch | 44 -- app-admin/collectd/files/collectd.confd-r1 | 45 -- app-admin/collectd/files/collectd.initd-r1 | 78 --- 8 files changed, 1830 deletions(-)}
New GLSA request filed.
CVE-2017-18240 Assigned.
This issue was resolved and addressed in GLSA 201803-10 at https://security.gentoo.org/glsa/201803-10 by GLSA coordinator Christopher Diaz Riveros (chrisadr).
