Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to drop privileges when launching X server #127

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common/configuration.c
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ config_init (Configuration *config)

g_hash_table_insert (config->priv->vnc_keys, "enabled", GINT_TO_POINTER (KEY_SUPPORTED));
g_hash_table_insert (config->priv->vnc_keys, "command", GINT_TO_POINTER (KEY_SUPPORTED));
g_hash_table_insert (config->priv->vnc_keys, "user", GINT_TO_POINTER (KEY_SUPPORTED));
g_hash_table_insert (config->priv->vnc_keys, "port", GINT_TO_POINTER (KEY_SUPPORTED));
g_hash_table_insert (config->priv->vnc_keys, "listen-address", GINT_TO_POINTER (KEY_SUPPORTED));
g_hash_table_insert (config->priv->vnc_keys, "width", GINT_TO_POINTER (KEY_SUPPORTED));
Expand Down
1 change: 1 addition & 0 deletions data/lightdm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
[VNCServer]
#enabled=false
#command=Xvnc
#user=
#port=5900
#listen-address=
#width=1024
Expand Down
51 changes: 51 additions & 0 deletions src/process.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* license.
*/

#define _GNU_SOURCE /* Required to set saved under uid and gid */
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
Expand All @@ -21,6 +22,7 @@

#include "log-file.h"
#include "process.h"
#include "accounts.h"

enum {
GOT_DATA,
Expand All @@ -44,6 +46,9 @@ typedef struct
/* Command to run */
gchar *command;

/* Optional user to drop privileges (switch) to */
User *user;

/* TRUE to clear the environment in this process */
gboolean clear_environment;

Expand Down Expand Up @@ -166,6 +171,17 @@ process_get_command (Process *process)
return priv->command;
}

void
process_set_user (Process *process, User *user)
{
ProcessPrivate *priv = process_get_instance_private (process);
g_return_if_fail (process != NULL);
g_return_if_fail (user != NULL);

g_clear_object (&priv->user);
priv->user = g_object_ref (user);
}

static void
process_watch_cb (GPid pid, gint status, gpointer data)
{
Expand Down Expand Up @@ -253,6 +269,40 @@ process_start (Process *process, gboolean block)
/* Reset SIGPIPE handler so the child has default behaviour (we disabled it at LightDM start) */
signal (SIGPIPE, SIG_DFL);

/* Drop privileges (checks duplicated from OpenSSH code) */
if (priv->user != NULL && getuid () == 0)
{
gid_t gid_new = user_get_gid (priv->user);
gid_t uid_new = user_get_uid (priv->user);
gid_t gid_old = getgid ();
uid_t uid_old = getuid ();

/* Switch gid and uid (gid has to be first) */
if (setresgid (gid_new, gid_new, gid_new) == -1)
g_error ("Switching user: setresgid %u failed: %s", (u_int)gid_new, strerror (errno));

if (setresuid(uid_new, uid_new, uid_new) == -1)
g_error ("Switching user: setresuid %u failed: %s", (u_int)uid_new, strerror (errno));

/* Verify unable to restore gid */
if (gid_old != gid_new && uid_new != 0)
if (setgid (gid_old) != -1 || setegid (gid_old) != -1)
g_error ("Switched user: able to restore old [e]gid");

/* Verify gid drop was successful */
if (getgid () != gid_new || getegid () != gid_new)
g_error ("Switched user: incorrect gid:%u egid:%u (should be %u)", (u_int)getgid (), (u_int)getegid (), (u_int)gid_new);

/* Verify unable to restore uid */
if (uid_old != uid_new)
if (setuid (uid_old) != -1 || seteuid (uid_old) != -1)
g_error ("Switched user: able to restore old [e]uid");

/* Verify uid drop was successful */
if (getuid () != uid_new || geteuid () != uid_new)
g_error ("Switched user: incorrect uid:%u euid:%u (should be %u)", (u_int)getuid (), (u_int)geteuid (), (u_int)uid_new);
}

execvp (argv[0], argv);
_exit (EXIT_FAILURE);
}
Expand Down Expand Up @@ -382,6 +432,7 @@ process_finalize (GObject *object)

g_clear_pointer (&priv->log_file, g_free);
g_clear_pointer (&priv->command, g_free);
g_clear_object (&priv->user);
g_hash_table_unref (priv->env);
if (priv->quit_timeout)
g_source_remove (priv->quit_timeout);
Expand Down
3 changes: 3 additions & 0 deletions src/process.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <glib-object.h>

#include "log-file.h"
#include "accounts.h"

G_BEGIN_DECLS

Expand Down Expand Up @@ -64,6 +65,8 @@ void process_set_command (Process *process, const gchar *command);

const gchar *process_get_command (Process *process);

void process_set_user (Process *process, User *user);

gboolean process_start (Process *process, gboolean block);

gboolean process_get_is_running (Process *process);
Expand Down
11 changes: 11 additions & 0 deletions src/seat-xvnc.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "seat-xvnc.h"
#include "x-server-xvnc.h"
#include "configuration.h"
#include "accounts.h"

typedef struct
{
Expand Down Expand Up @@ -66,6 +67,16 @@ seat_xvnc_create_display_server (Seat *seat, Session *session)
if (command)
x_server_local_set_command (X_SERVER_LOCAL (x_server), command);

const gchar *username = config_get_string (config_get_instance (), "VNCServer", "user");
if (username)
{
g_autoptr(User) user = accounts_get_user_by_name (username);
if (user)
x_server_local_set_user (X_SERVER_LOCAL (x_server), user);
else
l_warning(seat, "Unable to lookup records for user %s (will default to running user)", username);
}

if (config_has_key (config_get_instance (), "VNCServer", "width") &&
config_has_key (config_get_instance (), "VNCServer", "height"))
{
Expand Down
87 changes: 79 additions & 8 deletions src/x-server-local.c
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include "x-server-local.h"
#include "configuration.h"
#include "accounts.h"
#include "process.h"
#include "vt.h"

Expand All @@ -30,6 +31,9 @@ typedef struct
/* Command to run the X server */
gchar *command;

/* Optional user to drop privileges (switch) to */
User *user;

/* Display number to use */
guint display_number;

Expand Down Expand Up @@ -60,6 +64,9 @@ typedef struct
/* TRUE when received ready signal */
gboolean got_signal;

/* Poll source ID (fallback for signal if uids differ) */
guint poll_for_socket_source;

/* VT to run on */
gint vt;
gboolean have_vt_ref;
Expand Down Expand Up @@ -194,6 +201,16 @@ x_server_local_set_command (XServerLocal *server, const gchar *command)
priv->command = g_strdup (command);
}

void
x_server_local_set_user(XServerLocal *server, User *user)
{
XServerLocalPrivate *priv = x_server_local_get_instance_private (server);
g_return_if_fail (server != NULL);
g_return_if_fail (user != NULL);
g_clear_object (&priv->user);
priv->user = g_object_ref(user);
}

void
x_server_local_set_vt (XServerLocal *server, gint vt)
{
Expand Down Expand Up @@ -377,6 +394,46 @@ got_signal_cb (Process *process, int signum, XServerLocal *server)
}
}

static gboolean
poll_for_socket_cb (XServerLocal *server)
{
XServerLocalPrivate *priv = x_server_local_get_instance_private (server);

/* Check is X11 socket file exists as an alternative startup test to SIGUSR1 */
GStatBuf statbuf;
g_autofree gchar *socketpath = g_strdup_printf ("/tmp/.X11-unix/X%d", priv->display_number);
if ( g_stat (socketpath, &statbuf) == 0 )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace here doesn't match existing code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you mean blank like 405. I've deleted it. Please let me know if this wasn't it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it should change from if ( g_stat (socketpath, &statbuf) == 0 ) to if (g_stat (socketpath, &statbuf) == 0)

{
uid_t uid = priv->user ? user_get_uid(priv->user) : 0;

/* It has to be a valid socket file */
if (!(statbuf.st_mode & S_IFSOCK))
robert-ancell marked this conversation as resolved.
Show resolved Hide resolved
{
l_debug (server, "X11 socket file is not a socket: %s", socketpath);
return G_SOURCE_REMOVE;
}

/* It has to be owned by the correct user */
if (statbuf.st_uid != uid)
{
l_debug (server, "X11 socket file is not owned by uid %d: %s", uid, socketpath);
return G_SOURCE_REMOVE;
}

/* Consider SIGUSR1 to have been recieved */
priv->got_signal = TRUE;
l_debug (server, "Detected valid X11 socket for X server :%d", priv->display_number);

// FIXME: Check return value
DISPLAY_SERVER_CLASS (x_server_local_parent_class)->start (DISPLAY_SERVER (server));

return G_SOURCE_REMOVE;
}

/* Wait another second and check again */
return G_SOURCE_CONTINUE;
}

static void
stopped_cb (Process *process, XServerLocal *server)
{
Expand Down Expand Up @@ -411,19 +468,19 @@ write_authority_file (XServerLocal *server)
XServerLocalPrivate *priv = x_server_local_get_instance_private (server);

XAuthority *authority = x_server_get_authority (X_SERVER (server));
if (!authority)
return;
g_return_if_fail (authority != NULL);

/* Get file to write to if have authority */
if (!priv->authority_file)
if (priv->authority_file == NULL)
{
g_autofree gchar *run_dir = NULL;
g_autofree gchar *dir = NULL;
g_autofree gchar *run_dir = config_get_string (config_get_instance (), "LightDM", "run-directory");
g_autofree gchar *dir = g_build_filename (run_dir, priv->user ? user_get_name (priv->user) : "root", NULL);

run_dir = config_get_string (config_get_instance (), "LightDM", "run-directory");
dir = g_build_filename (run_dir, "root", NULL);
if (g_mkdir_with_parents (dir, S_IRWXU) < 0)
l_warning (server, "Failed to make authority directory %s: %s", dir, strerror (errno));
if (priv->user != NULL && getuid () == 0)
if (chown (dir, user_get_uid (priv->user), user_get_gid (priv->user)) < 0)
l_warning (server, "Failed to set ownership of x-server authority dir: %s", strerror (errno));

priv->authority_file = g_build_filename (dir, x_server_get_address (X_SERVER (server)), NULL);
}
Expand All @@ -434,6 +491,9 @@ write_authority_file (XServerLocal *server)
x_authority_write (authority, XAUTH_WRITE_MODE_REPLACE, priv->authority_file, &error);
if (error)
l_warning (server, "Failed to write authority: %s", error->message);
if (priv->user != NULL && getuid () == 0)
if (chown (priv->authority_file, user_get_uid (priv->user), user_get_gid (priv->user)) < 0)
l_warning (server, "Failed to set ownership of authority: %s", strerror (errno));
}

static gboolean
Expand All @@ -451,7 +511,10 @@ x_server_local_start (DisplayServer *display_server)
ProcessRunFunc run_cb = X_SERVER_LOCAL_GET_CLASS (server)->get_run_function (server);
priv->x_server_process = process_new (run_cb, server);
process_set_clear_environment (priv->x_server_process, TRUE);
g_signal_connect (priv->x_server_process, PROCESS_SIGNAL_GOT_SIGNAL, G_CALLBACK (got_signal_cb), server);
if (priv->user == NULL || user_get_uid (priv->user) == getuid ())
g_signal_connect (priv->x_server_process, PROCESS_SIGNAL_GOT_SIGNAL, G_CALLBACK (got_signal_cb), server);
else if (!priv->poll_for_socket_source)
priv->poll_for_socket_source = g_timeout_add_seconds (1, (GSourceFunc)poll_for_socket_cb, server);
g_signal_connect (priv->x_server_process, PROCESS_SIGNAL_STOPPED, G_CALLBACK (stopped_cb), server);

/* Setup logging */
Expand Down Expand Up @@ -514,6 +577,8 @@ x_server_local_start (DisplayServer *display_server)
X_SERVER_LOCAL_GET_CLASS (server)->add_args (server, command);

process_set_command (priv->x_server_process, command->str);
if (priv->user)
process_set_user (priv->x_server_process, priv->user);

l_debug (display_server, "Launching X Server");

Expand Down Expand Up @@ -575,8 +640,14 @@ x_server_local_finalize (GObject *object)

if (priv->x_server_process)
g_signal_handlers_disconnect_matched (priv->x_server_process, G_SIGNAL_MATCH_DATA, 0, 0, NULL, NULL, self);
if (priv->poll_for_socket_source)
{
g_source_remove (priv->poll_for_socket_source);
priv->poll_for_socket_source = 0;
}
g_clear_object (&priv->x_server_process);
g_clear_pointer (&priv->command, g_free);
g_clear_object (&priv->user);
g_clear_pointer (&priv->config_file, g_free);
g_clear_pointer (&priv->layout, g_free);
g_clear_pointer (&priv->xdg_seat, g_free);
Expand Down
3 changes: 3 additions & 0 deletions src/x-server-local.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include "x-server.h"
#include "process.h"
#include "accounts.h"

G_BEGIN_DECLS

Expand Down Expand Up @@ -53,6 +54,8 @@ XServerLocal *x_server_local_new (void);

void x_server_local_set_command (XServerLocal *server, const gchar *command);

void x_server_local_set_user(XServerLocal *server, User *user);

void x_server_local_set_vt (XServerLocal *server, gint vt);

void x_server_local_set_config (XServerLocal *server, const gchar *path);
Expand Down