Allow for xrdp not being able to delete PID file

If xrdp is running with dropped privileges it won't be able to delete
the PID file it's created. Places where xrdp is stopped need to cater
for this.

It's prefereable to do this than make the PID file writeable by xrdp
with dropped privileges, as this can still lead to DoS attacks if an
attacker manages to modify the PID file from a compromised xrdp
process.

common/os_calls.c

@@ -3555,6 +3555,12 @@ g_sigterm(int pid)
 #endif
 }
 
+/*****************************************************************************/
+int g_pid_is_active(int pid)
+{
+    return (kill(pid, 0) == 0);
+}
+
 /*****************************************************************************/
 /* does not work in win32 */
 int

common/os_calls.h

@@ -372,6 +372,12 @@ int      g_exit(int exit_code);
 int      g_getpid(void);
 int      g_sigterm(int pid);
 int      g_sighup(int pid);
+/*
+ * Is a particular PID active?
+ * @param pid PID to check
+ * Returns boolean
+ */
+int      g_pid_is_active(int pid);
 int      g_getuser_info_by_name(const char *username, int *uid, int *gid,
                                 char **shell, char **dir, char **gecos);
 int      g_getuser_info_by_uid(int uid, char **username, int *gid,

instfiles/Makefile.am

@@ -98,6 +98,9 @@ endif
 if FREEBSD
 # must be tab below
 install-data-hook:
-	sed -i '' 's|%%PREFIX%%|$(prefix)|g' $(DESTDIR)$(sysconfdir)/rc.d/xrdp \
-                                         $(DESTDIR)$(sysconfdir)/rc.d/xrdp-sesman
+	sed -e 's|%%PREFIX%%|$(prefix)|g' \
+	    -e 's|%%LOCALSTATEDIR%%|$(localstatedir)|g' \
+		-i '' \
+		$(DESTDIR)$(sysconfdir)/rc.d/xrdp \
+		$(DESTDIR)$(sysconfdir)/rc.d/xrdp-sesman
 endif

instfiles/init.d/xrdp

@@ -116,7 +116,7 @@ case "$1" in
         log_progress_msg $NAME
         if pidofproc -p  $PIDDIR/$NAME.pid $DAEMON  > /dev/null; then
             start-stop-daemon --stop --quiet --oknodo --pidfile $PIDDIR/$NAME.pid \
-	    --exec $DAEMON
+	    --remove-pidfile --exec $DAEMON
             value=$?
             [ $value -gt 0 ] && exitval=$value
         else

instfiles/rc.d/xrdp

@@ -48,6 +48,7 @@ command="%%PREFIX%%/sbin/xrdp"
 allstart_cmd="xrdp_allstart"
 allstop_cmd="xrdp_allstop"
 allrestart_cmd="xrdp_allrestart"
+stop_postcmd="xrdp_poststop"
 
 xrdp_allstart()
 {
@@ -79,4 +80,10 @@ xrdp_allrestart()
     run_rc_command "restart"
 }
 
+xrdp_poststop()
+{
+    # If running with dropped privileges, xrdp can't delete its own
+    # PID file
+    rm -f %%LOCALSTATEDIR%%/run/xrdp.pid
+}
 run_rc_command "$1"

xrdp/xrdp.c

@@ -448,6 +448,83 @@ check_drop_privileges(struct xrdp_startup_params *startup_params)
     return rv;
 }
 
+/*****************************************************************************/
+static int
+read_pid_file(const char *pid_file)
+{
+    int pid = -1;
+    int fd = g_file_open_ro(pid_file); /* xrdp.pid */
+    if (fd >= 0)
+    {
+        char text[32];
+        g_memset(text, 0, sizeof(text));
+        g_file_read(fd, text, sizeof(text) - 1);
+        pid = g_atoi(text);
+        g_file_close(fd);
+    }
+
+    return pid;
+}
+
+/*****************************************************************************/
+/**
+ * Kills an active xrdp daemon
+ *
+ * It is assumed that logging is not active
+ *
+ * @param pid_file PID file
+ * @return 0 for success
+ */
+static int
+kill_daemon(const char *pid_file)
+{
+    int status = 1;
+    int pid;
+    if (g_getuid() != 0)
+    {
+        g_writeln("Must be root");
+    }
+    else if ((pid = read_pid_file(pid_file)) > 0)
+    {
+        if (!g_pid_is_active(pid))
+        {
+            g_writeln("Daemon not active");
+            status = 0;
+        }
+        else
+        {
+            g_writeln("stopping process id %d", pid);
+            int i;
+            g_sigterm(pid);
+            g_sleep(100);
+            i = 5 * 1000 / 500;
+            while (i > 0 && g_pid_is_active(pid))
+            {
+                g_sleep(500);
+                --i;
+            }
+
+            if (g_pid_is_active(pid))
+            {
+                g_writeln("Can't stop process");
+            }
+            else
+            {
+                status = 0;
+            }
+        }
+
+        if (status == 0)
+        {
+            /* delete the xrdp.pid file, as xrdp can't do this
+             * if it's running without privilege */
+            g_file_delete(pid_file);
+        }
+    }
+
+    return status;
+}
+
 /*****************************************************************************/
 int
 main(int argc, char **argv)
@@ -520,36 +597,9 @@ main(int argc, char **argv)
 
     if (startup_params.kill)
     {
-        g_writeln("stopping xrdp");
-        /* read the xrdp.pid file */
-        int fd = -1;
-
-        if (g_file_exist(pid_file)) /* xrdp.pid */
-        {
-            fd = g_file_open_ro(pid_file); /* xrdp.pid */
-        }
-
-        if (fd == -1)
-        {
-            g_writeln("cannot open %s, maybe xrdp is not running", pid_file);
-        }
-        else
-        {
-            g_memset(text, 0, 32);
-            g_file_read(fd, text, 31);
-            pid = g_atoi(text);
-            g_writeln("stopping process id %d", pid);
-
-            if (pid > 0)
-            {
-                g_sigterm(pid);
-            }
-
-            g_file_close(fd);
-        }
-
+        int status = kill_daemon(pid_file);
         g_deinit();
-        g_exit(0);
+        g_exit(status);
     }
 
     /* starting logging subsystem */
@@ -587,9 +637,10 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
-    if (g_file_exist(pid_file)) /* xrdp.pid */
+    if ((pid = read_pid_file(pid_file)) > 0 && g_pid_is_active(pid))
     {
-        LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running.");
+        LOG(LOG_LEVEL_ALWAYS,
+            "It looks like xrdp (pid=%d) is already running.", pid);
         LOG(LOG_LEVEL_ALWAYS, "If not, delete %s and try again.", pid_file);
         log_end();
         g_deinit();
@@ -743,7 +794,8 @@ main(int argc, char **argv)
 
     if (daemon)
     {
-        /* delete the xrdp.pid file */
+        /* Try to delete the PID file, although if we've dropped
+         * privileges this won't be successful */
         g_file_delete(pid_file);
     }