From ce355fc2351b42bb9b10b207e960d1079573ce8c Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 28 Feb 2024 15:13:25 +0000 Subject: [PATCH] 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 | 6 +++ common/os_calls.h | 6 +++ instfiles/Makefile.am | 7 ++- instfiles/init.d/xrdp | 2 +- instfiles/rc.d/xrdp | 7 +++ xrdp/xrdp.c | 116 ++++++++++++++++++++++++++++++------------ 6 files changed, 109 insertions(+), 35 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index 53181f8c..afd5a958 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -3538,6 +3538,12 @@ g_sigterm(int pid) #endif } +/*****************************************************************************/ +int g_pid_is_active(int pid) +{ + return (kill(pid, 0) == 0); +} + /*****************************************************************************/ /* does not work in win32 */ int diff --git a/common/os_calls.h b/common/os_calls.h index 99e339b3..be06b07b 100644 --- a/common/os_calls.h +++ b/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, diff --git a/instfiles/Makefile.am b/instfiles/Makefile.am index 81275d10..9fd5d3d5 100644 --- a/instfiles/Makefile.am +++ b/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 diff --git a/instfiles/init.d/xrdp b/instfiles/init.d/xrdp index e3607b08..39f25cdd 100644 --- a/instfiles/init.d/xrdp +++ b/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 diff --git a/instfiles/rc.d/xrdp b/instfiles/rc.d/xrdp index b5dbcd25..39f35d65 100644 --- a/instfiles/rc.d/xrdp +++ b/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" diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c index 33aeb7b7..0b7a453d 100644 --- a/xrdp/xrdp.c +++ b/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); }