This is an automated email from the git hooks/post-receive script. guillem pushed a commit to branch master in repository dpkg.
View the commit online: https://git.dpkg.org/cgit/dpkg/dpkg.git/commit/?id=bc9736f6feae7625cc5ec063ea1b27d51a5f9317 commit bc9736f6feae7625cc5ec063ea1b27d51a5f9317 Author: Guillem Jover <guil...@debian.org> AuthorDate: Sat Sep 22 12:12:05 2018 +0200 s-s-d: Check whether standalone --pidfile use is secure If we are only matching on the pidfile, which is owned by a non-root user, and we are running as a root user then this is a security risk, and the contents cannot be trusted, because the daemon might have been compromised which would allow modifying the pid within. If we are then calling start-stop-daemon as a privileged user, that would allow acting on any PID in the system. Prompted-by: Michael Orlitzky <mich...@orlitzky.com> Ref: https://redmine.kannel.org/issues/771 --- debian/changelog | 2 ++ utils/start-stop-daemon.c | 40 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/debian/changelog b/debian/changelog index 1ffed7723..eb301b218 100644 --- a/debian/changelog +++ b/debian/changelog @@ -11,6 +11,8 @@ dpkg (1.19.3) UNRELEASED; urgency=medium Closes: #916456 * dpkg-scanpackages: Emit a warning with the list of repeat packages. Prompted by Johannes Schauer <jo...@debian.org>. + * start-stop-daemon: Check whether standalone --pidfile use is secure. + Prompted by Michael Orlitzky <mich...@orlitzky.com>. * Perl modules: - Dpkg::Changelog::Debian: Preserve modelines at EOF. Closes: #916056 Thanks to Chris Lamb <la...@debian.org> for initial test cases. diff --git a/utils/start-stop-daemon.c b/utils/start-stop-daemon.c index 13c453fa1..d4d30fbbb 100644 --- a/utils/start-stop-daemon.c +++ b/utils/start-stop-daemon.c @@ -183,6 +183,16 @@ enum action_code { ACTION_STATUS, }; +enum match_code { + MATCH_NONE = 0, + MATCH_PID = 1 << 0, + MATCH_PPID = 1 << 1, + MATCH_PIDFILE = 1 << 2, + MATCH_EXEC = 1 << 3, + MATCH_NAME = 1 << 4, + MATCH_USER = 1 << 5, +}; + /* Time conversion constants. */ enum { NANOSEC_IN_SEC = 1000000000L, @@ -194,6 +204,7 @@ enum { static const long MIN_POLL_INTERVAL = 20L * NANOSEC_IN_MILLISEC; static enum action_code action; +static enum match_code match_mode; static bool testmode = false; static int quietmode = 0; static int exitnodo = 1; @@ -1052,18 +1063,22 @@ parse_options(int argc, char * const *argv) startas = optarg; break; case 'n': /* --name <process-name> */ + match_mode |= MATCH_NAME; cmdname = optarg; break; case 'o': /* --oknodo */ exitnodo = 0; break; case OPT_PID: /* --pid <pid> */ + match_mode |= MATCH_PID; pid_str = optarg; break; case OPT_PPID: /* --ppid <ppid> */ + match_mode |= MATCH_PPID; ppid_str = optarg; break; case 'p': /* --pidfile <pid-file> */ + match_mode |= MATCH_PIDFILE; pidfile = optarg; break; case 'q': /* --quiet */ @@ -1076,12 +1091,14 @@ parse_options(int argc, char * const *argv) testmode = true; break; case 'u': /* --user <username>|<uid> */ + match_mode |= MATCH_USER; userspec = optarg; break; case 'v': /* --verbose */ quietmode = -1; break; case 'x': /* --exec <executable> */ + match_mode |= MATCH_EXEC; execname = optarg; break; case 'c': /* --chuid <username>|<uid> */ @@ -1171,8 +1188,9 @@ parse_options(int argc, char * const *argv) if (action == ACTION_NONE) badusage("need one of --start or --stop or --status"); - if (!execname && !pid_str && !ppid_str && !pidfile && !userspec && - !cmdname) + if (match_mode == MATCH_NONE || + (!execname && !cmdname && !userspec && + !pid_str && !ppid_str && !pidfile)) badusage("need at least one of --exec, --pid, --ppid, --pidfile, --user or --name"); #ifdef PROCESS_NAME_SIZE @@ -1994,6 +2012,24 @@ do_pidfile(const char *name) if (f) { enum status_code pid_status; + /* If we are only matching on the pidfile, and it is owned by + * a non-root user, then this is a security risk, and the + * contents cannot be trusted, because the daemon might have + * been compromised. */ + if (match_mode == MATCH_PIDFILE) { + struct stat st; + int fd = fileno(f); + + if (fstat(fd, &st) < 0) + fatal("cannot stat pidfile %s", name); + + if ((st.st_uid != getuid() && st.st_uid != 0) || + (st.st_gid != getgid() && st.st_gid != 0)) + fatal("matching only on non-root pidfile %s is insecure", name); + if (st.st_mode & 0002) + fatal("matching only on world-writable pidfile %s is insecure", name); + } + if (fscanf(f, "%d", &pid) == 1) pid_status = pid_check(pid); else -- Dpkg.Org's dpkg
