From b811fdb36b209fa240e2b18e15f9954dc86f4464 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 21 Apr 2023 12:20:24 +0100 Subject: [PATCH 1/6] os_calls: Add g_file_{get,set}_cloexec() functions Allows us to avoid file descriptor leaks when running a new executable --- common/log.c | 4 +--- common/os_calls.c | 43 +++++++++++++++++++++++++++++++++++++++++++ common/os_calls.h | 2 ++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/common/log.c b/common/log.c index 39ffb779..07565668 100644 --- a/common/log.c +++ b/common/log.c @@ -73,12 +73,10 @@ internal_log_file_open(const char *fname) } } -#ifdef FD_CLOEXEC if (ret != -1) { - fcntl(ret, F_SETFD, FD_CLOEXEC); + g_file_set_cloexec(ret, 1); } -#endif return ret; } diff --git a/common/os_calls.c b/common/os_calls.c index e53f4df4..ba8eaf60 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -2249,6 +2249,49 @@ g_file_lock(int fd, int start, int len) #endif } +/*****************************************************************************/ +/* Gets the close-on-exec flag for a file descriptor */ +int +g_file_get_cloexec(int fd) +{ + int rv = 0; + int flags = fcntl(fd, F_GETFD); + if (flags >= 0 && (flags & FD_CLOEXEC) != 0) + { + rv = 1; + } + + return rv; +} + +/*****************************************************************************/ +/* Sets/clears the close-on-exec flag for a file descriptor */ +/* return boolean */ +int +g_file_set_cloexec(int fd, int status) +{ + int rv = 0; + int current_flags = fcntl(fd, F_GETFD); + if (current_flags >= 0) + { + int new_flags; + if (status) + { + new_flags = current_flags | FD_CLOEXEC; + } + else + { + new_flags = current_flags & ~FD_CLOEXEC; + } + if (new_flags != current_flags) + { + rv = (fcntl(fd, F_SETFD, new_flags) >= 0); + } + } + + return rv; +} + /*****************************************************************************/ /* Converts a hex mask to a mode_t value */ #if !defined(_WIN32) diff --git a/common/os_calls.h b/common/os_calls.h index 3892237f..e9dcb4d9 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -215,6 +215,8 @@ int g_file_write(int fd, const char *ptr, int len); int g_file_seek(int fd, int offset); int g_file_lock(int fd, int start, int len); int g_file_duplicate_on(int fd, int target_fd); +int g_file_get_cloexec(int fd); +int g_file_set_cloexec(int fd, int status); int g_chmod_hex(const char *filename, int flags); int g_umask_hex(int flags); int g_chown(const char *name, int uid, int gid); From d712f3527abdd3b28232036822fb24d70c794b78 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 17 Apr 2023 20:02:59 +0100 Subject: [PATCH 2/6] os_calls: Add g_get_open_fds() --- common/os_calls.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ common/os_calls.h | 10 +++++++++ 2 files changed, 63 insertions(+) diff --git a/common/os_calls.c b/common/os_calls.c index ba8eaf60..6625723b 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -2292,6 +2292,59 @@ g_file_set_cloexec(int fd, int status) return rv; } +/*****************************************************************************/ +struct list * +g_get_open_fds(int min, int max) +{ + struct list *result = list_create(); + + if (result != NULL) + { + if (max < 0) + { + max = sysconf(_SC_OPEN_MAX); + } + + if (max > min) + { + struct pollfd *fds = g_new0(struct pollfd, max - min); + int i; + + if (fds == NULL) + { + goto nomem; + } + + for (i = min ; i < max ; ++i) + { + fds[i - min].fd = i; + } + + if (poll(fds, max - min, 0) >= 0) + { + for (i = min ; i < max ; ++i) + { + if (fds[i - min].revents != POLLNVAL) + { + // Descriptor is open + if (!list_add_item(result, i)) + { + goto nomem; + } + } + } + } + g_free(fds); + } + } + + return result; + +nomem: + list_delete(result); + return NULL; +} + /*****************************************************************************/ /* Converts a hex mask to a mode_t value */ #if !defined(_WIN32) diff --git a/common/os_calls.h b/common/os_calls.h index e9dcb4d9..c7fdcf3c 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -217,6 +217,16 @@ int g_file_lock(int fd, int start, int len); int g_file_duplicate_on(int fd, int target_fd); int g_file_get_cloexec(int fd); int g_file_set_cloexec(int fd, int status); +/** + * Get a list of open file descriptors + * + * @param min Min FD to consider + * @param max Max FD to consider (+1), or -1 for no limit + * @result Array of file descriptors, in ascending order. + * + * Call delete_list() on the result when you've finished with it. + */ +struct list *g_get_open_fds(int min, int max); int g_chmod_hex(const char *filename, int flags); int g_umask_hex(int flags); int g_chown(const char *name, int uid, int gid); From cf9e07d341fbb5733413a5758d6b38224c3963ff Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 24 Apr 2023 11:57:14 +0100 Subject: [PATCH 3/6] Add basic tests for cloexec and get_open_fds functions --- tests/common/test_os_calls.c | 67 +++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/common/test_os_calls.c b/tests/common/test_os_calls.c index 48b0e499..9343e85d 100644 --- a/tests/common/test_os_calls.c +++ b/tests/common/test_os_calls.c @@ -9,6 +9,7 @@ #include #include "os_calls.h" +#include "list.h" #include "test_common.h" @@ -198,6 +199,69 @@ START_TEST(test_g_file_get_size__5GiB) END_TEST #endif +/******************************************************************************/ +/* Just test we can set and clear the flag. We don't test its operation */ +START_TEST(test_g_file_cloexec) +{ + int flag; + int devzerofd = g_file_open("/dev/zero"); + ck_assert(devzerofd >= 0); + + (void)g_file_set_cloexec(devzerofd, 1); + flag = g_file_get_cloexec(devzerofd); + ck_assert(flag != 0); + + (void)g_file_set_cloexec(devzerofd, 0); + flag = g_file_get_cloexec(devzerofd); + ck_assert(flag == 0); + + g_file_close(devzerofd); +} +END_TEST + +/******************************************************************************/ +START_TEST(test_g_file_get_open_fds) +{ + int fd_count = get_open_fd_count(); + int i; + + struct list *start_list = g_get_open_fds(0, -1); + ck_assert_ptr_ne(start_list, NULL); + ck_assert_int_eq(start_list->count, fd_count); + + // Open another file + int devzerofd = g_file_open("/dev/zero"); + ck_assert(devzerofd >= 0); + + // Have we now got one more open file? + struct list *open_list = g_get_open_fds(0, -1); + ck_assert_ptr_ne(open_list, NULL); + ck_assert_int_eq(open_list->count, fd_count + 1); + + // Check the new file is not in the start list, but is in the open list + ck_assert_int_lt(list_index_of(start_list, devzerofd), 0); + ck_assert_int_ge(list_index_of(open_list, devzerofd), 0); + + g_file_close(devzerofd); + + struct list *finish_list = g_get_open_fds(0, -1); + ck_assert_ptr_ne(finish_list, NULL); + + // start list same as finish list? + ck_assert_int_eq(finish_list->count, fd_count); + + for (i = 0 ; i < start_list->count; ++i) + { + ck_assert_int_eq((int)finish_list->items[i], + (int)start_list->items[i]); + } + + list_delete(start_list); + list_delete(open_list); + list_delete(finish_list); +} +END_TEST + /******************************************************************************/ START_TEST(test_g_sck_fd_passing) { @@ -382,8 +446,9 @@ make_suite_test_os_calls(void) tcase_add_test(tc_os_calls, test_g_file_get_size__2GiB); tcase_add_test(tc_os_calls, test_g_file_get_size__5GiB); #endif + tcase_add_test(tc_os_calls, test_g_file_cloexec); + tcase_add_test(tc_os_calls, test_g_file_get_open_fds); tcase_add_test(tc_os_calls, test_g_sck_fd_passing); tcase_add_test(tc_os_calls, test_g_sck_fd_overflow); - return s; } From 1c798cee47fcab24310c436eb6ca0b409dee734f Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 21 Apr 2023 16:04:06 +0100 Subject: [PATCH 4/6] Logging: Add LOG_DEVEL_LOG_LEAKING_FDS --- common/log.c | 24 ++++++++++++++++++++++++ common/log.h | 21 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/common/log.c b/common/log.c index 07565668..b24ac043 100644 --- a/common/log.c +++ b/common/log.c @@ -1173,3 +1173,27 @@ getFormattedDateTime(char *replybuf, int bufsize) return replybuf; } +/*****************************************************************************/ +#ifdef USE_DEVEL_LOGGING +void +log_devel_leaking_fds(const char *exe, int min, int max) +{ + struct list *fd_list = g_get_open_fds(min, max); + + if (fd_list != NULL) + { + int i; + for (i = 0 ; i < fd_list->count ; ++i) + { + int fd = (int)fd_list->items[i]; + if (g_file_get_cloexec(fd) == 0) + { + LOG_DEVEL(LOG_LEVEL_WARNING, + "File descriptor %d is not CLOEXEC when running %s", + fd, exe); + } + } + } +} +#endif // USE_DEVEL_LOGGING + diff --git a/common/log.h b/common/log.h index d4254d9b..32906af1 100644 --- a/common/log.h +++ b/common/log.h @@ -149,6 +149,8 @@ enum logReturns #define LOG_HEXDUMP(log_level, message, buffer, length) \ log_hexdump_with_location(__func__, __FILE__, __LINE__, log_level, message, buffer, length) +#define LOG_DEVEL_LEAKING_FDS(exe,min,max) log_devel_leaking_fds(exe,min,max) + #else #define LOG(log_level, args...) log_message(log_level, args) #define LOG_HEXDUMP(log_level, message, buffer, length) \ @@ -160,6 +162,7 @@ enum logReturns #define LOG_DEVEL(log_level, args...) UNUSED_VAR(LOG_STARTUP_OK) #define LOG_DEVEL_HEXDUMP(log_level, message, buffer, length) UNUSED_VAR(LOG_STARTUP_OK) +#define LOG_DEVEL_LEAKING_FDS(exe,min,max) #endif /* Flags values for log_start() */ @@ -440,4 +443,22 @@ char *getLogFile(char *replybuf, int bufsize); * @return */ char *getFormattedDateTime(char *replybuf, int bufsize); + +#ifdef USE_DEVEL_LOGGING +/** + * Log open file descriptors not cloexec before execing another program + * + * Used to ensure file descriptors aren't leaking when running + * non-privileged executables + * + * Use the LOG_DEVEL_LEAKING_FDS() macro to invoke this function + * + * @param exe Executable we're about to launch + * @param min Minimum FD to consider + * @param max Maximum FD to consider + 1, or -1 for no upper FD + */ +void +log_devel_leaking_fds(const char *exe, int min, int max); +#endif + #endif From adb7476187d2f84de4c05ffcf8e3c9ab0e7de363 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 24 Apr 2023 11:36:33 +0100 Subject: [PATCH 5/6] Add LOG_DEVEL_LEAKING_FDS calls to the application --- sesman/session.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sesman/session.c b/sesman/session.c index f69c5d67..ca4a73ba 100644 --- a/sesman/session.c +++ b/sesman/session.c @@ -156,6 +156,8 @@ start_chansrv(struct auth_info *auth_info, g_cfg->env_names, g_cfg->env_values); + LOG_DEVEL_LEAKING_FDS("chansrv", 3, -1); + /* executing chansrv */ g_execvp_list(exe_path, chansrv_params); @@ -273,6 +275,8 @@ start_window_manager(struct auth_info *auth_info, g_cfg->env_values); auth_set_env(auth_info); + LOG_DEVEL_LEAKING_FDS("window manager", 3, -1); + if (s->directory[0] != '\0') { if (g_cfg->sec.allow_alternate_shell) @@ -555,6 +559,7 @@ start_x_server(struct auth_info *auth_info, LOG(LOG_LEVEL_INFO, "Starting X server on display %u: %s", s->display, dumpItemsToString(xserver_params, execvpparams, 2048)); + LOG_DEVEL_LEAKING_FDS("X server", 3, -1); g_execvp_list((const char *)xserver_params->items[0], xserver_params); } @@ -1117,6 +1122,8 @@ session_reconnect(int display, int uid, if (g_file_exist(g_cfg->reconnect_sh)) { + LOG_DEVEL_LEAKING_FDS("reconnect script", 3, -1); + LOG(LOG_LEVEL_INFO, "Starting session reconnection script on display %d: %s", display, g_cfg->reconnect_sh); From f08355a325220291c42e23a7acaf70f355df44ad Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 17 Jan 2023 10:55:19 +0000 Subject: [PATCH 6/6] Ensure commonly used file descriptors are close-on-exec --- common/os_calls.c | 2 ++ common/trans.c | 7 +++++++ sesman/lock_uds.c | 4 ++++ sesman/session.c | 7 +++++-- 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index 6625723b..c10d6215 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -1721,6 +1721,8 @@ g_create_wait_obj(const char *name) close(fds[1]); return 0; } + g_file_set_cloexec(fds[0], 1); + g_file_set_cloexec(fds[1], 1); return (fds[1] << 16) | fds[0]; #endif } diff --git a/common/trans.c b/common/trans.c index 0a6db4ad..b0358c3b 100644 --- a/common/trans.c +++ b/common/trans.c @@ -355,6 +355,7 @@ trans_check_wait_objs(struct trans *self) in_trans->type1 = TRANS_TYPE_SERVER; in_trans->status = TRANS_STATUS_UP; in_trans->is_term = self->is_term; + g_file_set_cloexec(in_sck, 1); g_sck_set_non_blocking(in_sck); if (self->trans_conn_in(self, in_trans) != 0) { @@ -796,6 +797,7 @@ trans_connect(struct trans *self, const char *server, const char *port, } /* Try to connect asynchronously */ + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); error = f_connect(self->sck, server, port); if (error == 0) @@ -881,6 +883,7 @@ trans_listen_address(struct trans *self, const char *port, const char *address) return 1; } + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); if (g_tcp_bind_address(self->sck, port, address) == 0) @@ -905,6 +908,7 @@ trans_listen_address(struct trans *self, const char *port, const char *address) return 1; } + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); if (g_tcp_local_bind(self->sck, port) == 0) @@ -928,6 +932,7 @@ trans_listen_address(struct trans *self, const char *port, const char *address) return 1; } + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); if (g_sck_vsock_bind_address(self->sck, port, address) == 0) @@ -947,6 +952,7 @@ trans_listen_address(struct trans *self, const char *port, const char *address) { return 1; } + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); if (g_tcp4_bind_address(self->sck, port, address) == 0) { @@ -965,6 +971,7 @@ trans_listen_address(struct trans *self, const char *port, const char *address) { return 1; } + g_file_set_cloexec(self->sck, 1); g_tcp_set_non_blocking(self->sck); if (g_tcp6_bind_address(self->sck, port, address) == 0) { diff --git a/sesman/lock_uds.c b/sesman/lock_uds.c index b2a53ba0..daab99d1 100644 --- a/sesman/lock_uds.c +++ b/sesman/lock_uds.c @@ -101,6 +101,10 @@ lock_uds(const char *sockname) g_file_close(fd); fd = -1; } + else + { + (void)g_file_set_cloexec(fd, 1); + } } } diff --git a/sesman/session.c b/sesman/session.c index ca4a73ba..b2318820 100644 --- a/sesman/session.c +++ b/sesman/session.c @@ -1020,9 +1020,12 @@ session_start(struct auth_info *auth_info, { /** * We're now forked from the main sesman process, so we - * can close file descriptors that we no longer need */ - + * can close file descriptors that we no longer need + * + * Set FD_CLOEXEC on the FD used to send our status back to + * sesman, as our sub-processes shouldn't be able to see it */ g_file_close(fd[0]); + g_file_set_cloexec(fd[1], 1); sesman_close_all(0);