Merge pull request #2643 from matt335672/close_unwanted_fds

Fix leaking file descriptors

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
+

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

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)

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);

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)
         {

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);
+            }
         }
     }
 

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);

tests/common/test_os_calls.c

@@ -9,6 +9,7 @@
 #include <poll.h>
 
 #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;
 }