start-stop-daemon pidfile permissions check
Hello to everyone,
commit bc9736f6 fixed a security hole in start-stop-daemon, but it is
not always backward-compatible and it affected at least 9 packages: see
bugs 920466 921016 921326 922395 923421 924312 924311 924640 927058
considering the sysvinit user-base is 1,5% of all popcon users and
codesearch shows 763 packages calling "start-stop-daemon --stop", there
is the probability other packages are broken
instead of checking if the process referenced by non-root pidfile is
matching a particular user and/or executable, I think the vulnerability
could be fixed simply checking if the owner of the pidfile matches the
user running the process to be killed
in fact, if start-stop-daemon is called as root and pidfile is owned by
non-root user, there is no security risk killing a process owned by the
same user that owns pidfile, because user can kill the process itself
it is more secure, as with the actual code, using only --user it allows
to kill any process of specified user and using only --exec it allows
to kill processes of any user running the specified executable
it should be more backward-compatible, as it does not require adding
--user or --exec to fix the init.d scripts, but on the other hand it
needs to fail if the pidfile is group-writable (hoping it is uncommon)
the code is split in two patches to simplify the review:
- the first one moves the pidfile permission checking in the new
pidfile_check function, without any functional change
- the second patch introduce the ability to check if the pidfile and
process owners do match
please tell me if it needs to be refined
in the sake of compatibility, the check is done only if --pidfile is
used alone, but I think that after buster release the condition
match_mode == MATCH_PIDFILE in pidfile_check should be removed to
enable full mitigation
ciao!
Trek http://www.trek.eu.org/devel
--- a/utils/start-stop-daemon.c 2019-05-18 00:35:33.278226922 +0200
+++ b/utils/start-stop-daemon.c 2019-05-18 02:57:34.405994102 +0200
@@ -2266,6 +2266,39 @@
return STATUS_OK;
}
+static void
+pidfile_check(const char *name, FILE *f)
+{
+ /* 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 the pidfile is world-writable we refuse to parse it.
+ *
+ * If we got /dev/null specified as the pidfile, we ignore the
+ * checks, as this is being used to run processes no matter
+ * what. */
+
+ struct stat st;
+ int fd;
+
+ if (strcmp(name, "/dev/null") == 0) return;
+
+ fd = fileno(f);
+
+ if (fstat(fd, &st) < 0)
+ fatale("cannot stat pidfile %s", name);
+
+ if (match_mode == MATCH_PIDFILE &&
+ ((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 on world-writable pidfile %s is insecure", name);
+}
+
static enum status_code
do_pidfile(const char *name)
{
@@ -2279,31 +2312,7 @@
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 the pidfile is world-writable we refuse to parse it.
- *
- * If we got /dev/null specified as the pidfile, we ignore the
- * checks, as this is being used to run processes no matter
- * what. */
- if (strcmp(name, "/dev/null") != 0) {
- struct stat st;
- int fd = fileno(f);
-
- if (fstat(fd, &st) < 0)
- fatale("cannot stat pidfile %s", name);
-
- if (match_mode == MATCH_PIDFILE &&
- ((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 on world-writable pidfile %s is insecure", name);
-
- }
+ pidfile_check(name, f);
if (fscanf(f, "%d", &pid) == 1)
pid_status = pid_check(pid);
--- a/utils/start-stop-daemon.c 2019-05-18 02:57:34.405994102 +0200
+++ b/utils/start-stop-daemon.c 2019-05-18 03:29:19.813942041 +0200
@@ -2267,7 +2267,7 @@
}
static void
-pidfile_check(const char *name, FILE *f)
+pidfile_check(const char *name, FILE *f, pid_t pid)
{
/* 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
@@ -2291,12 +2291,11 @@
fatale("cannot stat pidfile %s", name);
if (match_mode == MATCH_PIDFILE &&
- ((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);
+ st.st_uid != getuid() && !pid_is_user(pid, st.st_uid))
+ fatal("pidfile %s owner does not match the process uid", name);
- if (st.st_mode & 0002)
- fatal("matching on world-writable pidfile %s is insecure", name);
+ if (st.st_mode & 0022)
+ fatal("matching on group or world-writable pidfile %s is insecure", name);
}
static enum status_code
@@ -2312,11 +2311,10 @@
if (f) {
enum status_code pid_status;
- pidfile_check(name, f);
-
- if (fscanf(f, "%d", &pid) == 1)
+ if (fscanf(f, "%d", &pid) == 1) {
+ pidfile_check(name, f, pid);
pid_status = pid_check(pid);
- else
+ } else
pid_status = STATUS_UNKNOWN;
fclose(f);
Reply to: