From ddff9ebb322f3a6f3b0b6215e626dd6018b18235 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 27 Feb 2024 11:53:34 +0000 Subject: [PATCH] Refactor xrdp_listen to allow for privilege drop - xrdp_listen.c is refactored so we can create the listening socket(s) before dropping privileges. - The code which reads startup params from xrdp.ini is moved from xrdp_listen.c to xrdp.c, so it is only called once if we test the listen before starting the daemon. --- common/thread_calls.c | 7 +- xrdp/xrdp.c | 216 ++++++++++++++++++++++++++++++++++-------- xrdp/xrdp.h | 7 +- xrdp/xrdp_listen.c | 147 ++-------------------------- 4 files changed, 195 insertions(+), 182 deletions(-) diff --git a/common/thread_calls.c b/common/thread_calls.c index 0b821b8f..b575d4ea 100644 --- a/common/thread_calls.c +++ b/common/thread_calls.c @@ -122,8 +122,11 @@ tc_mutex_delete(tbus mutex) pthread_mutex_t *lmutex; lmutex = (pthread_mutex_t *)mutex; - pthread_mutex_destroy(lmutex); - g_free(lmutex); + if (lmutex != NULL) + { + pthread_mutex_destroy(lmutex); + g_free(lmutex); + } #endif } diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c index 08377b23..9c483819 100644 --- a/xrdp/xrdp.c +++ b/xrdp/xrdp.c @@ -240,6 +240,127 @@ xrdp_process_params(int argc, char **argv, return 0; } +/*****************************************************************************/ +/** + * + * @brief Read additional startup parameters from xrdp.ini + * + * @param [in,out] startup parameters from the command line + * @return 0 on success + * + */ +static int +read_xrdp_ini_startup_params(struct xrdp_startup_params *startup_params) +{ + int rv = 0; + int fd; + int index; + int port_override; + int fork_override; + const char *name; + const char *val; + struct list *names; + struct list *values; + + port_override = startup_params->port[0] != 0; + fork_override = startup_params->fork; + names = list_create(); + names->auto_free = 1; + values = list_create(); + values->auto_free = 1; + + fd = g_file_open_ro(startup_params->xrdp_ini); + if (fd < 0) + { + LOG(LOG_LEVEL_ERROR, "Can't open %s [%s]", startup_params->xrdp_ini, + g_get_strerror()); + rv = 1; + } + else if (file_read_section(fd, "globals", names, values) != 0) + { + LOG(LOG_LEVEL_ERROR, "Can't read [Globals] from %s", + startup_params->xrdp_ini); + rv = 1; + } + else + { + for (index = 0; index < names->count; index++) + { + name = (const char *)list_get_item(names, index); + val = (const char *)list_get_item(values, index); + if (name == 0 || val == 0) + { + continue; + } + + if (g_strcasecmp(name, "port") == 0) + { + if (port_override == 0) + { + g_strncpy(startup_params->port, val, + sizeof(startup_params->port) - 1); + } + } + + else if (g_strcasecmp(name, "fork") == 0) + { + if (fork_override == 0) + { + startup_params->fork = g_text2bool(val); + } + } + + else if (g_strcasecmp(name, "tcp_nodelay") == 0) + { + startup_params->tcp_nodelay = g_text2bool(val); + } + + else if (g_strcasecmp(name, "tcp_keepalive") == 0) + { + startup_params->tcp_keepalive = g_text2bool(val); + } + + else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0) + { + startup_params->tcp_send_buffer_bytes = g_atoi(val); + } + + else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0) + { + startup_params->tcp_recv_buffer_bytes = g_atoi(val); + } + + else if (g_strcasecmp(name, "use_vsock") == 0) + { + startup_params->use_vsock = g_text2bool(val); + } + + else if (g_strcasecmp(name, "runtime_user") == 0) + { + g_snprintf(startup_params->runtime_user, + sizeof(startup_params->runtime_user), + "%s", val); + } + + else if (g_strcasecmp(name, "runtime_group") == 0) + { + g_snprintf(startup_params->runtime_group, + sizeof(startup_params->runtime_group), + "%s", val); + } + } + } + + list_delete(names); + list_delete(values); + if (fd >= 0) + { + g_file_close(fd); + } + return rv; +} + + /*****************************************************************************/ /* Basic sanity checks before any forking */ static int @@ -299,7 +420,7 @@ xrdp_sanity_check(void) int main(int argc, char **argv) { - int exit_status = 0; + int exit_status = 1; enum logReturns error; struct xrdp_startup_params startup_params = {0}; int pid; @@ -428,6 +549,13 @@ main(int argc, char **argv) g_exit(1); } + if (!read_xrdp_ini_startup_params(&startup_params)) + { + log_end(); + g_deinit(); + g_exit(1); + } + if (g_file_exist(pid_file)) /* xrdp.pid */ { LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running."); @@ -473,8 +601,14 @@ main(int argc, char **argv) if (daemon) { - /* if can't listen, exit with failure status */ - if (xrdp_listen_test(&startup_params) != 0) + /* Before daemonising, check we can listen. + * If we can't listen, exit with failure status */ + struct xrdp_listen *xrdp_listen; + xrdp_listen = xrdp_listen_create(&startup_params); + int status = xrdp_listen_init(xrdp_listen); + xrdp_listen_delete(xrdp_listen); + + if (status != 0) { LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, " "possibly address already in use."); @@ -540,43 +674,51 @@ main(int argc, char **argv) /* end of daemonizing code */ } - g_set_threadid(tc_get_threadid()); - g_listen = xrdp_listen_create(); - g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */ - g_signal_pipe(xrdp_sig_no_op); /* SIGPIPE */ - g_signal_terminate(xrdp_shutdown); /* SIGTERM */ - g_signal_child_stop(xrdp_child); /* SIGCHLD */ - g_signal_hang_up(xrdp_sig_no_op); /* SIGHUP */ - g_set_sync_mutex(tc_mutex_create()); - g_set_sync1_mutex(tc_mutex_create()); - pid = g_getpid(); - LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid); - g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid); - g_set_term_event(g_create_wait_obj(text)); - - if (g_get_term() == 0) + g_listen = xrdp_listen_create(&startup_params); + if (xrdp_listen_init(g_listen) != 0) { - LOG(LOG_LEVEL_WARNING, "error creating g_term_event"); + LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, " + "possibly address already in use."); + } + else + { + g_set_threadid(tc_get_threadid()); + g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */ + g_signal_pipe(xrdp_sig_no_op); /* SIGPIPE */ + g_signal_terminate(xrdp_shutdown); /* SIGTERM */ + g_signal_child_stop(xrdp_child); /* SIGCHLD */ + g_signal_hang_up(xrdp_sig_no_op); /* SIGHUP */ + g_set_sync_mutex(tc_mutex_create()); + g_set_sync1_mutex(tc_mutex_create()); + pid = g_getpid(); + LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid); + g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid); + g_set_term_event(g_create_wait_obj(text)); + + if (g_get_term() == 0) + { + LOG(LOG_LEVEL_WARNING, "error creating g_term_event"); + } + + g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid); + g_set_sigchld_event(g_create_wait_obj(text)); + + if (g_get_sigchld() == 0) + { + LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event"); + } + + g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid); + g_set_sync_event(g_create_wait_obj(text)); + + if (g_get_sync_event() == 0) + { + LOG(LOG_LEVEL_WARNING, "error creating g_sync_event"); + } + + exit_status = xrdp_listen_main_loop(g_listen); } - g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid); - g_set_sigchld_event(g_create_wait_obj(text)); - - if (g_get_sigchld() == 0) - { - LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event"); - } - - g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid); - g_set_sync_event(g_create_wait_obj(text)); - - if (g_get_sync_event() == 0) - { - LOG(LOG_LEVEL_WARNING, "error creating g_sync_event"); - } - - g_listen->startup_params = &startup_params; - exit_status = xrdp_listen_main_loop(g_listen); xrdp_listen_delete(g_listen); tc_mutex_delete(g_get_sync_mutex()); diff --git a/xrdp/xrdp.h b/xrdp/xrdp.h index 91a26217..84e27295 100644 --- a/xrdp/xrdp.h +++ b/xrdp/xrdp.h @@ -194,13 +194,14 @@ xrdp_process_main_loop(struct xrdp_process *self); /* xrdp_listen.c */ struct xrdp_listen * -xrdp_listen_create(void); +xrdp_listen_create(struct xrdp_startup_params *startup_params); +int +xrdp_listen_init(struct xrdp_listen *self); void xrdp_listen_delete(struct xrdp_listen *self); int xrdp_listen_main_loop(struct xrdp_listen *self); -int -xrdp_listen_test(struct xrdp_startup_params *startup_params); + /* xrdp_region.c */ struct xrdp_region * diff --git a/xrdp/xrdp_listen.c b/xrdp/xrdp_listen.c index 618b96ed..c13d06fb 100644 --- a/xrdp/xrdp_listen.c +++ b/xrdp/xrdp_listen.c @@ -55,7 +55,7 @@ xrdp_listen_create_pro_done(struct xrdp_listen *self) /*****************************************************************************/ struct xrdp_listen * -xrdp_listen_create(void) +xrdp_listen_create(struct xrdp_startup_params *startup_params) { struct xrdp_listen *self; @@ -64,6 +64,7 @@ xrdp_listen_create(void) self->trans_list = list_create(); self->process_list = list_create(); self->fork_list = list_create(); + self->startup_params = startup_params; if (g_process_sem == 0) { @@ -153,107 +154,6 @@ xrdp_process_run(void *in_val) LOG_DEVEL(LOG_LEVEL_TRACE, "process done"); return 0; } - -/*****************************************************************************/ -static int -xrdp_listen_get_startup_params(struct xrdp_listen *self) -{ - int fd; - int index; - int port_override; - int fork_override; - const char *name; - const char *val; - struct list *names; - struct list *values; - struct xrdp_startup_params *startup_params; - - startup_params = self->startup_params; - port_override = startup_params->port[0] != 0; - fork_override = startup_params->fork; - fd = g_file_open_ro(startup_params->xrdp_ini); - if (fd != -1) - { - names = list_create(); - names->auto_free = 1; - values = list_create(); - values->auto_free = 1; - if (file_read_section(fd, "globals", names, values) == 0) - { - for (index = 0; index < names->count; index++) - { - name = (const char *)list_get_item(names, index); - val = (const char *)list_get_item(values, index); - if (name == 0 || val == 0) - { - continue; - } - - if (g_strcasecmp(name, "port") == 0) - { - if (port_override == 0) - { - g_strncpy(startup_params->port, val, - sizeof(startup_params->port) - 1); - } - } - - else if (g_strcasecmp(name, "fork") == 0) - { - if (fork_override == 0) - { - startup_params->fork = g_text2bool(val); - } - } - - else if (g_strcasecmp(name, "tcp_nodelay") == 0) - { - startup_params->tcp_nodelay = g_text2bool(val); - } - - else if (g_strcasecmp(name, "tcp_keepalive") == 0) - { - startup_params->tcp_keepalive = g_text2bool(val); - } - - else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0) - { - startup_params->tcp_send_buffer_bytes = g_atoi(val); - } - - else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0) - { - startup_params->tcp_recv_buffer_bytes = g_atoi(val); - } - - else if (g_strcasecmp(name, "use_vsock") == 0) - { - startup_params->use_vsock = g_text2bool(val); - } - - else if (g_strcasecmp(name, "runtime_user") == 0) - { - g_snprintf(startup_params->runtime_user, - sizeof(startup_params->runtime_user), - "%s", val); - } - - else if (g_strcasecmp(name, "runtime_group") == 0) - { - g_snprintf(startup_params->runtime_group, - sizeof(startup_params->runtime_group), - "%s", val); - } - } - } - - list_delete(names); - list_delete(values); - g_file_close(fd); - } - return 0; -} - /*****************************************************************************/ static int xrdp_listen_stop_all_listen(struct xrdp_listen *self) @@ -663,8 +563,10 @@ xrdp_listen_pp(struct xrdp_listen *self, int *index, } /*****************************************************************************/ -static int -xrdp_listen_process_startup_params(struct xrdp_listen *self) +/* returns 0 if xrdp is listening correctly + returns 1 if xrdp is not listening correctly */ +int +xrdp_listen_init(struct xrdp_listen *self) { int mode; /* TRANS_MODE_TCP*, TRANS_MODE_UNIX, TRANS_MODE_VSOCK */ int error; @@ -898,18 +800,7 @@ xrdp_listen_main_loop(struct xrdp_listen *self) struct trans *ltrans; self->status = 1; - if (xrdp_listen_get_startup_params(self) != 0) - { - LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed"); - self->status = -1; - return 1; - } - if (xrdp_listen_process_startup_params(self) != 0) - { - LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed"); - self->status = -1; - return 1; - } + term_obj = g_get_term(); /*Global termination event */ sigchld_obj = g_get_sigchld(); sync_obj = g_get_sync_event(); @@ -1049,27 +940,3 @@ xrdp_listen_main_loop(struct xrdp_listen *self) self->status = -1; return 0; } - -/*****************************************************************************/ -/* returns 0 if xrdp can listen - returns 1 if xrdp cannot listen */ -int -xrdp_listen_test(struct xrdp_startup_params *startup_params) -{ - struct xrdp_listen *xrdp_listen; - - xrdp_listen = xrdp_listen_create(); - xrdp_listen->startup_params = startup_params; - if (xrdp_listen_get_startup_params(xrdp_listen) != 0) - { - xrdp_listen_delete(xrdp_listen); - return 1; - } - if (xrdp_listen_process_startup_params(xrdp_listen) != 0) - { - xrdp_listen_delete(xrdp_listen); - return 1; - } - xrdp_listen_delete(xrdp_listen); - return 0; -}