Stop file descriptors leaking into the session processes

17 files changed, 267 insertions, 38 deletions

diff --git a/NEWS b/NEWS
index 4b853c3a..af1cd352 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
 Overview of changes in lightdm 1.1.4
 
+    * Stop file descriptors leaking into the session processes
     * Change session directory once user permissions are set so it works
       on NFS filesystems that don't allow root to access files.
     * Restructure session code so the PAM authentication is run in its
diff --git a/src/display.c b/src/display.c
index da6949dd..40a0efcf 100644
--- a/src/display.c
+++ b/src/display.c
@@ -430,7 +430,7 @@ start_greeter (Display *display)
     }
 
     g_signal_connect_after (display->priv->session, "stopped", G_CALLBACK (greeter_session_stopped_cb), display);
-    result = session_start (display->priv->session, display->priv->pam_service, greeter_user, FALSE, FALSE);
+    result = greeter_start (display->priv->greeter, display->priv->pam_service, greeter_user);
     g_free (greeter_user);
 
     if (!result)
diff --git a/src/greeter.c b/src/greeter.c
index 434a6bd3..bcdc2ae3 100644
--- a/src/greeter.c
+++ b/src/greeter.c
@@ -61,8 +61,6 @@ struct GreeterPrivate
     gboolean guest_account_authenticated;
 
     /* Communication channels to communicate with */
-    int to_greeter_pipe[2];
-    int from_greeter_pipe[2];
     GIOChannel *to_greeter_channel;
     GIOChannel *from_greeter_channel;
 };
@@ -95,38 +93,12 @@ static gboolean read_cb (GIOChannel *source, GIOCondition condition, gpointer da
 Greeter *
 greeter_new (Session *session, const gchar *pam_service)
 {
-    gchar *value;
     Greeter *greeter;
 
     greeter = g_object_new (GREETER_TYPE, NULL);
     greeter->priv->session = g_object_ref (session);
     greeter->priv->pam_service = g_strdup (pam_service);
 
-    /* Create a pipe to talk with the greeter */
-    if (pipe (greeter->priv->to_greeter_pipe) != 0 || pipe (greeter->priv->from_greeter_pipe) != 0)
-    {
-        g_warning ("Failed to create pipes: %s", strerror (errno));
-        //return;
-    }
-    greeter->priv->to_greeter_channel = g_io_channel_unix_new (greeter->priv->to_greeter_pipe[1]);
-    g_io_channel_set_encoding (greeter->priv->to_greeter_channel, NULL, NULL);
-    greeter->priv->from_greeter_channel = g_io_channel_unix_new (greeter->priv->from_greeter_pipe[0]);
-    g_io_channel_set_encoding (greeter->priv->from_greeter_channel, NULL, NULL);
-    g_io_channel_set_buffered (greeter->priv->from_greeter_channel, FALSE);
-    g_io_add_watch (greeter->priv->from_greeter_channel, G_IO_IN | G_IO_HUP, read_cb, greeter);
-
-    /* Let the greeter session know how to communicate with the daemon */
-    value = g_strdup_printf ("%d", greeter->priv->from_greeter_pipe[1]);
-    session_set_env (greeter->priv->session, "LIGHTDM_TO_SERVER_FD", value);
-    g_free (value);
-    value = g_strdup_printf ("%d", greeter->priv->to_greeter_pipe[0]);
-    session_set_env (greeter->priv->session, "LIGHTDM_FROM_SERVER_FD", value);
-    g_free (value);
-
-    /* Don't allow the daemon end of the pipes to be accessed in child processes */
-    fcntl (greeter->priv->to_greeter_pipe[1], F_SETFD, FD_CLOEXEC);
-    fcntl (greeter->priv->from_greeter_pipe[0], F_SETFD, FD_CLOEXEC);
-
     return greeter;
 }
 
@@ -650,6 +622,47 @@ greeter_get_start_session (Greeter *greeter)
     return greeter->priv->start_session;
 }
 
+gboolean
+greeter_start (Greeter *greeter, const gchar *service, const gchar *username)
+{
+    int to_greeter_pipe[2], from_greeter_pipe[2];
+    gboolean result = FALSE;
+    gchar *value;
+
+    /* Create a pipe to talk with the greeter */
+    if (pipe (to_greeter_pipe) != 0 || pipe (from_greeter_pipe) != 0)
+    {
+        g_warning ("Failed to create pipes: %s", strerror (errno));
+        return FALSE;
+    }
+    greeter->priv->to_greeter_channel = g_io_channel_unix_new (to_greeter_pipe[1]);
+    g_io_channel_set_encoding (greeter->priv->to_greeter_channel, NULL, NULL);
+    greeter->priv->from_greeter_channel = g_io_channel_unix_new (from_greeter_pipe[0]);
+    g_io_channel_set_encoding (greeter->priv->from_greeter_channel, NULL, NULL);
+    g_io_channel_set_buffered (greeter->priv->from_greeter_channel, FALSE);
+    g_io_add_watch (greeter->priv->from_greeter_channel, G_IO_IN | G_IO_HUP, read_cb, greeter);
+
+    /* Let the greeter session know how to communicate with the daemon */
+    value = g_strdup_printf ("%d", from_greeter_pipe[1]);
+    session_set_env (greeter->priv->session, "LIGHTDM_TO_SERVER_FD", value);
+    g_free (value);
+    value = g_strdup_printf ("%d", to_greeter_pipe[0]);
+    session_set_env (greeter->priv->session, "LIGHTDM_FROM_SERVER_FD", value);
+    g_free (value);
+
+    /* Don't allow the daemon end of the pipes to be accessed in child processes */
+    fcntl (to_greeter_pipe[1], F_SETFD, FD_CLOEXEC);
+    fcntl (from_greeter_pipe[0], F_SETFD, FD_CLOEXEC);
+
+    result = session_start (greeter->priv->session, service, username, FALSE, FALSE);
+
+    /* Close the session ends of the pipe */
+    close (to_greeter_pipe[0]);
+    close (from_greeter_pipe[1]);
+
+    return result;
+}
+
 static Session *
 greeter_real_start_authentication (Greeter *greeter, const gchar *username)
 {
diff --git a/src/greeter.h b/src/greeter.h
index 56eea002..88fb2bc8 100644
--- a/src/greeter.h
+++ b/src/greeter.h
@@ -49,6 +49,8 @@ Session *greeter_get_authentication_session (Greeter *greeter);
 
 gboolean greeter_get_start_session (Greeter *greeter);
 
+gboolean greeter_start (Greeter *greeter, const gchar *service, const gchar *username);
+
 G_END_DECLS
 
 #endif /* _GREETER_H_ */
diff --git a/src/lightdm.c b/src/lightdm.c
index 4bc5c0c1..e2b9f8d2 100644
--- a/src/lightdm.c
+++ b/src/lightdm.c
@@ -124,6 +124,7 @@ log_init (void)
     g_free (log_dir);
 
     log_fd = open (path, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    fcntl (log_fd, F_SETFD, FD_CLOEXEC);
     g_log_set_default_handler (log_cb, NULL);
 
     g_debug ("Logging to %s", path);
diff --git a/src/process.c b/src/process.c
index 5236ab8d..2910cd99 100644
--- a/src/process.c
+++ b/src/process.c
@@ -377,6 +377,8 @@ process_class_init (ProcessClass *klass)
     processes = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
     if (pipe (signal_pipe) != 0)
         g_critical ("Failed to create signal pipe");
+    fcntl (signal_pipe[0], F_SETFD, FD_CLOEXEC);
+    fcntl (signal_pipe[1], F_SETFD, FD_CLOEXEC);
     g_io_add_watch (g_io_channel_unix_new (signal_pipe[0]), G_IO_IN, handle_signal, NULL);
     action.sa_sigaction = signal_cb;
     sigemptyset (&action.sa_mask);
diff --git a/src/session-child.c b/src/session-child.c
index 9ea86deb..7eaa645c 100644
--- a/src/session-child.c
+++ b/src/session-child.c
@@ -170,7 +170,7 @@ session_child_run (int argc, char **argv)
     GDBusConnection *bus;
     gchar *console_kit_cookie;
     GError *error = NULL;
-  
+
     g_type_init ();
 
     /* Make input non-blocking */
@@ -193,10 +193,13 @@ session_child_run (int argc, char **argv)
     to_daemon_input = atoi (argv[3]);
     if (from_daemon_output == 0 || to_daemon_input == 0)
     {
-        g_printerr ("Invalid LIGHTDM_DAEMON_PIPE\n");
+        g_printerr ("Invalid file descriptors %s %s\n", argv[2], argv[3]);
         return EXIT_FAILURE;
     }
-    g_unsetenv ("LIGHTDM_DAEMON_PIPE");
+
+    /* Don't let these pipes leak to the command we will run */
+    fcntl (from_daemon_output, F_SETFD, FD_CLOEXEC);
+    fcntl (to_daemon_input, F_SETFD, FD_CLOEXEC);
 
     /* Read a version number so we can handle upgrades (i.e. a newer version of session child is run for an old daemon */
     read_data (&version, sizeof (version));
diff --git a/src/session.c b/src/session.c
index cb300d65..c2b4e7ad 100644
--- a/src/session.c
+++ b/src/session.c
@@ -326,7 +326,7 @@ session_start (Session *session, const gchar *service, const gchar *username, gb
     /* Create pipes to talk to the child */
     if (pipe (to_child_pipe) < 0 || pipe (from_child_pipe) < 0)
     {
-        g_warning ("Failed to create pipe to communicated with session process: %s", strerror (errno));
+        g_warning ("Failed to create pipe to communicate with session process: %s", strerror (errno));
         return FALSE;
     }
     to_child_output = to_child_pipe[0];
diff --git a/tests/Makefile.am b/tests/Makefile.am
index e9a893e8..99d91175 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -91,7 +91,9 @@ TESTS = \
 	test-xdmcp-login \
 	test-no-accounts-service \
 	test-console-kit \
-	test-no-console-kit
+	test-no-console-kit \
+	test-open-file-descriptors \
+	test-xdmcp-open-file-descriptors
 
 #	test-session-exit-error
 #	test-greeter-no-exit
diff --git a/tests/scripts/open-file-descriptors.conf b/tests/scripts/open-file-descriptors.conf
new file mode 100644
index 00000000..13dad3c1
--- /dev/null
+++ b/tests/scripts/open-file-descriptors.conf
@@ -0,0 +1,47 @@
+#
+# Check session doesn't have any file descriptors to the daemon open.
+# Use a greeter so its file descriptors are around at the time the session starts.
+#
+
+[LightDM]
+minimum-display-number=50
+
+#?RUNNER DAEMON-START
+
+# X server starts
+#?XSERVER :50 START
+#?XSERVER :50 INDICATE-READY
+
+# LightDM connects to X server
+#?XSERVER :50 ACCEPT-CONNECT
+
+# Greeter starts
+#?GREETER :50 START
+#?XSERVER :50 ACCEPT-CONNECT
+#?GREETER :50 CONNECT-XSERVER
+#?GREETER :50 CONNECT-TO-DAEMON
+#?GREETER :50 CONNECTED-TO-DAEMON
+
+# Log in
+#?*GREETER :50 AUTHENTICATE USERNAME=have-password1
+#?GREETER :50 SHOW-PROMPT TEXT="Password:"
+#?*GREETER :50 RESPOND TEXT="password"
+#?GREETER :50 AUTHENTICATION-COMPLETE USERNAME=have-password1 AUTHENTICATED=TRUE
+#?*GREETER :50 START-SESSION
+#?GREETER :50 TERMINATE SIGNAL=15
+
+# Session starts
+#?SESSION :50 START USER=have-password1
+#?XSERVER :50 ACCEPT-CONNECT
+#?SESSION :50 CONNECT-XSERVER
+
+# Check file descriptors
+#?*SESSION :50 LIST-UNKNOWN-FILE-DESCRIPTORS
+#?SESSION :50 LIST-UNKNOWN-FILE-DESCRIPTORS FDS=
+
+# Cleanup
+#?*STOP-DAEMON
+# Don't know what order they will terminate
+#?(SESSION :50 TERMINATE SIGNAL=15|XSERVER :50 TERMINATE SIGNAL=15)
+#?(SESSION :50 TERMINATE SIGNAL=15|XSERVER :50 TERMINATE SIGNAL=15)
+#?RUNNER DAEMON-EXIT STATUS=0
diff --git a/tests/scripts/vnc-open-file-descriptors.conf b/tests/scripts/vnc-open-file-descriptors.conf
new file mode 100644
index 00000000..d4daa689
--- /dev/null
+++ b/tests/scripts/vnc-open-file-descriptors.conf
@@ -0,0 +1,62 @@
+#
+# Check that a VNC session doesn't have any unknown file descriptors
+#
+
+[LightDM]
+minimum-display-number=50
+start-default-seat=false
+
+[VNCServer]
+enabled=true
+port=9999
+
+#?RUNNER DAEMON-START
+#?*WAIT 1
+
+# Start a VNC client
+#?*START-VNC-CLIENT ARGS="::9999"
+#?VNC-CLIENT START
+#?VNC-CLIENT CONNECT SERVER=::9999
+
+# Xvnc server starts
+#?XSERVER :50 START GEOMETRY=1024x768 DEPTH=8
+
+# Negotiate with Xvnc
+#?VNC-CLIENT CONNECTED VERSION="RFB 003.007"
+
+#?XSERVER :50 INDICATE-READY
+
+#?XSERVER :50 VNC-CLIENT-CONNECT VERSION="RFB 003.003"
+
+# LightDM connects to X server
+#?XSERVER :50 ACCEPT-CONNECT
+
+# Greeter starts and connects to remote X server
+#?GREETER :50 START
+#?XSERVER :50 ACCEPT-CONNECT
+#?GREETER :50 CONNECT-XSERVER
+#?GREETER :50 CONNECT-TO-DAEMON
+#?GREETER :50 CONNECTED-TO-DAEMON
+
+# Log in
+#?*GREETER :50 AUTHENTICATE USERNAME=have-password1
+#?GREETER :50 SHOW-PROMPT TEXT="Password:"
+#?*GREETER :50 RESPOND TEXT="password"
+#?GREETER :50 AUTHENTICATION-COMPLETE USERNAME=have-password1 AUTHENTICATED=TRUE
+#?*GREETER :50 START-SESSION
+#?GREETER :50 TERMINATE SIGNAL=15
+
+# Session starts
+#?SESSION :50 START USER=have-password1
+#?XSERVER :50 ACCEPT-CONNECT
+#?SESSION :50 CONNECT-XSERVER
+
+# Check file descriptors
+#?*SESSION :50 LIST-UNKNOWN-FILE-DESCRIPTORS
+#?SESSION :50 LIST-UNKNOWN-FILE-DESCRIPTORS FDS=
+
+# Clean up
+#?*STOP-DAEMON
+#?(SESSION :50 TERMINATE SIGNAL=15|XSERVER :50 TERMINATE SIGNAL=15)
+#?(SESSION :50 TERMINATE SIGNAL=15|XSERVER :50 TERMINATE SIGNAL=15)
+#?RUNNER DAEMON-EXIT STATUS=0
diff --git a/tests/scripts/xdmcp-open-file-descriptors.conf b/tests/scripts/xdmcp-open-file-descriptors.conf
new file mode 100644
index 00000000..779b6be9
--- /dev/null
+++ b/tests/scripts/xdmcp-open-file-descriptors.conf
@@ -0,0 +1,57 @@
+#
+# Check that an XDMCP session doesn't have any unknown file descriptors
+#
+
+[LightDM]
+start-default-seat=false
+minimum-display-number=50
+
+[XDMCPServer]
+enabled=true
+port=9999
+
+#?RUNNER DAEMON-START
+#?*WAIT 1
+
+# Start a remote X server to log in with XDMCP
+#?*START-XSERVER ARGS=":98 -query localhost -port 9999 -nolisten unix"
+#?XSERVER :98 START
+#?XSERVER :98 SEND-QUERY
+
+# Negotiate with daemon
+#?XSERVER :98 GOT-WILLING AUTHENTICATION-NAME="" HOSTNAME="" STATUS=""
+#?XSERVER :98 SEND-REQUEST DISPLAY-NUMBER=98 AUTHORIZATION-NAME="MIT-MAGIC-COOKIE-1" MFID="TEST XSERVER"
+#?XSERVER :98 GOT-ACCEPT SESSION-ID=[0-9]* AUTHENTICATION-NAME="" AUTHORIZATION-NAME="MIT-MAGIC-COOKIE-1"
+#?XSERVER :98 SEND-MANAGE SESSION-ID=[0-9]* DISPLAY-NUMBER=98 DISPLAY-CLASS="DISPLAY CLASS"
+
+# LightDM connects to X server
+#?XSERVER :98 TCP-ACCEPT-CONNECT
+
+# Greeter starts and connects to remote X server
+#?GREETER 127.0.0.1:98 START
+#?XSERVER :98 TCP-ACCEPT-CONNECT
+#?GREETER 127.0.0.1:98 CONNECT-XSERVER
+#?GREETER 127.0.0.1:98 CONNECT-TO-DAEMON
+#?GREETER 127.0.0.1:98 CONNECTED-TO-DAEMON
+
+# Log in
+#?*GREETER 127.0.0.1:98 AUTHENTICATE USERNAME=have-password1
+#?GREETER 127.0.0.1:98 SHOW-PROMPT TEXT="Password:"
+#?*GREETER 127.0.0.1:98 RESPOND TEXT="password"
+#?GREETER 127.0.0.1:98 AUTHENTICATION-COMPLETE USERNAME=have-password1 AUTHENTICATED=TRUE
+#?*GREETER 127.0.0.1:98 START-SESSION
+#?GREETER 127.0.0.1:98 TERMINATE SIGNAL=15
+
+# Session starts
+#?SESSION 127.0.0.1:98 START USER=have-password1
+#?XSERVER :98 TCP-ACCEPT-CONNECT
+#?SESSION 127.0.0.1:98 CONNECT-XSERVER
+
+# Check file descriptors
+#?*SESSION 127.0.0.1:98 LIST-UNKNOWN-FILE-DESCRIPTORS
+#?SESSION 127.0.0.1:98 LIST-UNKNOWN-FILE-DESCRIPTORS FDS=
+
+# Clean up
+#?*STOP-DAEMON
+#?SESSION 127.0.0.1:98 TERMINATE SIGNAL=15
+#?RUNNER DAEMON-EXIT STATUS=0
diff --git a/tests/src/libsystem.c b/tests/src/libsystem.c
index d964e82f..1ebfb082 100644
--- a/tests/src/libsystem.c
+++ b/tests/src/libsystem.c
@@ -4,6 +4,7 @@
 #include <pwd.h>
 #include <security/pam_appl.h>
 #include <unistd.h>
+#include <fcntl.h>
 #define __USE_GNU
 #include <dlfcn.h>
 #ifdef __linux__
@@ -59,19 +60,31 @@ setuid (uid_t uid)
 
 #ifdef __linux__
 int
-open (const char *pathname, int flags, mode_t mode)
+open (const char *pathname, int flags, ...)
 {
     int (*_open) (const char * pathname, int flags, mode_t mode);
+    int mode = 0;
+  
+    if (flags & O_CREAT)
+    {
+        va_list ap;
+        va_start (ap, flags);
+        mode = va_arg (ap, int);
+        va_end (ap);
+    }
 
     _open = (int (*)(const char * pathname, int flags, mode_t mode)) dlsym (RTLD_NEXT, "open");      
     if (strcmp (pathname, "/dev/console") == 0)
     {
         if (console_fd < 0)
-            console_fd = _open ("/dev/null", 0, 0);
+        {
+            console_fd = _open ("/dev/null", flags, mode);
+            fcntl (console_fd, F_SETFD, FD_CLOEXEC);
+        }
         return console_fd;
     }
     else
-        return _open(pathname, flags, mode);
+        return _open (pathname, flags, mode);
 }
 
 int
diff --git a/tests/src/test-session.c b/tests/src/test-session.c
index fcb68954..8df53d45 100644
--- a/tests/src/test-session.c
+++ b/tests/src/test-session.c
@@ -4,6 +4,7 @@
 #include <signal.h>
 #include <sys/types.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include <xcb/xcb.h>
 #include <glib.h>
 #include <glib-object.h>
@@ -11,8 +12,12 @@
 
 #include "status.h"
 
+static GString *open_fds;
+
 static GKeyFile *config;
 
+static xcb_connection_t *connection;
+
 static void
 quit_cb (int signum)
 {
@@ -103,13 +108,28 @@ request_cb (const gchar *request)
         g_clear_error (&error);
     }
     g_free (r);
+
+    r = g_strdup_printf ("SESSION %s LIST-UNKNOWN-FILE-DESCRIPTORS", getenv ("DISPLAY"));
+    if (strcmp (request, r) == 0)
+        status_notify ("SESSION %s LIST-UNKNOWN-FILE-DESCRIPTORS FDS=%s", getenv ("DISPLAY"), open_fds->str);
+    g_free (r);
 }
 
 int
 main (int argc, char **argv)
 {
     GMainLoop *loop;
-    xcb_connection_t *connection;
+    int fd, open_max;
+
+    open_fds = g_string_new ("");
+    open_max = sysconf (_SC_OPEN_MAX);
+    for (fd = STDERR_FILENO + 1; fd < open_max; fd++)
+    {
+        if (fcntl (fd, F_GETFD) >= 0)
+            g_string_append_printf (open_fds, "%d,", fd);
+    }
+    if (g_str_has_suffix (open_fds->str, ","))
+        open_fds->str[strlen (open_fds->str) - 1] = '\0';
 
     signal (SIGINT, quit_cb);
     signal (SIGTERM, quit_cb);
diff --git a/tests/test-open-file-descriptors b/tests/test-open-file-descriptors
new file mode 100755
index 00000000..c067f1d6
--- /dev/null
+++ b/tests/test-open-file-descriptors
@@ -0,0 +1,2 @@
+#!/bin/sh
+./src/dbus-env ./src/test-runner open-file-descriptors test-gobject-greeter
diff --git a/tests/test-vnc-open-file-descriptors b/tests/test-vnc-open-file-descriptors
new file mode 100755
index 00000000..ada6a3bd
--- /dev/null
+++ b/tests/test-vnc-open-file-descriptors
@@ -0,0 +1,2 @@
+#!/bin/sh
+./src/dbus-env ./src/test-runner vnc-open-file-descriptors test-gobject-greeter
diff --git a/tests/test-xdmcp-open-file-descriptors b/tests/test-xdmcp-open-file-descriptors
new file mode 100755
index 00000000..d8ddec32
--- /dev/null
+++ b/tests/test-xdmcp-open-file-descriptors
@@ -0,0 +1,2 @@
+#!/bin/sh
+./src/dbus-env ./src/test-runner xdmcp-open-file-descriptors test-gobject-greeter