diff --git a/common/log.c b/common/log.c index 39ffb779..b24ac043 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; } @@ -1175,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 diff --git a/common/os_calls.c b/common/os_calls.c index e53f4df4..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 } @@ -2249,6 +2251,102 @@ 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; +} + +/*****************************************************************************/ +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 3892237f..c7fdcf3c 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -215,6 +215,18 @@ 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); +/** + * 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); 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 f69c5d67..b2318820 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); } @@ -1015,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); @@ -1117,6 +1125,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); 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; }