From 17a56567d21edad420ad179f850a061de7bfe986 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Mon, 26 Feb 2024 15:45:04 +0000
Subject: [PATCH 1/7] Add params to allow xrdp to be run as non-root

runtime_user and runtime_group are added to the xrdp.ini file
so that the service knows how to reduce privilege
---
 docs/man/sesman.ini.5.in |  3 +-
 docs/man/xrdp.ini.5.in   | 11 +++++
 sesman/sesman.ini.in     |  8 ++--
 xrdp/xrdp.c              |  2 -
 xrdp/xrdp.ini.in         |  9 ++++
 xrdp/xrdp_listen.c       | 94 ++++++++++++++++++++++------------------
 xrdp/xrdp_types.h        |  4 ++
 7 files changed, 83 insertions(+), 48 deletions(-)

diff --git a/docs/man/sesman.ini.5.in b/docs/man/sesman.ini.5.in
index d2ca98324b..3495c779f2 100644
--- a/docs/man/sesman.ini.5.in
+++ b/docs/man/sesman.ini.5.in
@@ -315,7 +315,8 @@ transitions between confinement domains.
 .TP
 \fBSessionSockdirGroup\fR=\fIgroup\fR
 Sets the group owner of the directories containing session sockets. This
-is normally the GID of the xrdp process so xrdp can connect to user sessions.
+MUST be the same as runtime_group in xrdp.ini, or xrdp will not
+be able to connect to any sessions.
 
 .SH "X11 SERVER"
 Following parameters can be used in the \fB[Xvnc]\fR and
diff --git a/docs/man/xrdp.ini.5.in b/docs/man/xrdp.ini.5.in
index e44327ce59..c7a228411e 100644
--- a/docs/man/xrdp.ini.5.in
+++ b/docs/man/xrdp.ini.5.in
@@ -119,6 +119,17 @@ The default port for RDP is \fB3389\fP.
 Multiple address:port instances must be separated by spaces or commas. Check the .ini file for examples.
 Specifying interfaces requires said interfaces to be UP before xrdp starts.
 
+.TP
+\fBruntime_user\fP=\fIusername\fP
+.TP
+\fBruntime_group\fP=\fIgroupname\fP
+User name and group to run the xrdp daemon under.
+
+After xrdp starts, it sets its UID and GID to values derived from these
+settings, so that it's running without system privilege.
+The \fBruntime_group\fP MUST be set to the same value as
+\fBSessionSockdirGroup\fP in \fBsesman.ini\fP if you want to run sessions.
+
 .TP
 \fBenable_token_login\fP=\fI[true|false]\fP
 If set to \fB1\fP, \fBtrue\fP or \fByes\fP, \fBxrdp\fP will scan the user name provided by the
diff --git a/sesman/sesman.ini.in b/sesman/sesman.ini.in
index 685af28b7e..62eae2761b 100644
--- a/sesman/sesman.ini.in
+++ b/sesman/sesman.ini.in
@@ -45,10 +45,10 @@ RestrictInboundClipboard=none
 ; Leave this unset unless you need to disable it.
 #XorgNoNewPrivileges=true
 ; Specify the group which is to have read access to the directory where
-; local sockets for the session are created. This is normally the GID
-; which the xrdp process runs as.
-; Default is 'root'
-#SessionSockdirGroup=root
+; local sockets for the session are created.
+; This MUST be the same as runtime_group in xrdp.ini, or xrdp will not
+; be able to connect to your sessions.
+#SessionSockdirGroup=xrdp
 
 
 [Sessions]
diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c
index b2f9492002..08377b23f7 100644
--- a/xrdp/xrdp.c
+++ b/xrdp/xrdp.c
@@ -428,8 +428,6 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
-
-
     if (g_file_exist(pid_file)) /* xrdp.pid */
     {
         LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running.");
diff --git a/xrdp/xrdp.ini.in b/xrdp/xrdp.ini.in
index 11ecada6f8..d2998a3c3a 100644
--- a/xrdp/xrdp.ini.in
+++ b/xrdp/xrdp.ini.in
@@ -27,6 +27,15 @@ port=3389
 ; prefer use vsock://<cid>:<port> above
 use_vsock=false
 
+; Unprivileged User name and group to run the xrdp daemon.
+; It is HIGHLY RECOMMENDED you set these values.
+; A suitable user and group can be added with a command like this (Linux):-
+;     useradd xrdp -d / -c 'xrdp daemon' -s /usr/sbin/nologin
+; Be aware that runtime_group here, and SessionSockdirGroup in sesman.ini
+; MUST be the same if you want to run sessions.
+#runtime_user=xrdp
+#runtime_group=xrdp
+
 ; regulate if the listening socket use socket option tcp_nodelay
 ; no buffering will be performed in the TCP stack
 tcp_nodelay=true
diff --git a/xrdp/xrdp_listen.c b/xrdp/xrdp_listen.c
index 7981e7cc29..618b96edaf 100644
--- a/xrdp/xrdp_listen.c
+++ b/xrdp/xrdp_listen.c
@@ -162,7 +162,8 @@ xrdp_listen_get_startup_params(struct xrdp_listen *self)
     int index;
     int port_override;
     int fork_override;
-    char *val;
+    const char *name;
+    const char *val;
     struct list *names;
     struct list *values;
     struct xrdp_startup_params *startup_params;
@@ -181,56 +182,67 @@ xrdp_listen_get_startup_params(struct xrdp_listen *self)
         {
             for (index = 0; index < names->count; index++)
             {
-                val = (char *)list_get_item(names, index);
-                if (val != 0)
+                name = (const char *)list_get_item(names, index);
+                val = (const char *)list_get_item(values, index);
+                if (name == 0 || val == 0)
                 {
-                    if (g_strcasecmp(val, "port") == 0)
-                    {
-                        if (port_override == 0)
-                        {
-                            val = (char *) list_get_item(values, index);
-                            g_strncpy(startup_params->port, val,
-                                      sizeof(startup_params->port) - 1);
-                        }
-                    }
-                    if (g_strcasecmp(val, "fork") == 0)
-                    {
-                        if (fork_override == 0)
-                        {
-                            val = (char *) list_get_item(values, index);
-                            startup_params->fork = g_text2bool(val);
-                        }
-                    }
+                    continue;
+                }
 
-                    if (g_strcasecmp(val, "tcp_nodelay") == 0)
+                if (g_strcasecmp(name, "port") == 0)
+                {
+                    if (port_override == 0)
                     {
-                        val = (char *)list_get_item(values, index);
-                        startup_params->tcp_nodelay = g_text2bool(val);
+                        g_strncpy(startup_params->port, val,
+                                  sizeof(startup_params->port) - 1);
                     }
+                }
 
-                    if (g_strcasecmp(val, "tcp_keepalive") == 0)
+                else if (g_strcasecmp(name, "fork") == 0)
+                {
+                    if (fork_override == 0)
                     {
-                        val = (char *)list_get_item(values, index);
-                        startup_params->tcp_keepalive = g_text2bool(val);
+                        startup_params->fork = g_text2bool(val);
                     }
+                }
 
-                    if (g_strcasecmp(val, "tcp_send_buffer_bytes") == 0)
-                    {
-                        val = (char *)list_get_item(values, index);
-                        startup_params->tcp_send_buffer_bytes = g_atoi(val);
-                    }
+                else if (g_strcasecmp(name, "tcp_nodelay") == 0)
+                {
+                    startup_params->tcp_nodelay = g_text2bool(val);
+                }
 
-                    if (g_strcasecmp(val, "tcp_recv_buffer_bytes") == 0)
-                    {
-                        val = (char *)list_get_item(values, index);
-                        startup_params->tcp_recv_buffer_bytes = g_atoi(val);
-                    }
+                else if (g_strcasecmp(name, "tcp_keepalive") == 0)
+                {
+                    startup_params->tcp_keepalive = g_text2bool(val);
+                }
 
-                    if (g_strcasecmp(val, "use_vsock") == 0)
-                    {
-                        val = (char *)list_get_item(values, index);
-                        startup_params->use_vsock = g_text2bool(val);
-                    }
+                else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0)
+                {
+                    startup_params->tcp_send_buffer_bytes = g_atoi(val);
+                }
+
+                else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0)
+                {
+                    startup_params->tcp_recv_buffer_bytes = g_atoi(val);
+                }
+
+                else if (g_strcasecmp(name, "use_vsock") == 0)
+                {
+                    startup_params->use_vsock = g_text2bool(val);
+                }
+
+                else if (g_strcasecmp(name, "runtime_user") == 0)
+                {
+                    g_snprintf(startup_params->runtime_user,
+                               sizeof(startup_params->runtime_user),
+                               "%s", val);
+                }
+
+                else if (g_strcasecmp(name, "runtime_group") == 0)
+                {
+                    g_snprintf(startup_params->runtime_group,
+                               sizeof(startup_params->runtime_group),
+                               "%s", val);
                 }
             }
         }
diff --git a/xrdp/xrdp_types.h b/xrdp/xrdp_types.h
index de9849e458..6ffa0fce89 100644
--- a/xrdp/xrdp_types.h
+++ b/xrdp/xrdp_types.h
@@ -750,6 +750,10 @@ struct xrdp_startup_params
     int tcp_nodelay;
     int tcp_keepalive;
     int use_vsock;
+    // These should be local users/groups, and so we shouldn't need
+    // a lot of storage for them.
+    char runtime_user[64];
+    char runtime_group[64];
 };
 
 /*

From ddff9ebb322f3a6f3b0b6215e626dd6018b18235 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Tue, 27 Feb 2024 11:53:34 +0000
Subject: [PATCH 2/7] Refactor xrdp_listen to allow for privilege drop

- xrdp_listen.c is refactored so we can create the
  listening socket(s) before dropping privileges.
- The code which reads startup params from xrdp.ini
  is moved from xrdp_listen.c to xrdp.c, so it
  is only called once if we test the listen before
  starting the daemon.
---
 common/thread_calls.c |   7 +-
 xrdp/xrdp.c           | 206 +++++++++++++++++++++++++++++++++++-------
 xrdp/xrdp.h           |   7 +-
 xrdp/xrdp_listen.c    | 147 ++----------------------------
 4 files changed, 190 insertions(+), 177 deletions(-)

diff --git a/common/thread_calls.c b/common/thread_calls.c
index 0b821b8fd3..b575d4eade 100644
--- a/common/thread_calls.c
+++ b/common/thread_calls.c
@@ -122,8 +122,11 @@ tc_mutex_delete(tbus mutex)
     pthread_mutex_t *lmutex;
 
     lmutex = (pthread_mutex_t *)mutex;
-    pthread_mutex_destroy(lmutex);
-    g_free(lmutex);
+    if (lmutex != NULL)
+    {
+        pthread_mutex_destroy(lmutex);
+        g_free(lmutex);
+    }
 #endif
 }
 
diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c
index 08377b23f7..9c48381936 100644
--- a/xrdp/xrdp.c
+++ b/xrdp/xrdp.c
@@ -240,6 +240,127 @@ xrdp_process_params(int argc, char **argv,
     return 0;
 }
 
+/*****************************************************************************/
+/**
+ *
+ * @brief  Read additional startup parameters from xrdp.ini
+ *
+ * @param  [in,out] startup parameters from the command line
+ * @return 0 on success
+ *
+ */
+static int
+read_xrdp_ini_startup_params(struct xrdp_startup_params *startup_params)
+{
+    int rv = 0;
+    int fd;
+    int index;
+    int port_override;
+    int fork_override;
+    const char *name;
+    const char *val;
+    struct list *names;
+    struct list *values;
+
+    port_override = startup_params->port[0] != 0;
+    fork_override = startup_params->fork;
+    names = list_create();
+    names->auto_free = 1;
+    values = list_create();
+    values->auto_free = 1;
+
+    fd = g_file_open_ro(startup_params->xrdp_ini);
+    if (fd < 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Can't open %s [%s]", startup_params->xrdp_ini,
+            g_get_strerror());
+        rv = 1;
+    }
+    else if (file_read_section(fd, "globals", names, values) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Can't read [Globals] from %s",
+            startup_params->xrdp_ini);
+        rv = 1;
+    }
+    else
+    {
+        for (index = 0; index < names->count; index++)
+        {
+            name = (const char *)list_get_item(names, index);
+            val = (const char *)list_get_item(values, index);
+            if (name == 0 || val == 0)
+            {
+                continue;
+            }
+
+            if (g_strcasecmp(name, "port") == 0)
+            {
+                if (port_override == 0)
+                {
+                    g_strncpy(startup_params->port, val,
+                              sizeof(startup_params->port) - 1);
+                }
+            }
+
+            else if (g_strcasecmp(name, "fork") == 0)
+            {
+                if (fork_override == 0)
+                {
+                    startup_params->fork = g_text2bool(val);
+                }
+            }
+
+            else if (g_strcasecmp(name, "tcp_nodelay") == 0)
+            {
+                startup_params->tcp_nodelay = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_keepalive") == 0)
+            {
+                startup_params->tcp_keepalive = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0)
+            {
+                startup_params->tcp_send_buffer_bytes = g_atoi(val);
+            }
+
+            else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0)
+            {
+                startup_params->tcp_recv_buffer_bytes = g_atoi(val);
+            }
+
+            else if (g_strcasecmp(name, "use_vsock") == 0)
+            {
+                startup_params->use_vsock = g_text2bool(val);
+            }
+
+            else if (g_strcasecmp(name, "runtime_user") == 0)
+            {
+                g_snprintf(startup_params->runtime_user,
+                           sizeof(startup_params->runtime_user),
+                           "%s", val);
+            }
+
+            else if (g_strcasecmp(name, "runtime_group") == 0)
+            {
+                g_snprintf(startup_params->runtime_group,
+                           sizeof(startup_params->runtime_group),
+                           "%s", val);
+            }
+        }
+    }
+
+    list_delete(names);
+    list_delete(values);
+    if (fd >= 0)
+    {
+        g_file_close(fd);
+    }
+    return rv;
+}
+
+
 /*****************************************************************************/
 /* Basic sanity checks before any forking */
 static int
@@ -299,7 +420,7 @@ xrdp_sanity_check(void)
 int
 main(int argc, char **argv)
 {
-    int exit_status = 0;
+    int exit_status = 1;
     enum logReturns error;
     struct xrdp_startup_params startup_params = {0};
     int pid;
@@ -428,6 +549,13 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
+    if (!read_xrdp_ini_startup_params(&startup_params))
+    {
+        log_end();
+        g_deinit();
+        g_exit(1);
+    }
+
     if (g_file_exist(pid_file)) /* xrdp.pid */
     {
         LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running.");
@@ -473,8 +601,14 @@ main(int argc, char **argv)
 
     if (daemon)
     {
-        /* if can't listen, exit with failure status */
-        if (xrdp_listen_test(&startup_params) != 0)
+        /* Before daemonising, check we can listen.
+         * If we can't listen, exit with failure status */
+        struct xrdp_listen *xrdp_listen;
+        xrdp_listen = xrdp_listen_create(&startup_params);
+        int status = xrdp_listen_init(xrdp_listen);
+        xrdp_listen_delete(xrdp_listen);
+
+        if (status != 0)
         {
             LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, "
                 "possibly address already in use.");
@@ -540,43 +674,51 @@ main(int argc, char **argv)
         /* end of daemonizing code */
     }
 
-    g_set_threadid(tc_get_threadid());
-    g_listen = xrdp_listen_create();
-    g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */
-    g_signal_pipe(xrdp_sig_no_op);          /* SIGPIPE */
-    g_signal_terminate(xrdp_shutdown);      /* SIGTERM */
-    g_signal_child_stop(xrdp_child);        /* SIGCHLD */
-    g_signal_hang_up(xrdp_sig_no_op);       /* SIGHUP */
-    g_set_sync_mutex(tc_mutex_create());
-    g_set_sync1_mutex(tc_mutex_create());
-    pid = g_getpid();
-    LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid);
-    g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid);
-    g_set_term_event(g_create_wait_obj(text));
-
-    if (g_get_term() == 0)
+    g_listen = xrdp_listen_create(&startup_params);
+    if (xrdp_listen_init(g_listen) != 0)
     {
-        LOG(LOG_LEVEL_WARNING, "error creating g_term_event");
+        LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, "
+            "possibly address already in use.");
     }
+    else
+    {
+        g_set_threadid(tc_get_threadid());
+        g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */
+        g_signal_pipe(xrdp_sig_no_op);          /* SIGPIPE */
+        g_signal_terminate(xrdp_shutdown);      /* SIGTERM */
+        g_signal_child_stop(xrdp_child);        /* SIGCHLD */
+        g_signal_hang_up(xrdp_sig_no_op);       /* SIGHUP */
+        g_set_sync_mutex(tc_mutex_create());
+        g_set_sync1_mutex(tc_mutex_create());
+        pid = g_getpid();
+        LOG(LOG_LEVEL_INFO, "starting xrdp with pid %d", pid);
+        g_snprintf(text, 255, "xrdp_%8.8x_main_term", pid);
+        g_set_term_event(g_create_wait_obj(text));
+
+        if (g_get_term() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_term_event");
+        }
 
-    g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid);
-    g_set_sigchld_event(g_create_wait_obj(text));
+        g_snprintf(text, 255, "xrdp_%8.8x_main_sigchld", pid);
+        g_set_sigchld_event(g_create_wait_obj(text));
 
-    if (g_get_sigchld() == 0)
-    {
-        LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event");
-    }
+        if (g_get_sigchld() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_sigchld_event");
+        }
 
-    g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid);
-    g_set_sync_event(g_create_wait_obj(text));
+        g_snprintf(text, 255, "xrdp_%8.8x_main_sync", pid);
+        g_set_sync_event(g_create_wait_obj(text));
 
-    if (g_get_sync_event() == 0)
-    {
-        LOG(LOG_LEVEL_WARNING, "error creating g_sync_event");
+        if (g_get_sync_event() == 0)
+        {
+            LOG(LOG_LEVEL_WARNING, "error creating g_sync_event");
+        }
+
+        exit_status = xrdp_listen_main_loop(g_listen);
     }
 
-    g_listen->startup_params = &startup_params;
-    exit_status = xrdp_listen_main_loop(g_listen);
     xrdp_listen_delete(g_listen);
 
     tc_mutex_delete(g_get_sync_mutex());
diff --git a/xrdp/xrdp.h b/xrdp/xrdp.h
index 91a262179c..84e272959f 100644
--- a/xrdp/xrdp.h
+++ b/xrdp/xrdp.h
@@ -194,13 +194,14 @@ xrdp_process_main_loop(struct xrdp_process *self);
 
 /* xrdp_listen.c */
 struct xrdp_listen *
-xrdp_listen_create(void);
+xrdp_listen_create(struct xrdp_startup_params *startup_params);
+int
+xrdp_listen_init(struct xrdp_listen *self);
 void
 xrdp_listen_delete(struct xrdp_listen *self);
 int
 xrdp_listen_main_loop(struct xrdp_listen *self);
-int
-xrdp_listen_test(struct xrdp_startup_params *startup_params);
+
 
 /* xrdp_region.c */
 struct xrdp_region *
diff --git a/xrdp/xrdp_listen.c b/xrdp/xrdp_listen.c
index 618b96edaf..c13d06fb7c 100644
--- a/xrdp/xrdp_listen.c
+++ b/xrdp/xrdp_listen.c
@@ -55,7 +55,7 @@ xrdp_listen_create_pro_done(struct xrdp_listen *self)
 
 /*****************************************************************************/
 struct xrdp_listen *
-xrdp_listen_create(void)
+xrdp_listen_create(struct xrdp_startup_params *startup_params)
 {
     struct xrdp_listen *self;
 
@@ -64,6 +64,7 @@ xrdp_listen_create(void)
     self->trans_list = list_create();
     self->process_list = list_create();
     self->fork_list = list_create();
+    self->startup_params = startup_params;
 
     if (g_process_sem == 0)
     {
@@ -153,107 +154,6 @@ xrdp_process_run(void *in_val)
     LOG_DEVEL(LOG_LEVEL_TRACE, "process done");
     return 0;
 }
-
-/*****************************************************************************/
-static int
-xrdp_listen_get_startup_params(struct xrdp_listen *self)
-{
-    int fd;
-    int index;
-    int port_override;
-    int fork_override;
-    const char *name;
-    const char *val;
-    struct list *names;
-    struct list *values;
-    struct xrdp_startup_params *startup_params;
-
-    startup_params = self->startup_params;
-    port_override = startup_params->port[0] != 0;
-    fork_override = startup_params->fork;
-    fd = g_file_open_ro(startup_params->xrdp_ini);
-    if (fd != -1)
-    {
-        names = list_create();
-        names->auto_free = 1;
-        values = list_create();
-        values->auto_free = 1;
-        if (file_read_section(fd, "globals", names, values) == 0)
-        {
-            for (index = 0; index < names->count; index++)
-            {
-                name = (const char *)list_get_item(names, index);
-                val = (const char *)list_get_item(values, index);
-                if (name == 0 || val == 0)
-                {
-                    continue;
-                }
-
-                if (g_strcasecmp(name, "port") == 0)
-                {
-                    if (port_override == 0)
-                    {
-                        g_strncpy(startup_params->port, val,
-                                  sizeof(startup_params->port) - 1);
-                    }
-                }
-
-                else if (g_strcasecmp(name, "fork") == 0)
-                {
-                    if (fork_override == 0)
-                    {
-                        startup_params->fork = g_text2bool(val);
-                    }
-                }
-
-                else if (g_strcasecmp(name, "tcp_nodelay") == 0)
-                {
-                    startup_params->tcp_nodelay = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_keepalive") == 0)
-                {
-                    startup_params->tcp_keepalive = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_send_buffer_bytes") == 0)
-                {
-                    startup_params->tcp_send_buffer_bytes = g_atoi(val);
-                }
-
-                else if (g_strcasecmp(name, "tcp_recv_buffer_bytes") == 0)
-                {
-                    startup_params->tcp_recv_buffer_bytes = g_atoi(val);
-                }
-
-                else if (g_strcasecmp(name, "use_vsock") == 0)
-                {
-                    startup_params->use_vsock = g_text2bool(val);
-                }
-
-                else if (g_strcasecmp(name, "runtime_user") == 0)
-                {
-                    g_snprintf(startup_params->runtime_user,
-                               sizeof(startup_params->runtime_user),
-                               "%s", val);
-                }
-
-                else if (g_strcasecmp(name, "runtime_group") == 0)
-                {
-                    g_snprintf(startup_params->runtime_group,
-                               sizeof(startup_params->runtime_group),
-                               "%s", val);
-                }
-            }
-        }
-
-        list_delete(names);
-        list_delete(values);
-        g_file_close(fd);
-    }
-    return 0;
-}
-
 /*****************************************************************************/
 static int
 xrdp_listen_stop_all_listen(struct xrdp_listen *self)
@@ -663,8 +563,10 @@ xrdp_listen_pp(struct xrdp_listen *self, int *index,
 }
 
 /*****************************************************************************/
-static int
-xrdp_listen_process_startup_params(struct xrdp_listen *self)
+/* returns 0 if xrdp is listening correctly
+   returns 1 if xrdp is not listening correctly */
+int
+xrdp_listen_init(struct xrdp_listen *self)
 {
     int mode; /* TRANS_MODE_TCP*, TRANS_MODE_UNIX, TRANS_MODE_VSOCK */
     int error;
@@ -898,18 +800,7 @@ xrdp_listen_main_loop(struct xrdp_listen *self)
     struct trans *ltrans;
 
     self->status = 1;
-    if (xrdp_listen_get_startup_params(self) != 0)
-    {
-        LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed");
-        self->status = -1;
-        return 1;
-    }
-    if (xrdp_listen_process_startup_params(self) != 0)
-    {
-        LOG(LOG_LEVEL_ERROR, "xrdp_listen_main_loop: xrdp_listen_get_port failed");
-        self->status = -1;
-        return 1;
-    }
+
     term_obj = g_get_term(); /*Global termination event */
     sigchld_obj = g_get_sigchld();
     sync_obj = g_get_sync_event();
@@ -1049,27 +940,3 @@ xrdp_listen_main_loop(struct xrdp_listen *self)
     self->status = -1;
     return 0;
 }
-
-/*****************************************************************************/
-/* returns 0 if xrdp can listen
-   returns 1 if xrdp cannot listen */
-int
-xrdp_listen_test(struct xrdp_startup_params *startup_params)
-{
-    struct xrdp_listen *xrdp_listen;
-
-    xrdp_listen = xrdp_listen_create();
-    xrdp_listen->startup_params = startup_params;
-    if (xrdp_listen_get_startup_params(xrdp_listen) != 0)
-    {
-        xrdp_listen_delete(xrdp_listen);
-        return 1;
-    }
-    if (xrdp_listen_process_startup_params(xrdp_listen) != 0)
-    {
-        xrdp_listen_delete(xrdp_listen);
-        return 1;
-    }
-    xrdp_listen_delete(xrdp_listen);
-    return 0;
-}

From 2446c206e6e84a5bb4ec0546255f3da5697b42e5 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Wed, 28 Feb 2024 10:06:47 +0000
Subject: [PATCH 3/7] xrdp: PID file handling tidy-ups

Now we have g_file_open_rw() we don't need to try to write to
the PID file to see if we can. Just leave the file open and write to
it after forking.
---
 xrdp/xrdp.c | 46 +++++++++-------------------------------------
 1 file changed, 9 insertions(+), 37 deletions(-)

diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c
index 9c48381936..9646d9e2d8 100644
--- a/xrdp/xrdp.c
+++ b/xrdp/xrdp.c
@@ -424,7 +424,6 @@ main(int argc, char **argv)
     enum logReturns error;
     struct xrdp_startup_params startup_params = {0};
     int pid;
-    int fd;
     int daemon;
     char text[256];
     const char *pid_file = XRDP_PID_PATH "/xrdp.pid";
@@ -491,7 +490,7 @@ main(int argc, char **argv)
     {
         g_writeln("stopping xrdp");
         /* read the xrdp.pid file */
-        fd = -1;
+        int fd = -1;
 
         if (g_file_exist(pid_file)) /* xrdp.pid */
         {
@@ -570,23 +569,13 @@ main(int argc, char **argv)
 
     if (daemon)
     {
-
         /* make sure containing directory exists */
         g_create_path(pid_file);
 
         /* make sure we can write to pid file */
-        fd = g_file_open_rw(pid_file); /* xrdp.pid */
-
-        if (fd == -1)
-        {
-            LOG(LOG_LEVEL_ALWAYS,
-                "running in daemon mode with no access to pid files, quitting");
-            log_end();
-            g_deinit();
-            g_exit(1);
-        }
+        int pid_fd = g_file_open_rw(pid_file); /* xrdp.pid */
 
-        if (g_file_write(fd, "0", 1) == -1)
+        if (pid_fd == -1)
         {
             LOG(LOG_LEVEL_ALWAYS,
                 "running in daemon mode with no access to pid files, quitting");
@@ -595,12 +584,6 @@ main(int argc, char **argv)
             g_exit(1);
         }
 
-        g_file_close(fd);
-        g_file_delete(pid_file);
-    }
-
-    if (daemon)
-    {
         /* Before daemonising, check we can listen.
          * If we can't listen, exit with failure status */
         struct xrdp_listen *xrdp_listen;
@@ -618,6 +601,7 @@ main(int argc, char **argv)
                or systemd cannot detect xrdp daemon couldn't start properly */
             g_exit(1);
         }
+
         /* start of daemonizing code */
         pid = g_fork();
 
@@ -638,21 +622,10 @@ main(int argc, char **argv)
         }
 
         g_sleep(1000);
-        /* write the pid to file */
-        pid = g_getpid();
-        fd = g_file_open_rw(pid_file); /* xrdp.pid */
-
-        if (fd == -1)
-        {
-            LOG(LOG_LEVEL_WARNING, "Can't open %s for writing [%s]",
-                pid_file, g_get_strerror());
-        }
-        else
-        {
-            g_sprintf(text, "%d", pid);
-            g_file_write(fd, text, g_strlen(text));
-            g_file_close(fd);
-        }
+        /* write our pid to file */
+        g_sprintf(text, "%d", g_getpid());
+        g_file_write(pid_fd, text, g_strlen(text));
+        g_file_close(pid_fd);
 
         g_sleep(1000);
         g_file_close(0);
@@ -736,8 +709,7 @@ main(int argc, char **argv)
     g_delete_wait_obj(g_get_sync_event());
     g_set_sync_event(0);
 
-    /* only main process should delete pid file */
-    if (daemon && (pid == g_getpid()))
+    if (daemon)
     {
         /* delete the xrdp.pid file */
         g_file_delete(pid_file);

From b1d842857931243aa363d93ae9d86444c43ef8df Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Wed, 28 Feb 2024 10:35:06 +0000
Subject: [PATCH 4/7] Add code to drop privileges of xrdp daemon

---
 common/os_calls.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 common/os_calls.h |  1 +
 xrdp/xrdp.c       | 36 ++++++++++++++++++++++++++++++++++--
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/common/os_calls.c b/common/os_calls.c
index 6de4360b34..53181f8c9b 100644
--- a/common/os_calls.c
+++ b/common/os_calls.c
@@ -3167,6 +3167,48 @@ g_setgid(int pid)
 #endif
 }
 
+/*****************************************************************************/
+/* Used by daemonizing code */
+/* returns error, zero is success, non zero is error */
+int
+g_drop_privileges(const char *user, const char *group)
+{
+    int rv = 1;
+    int uid;
+    int gid;
+    if (g_getuser_info_by_name(user, &uid, NULL, NULL, NULL, NULL) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to get UID for user '%s' [%s]", user,
+            g_get_strerror());
+    }
+    else if (g_getgroup_info(group, &gid) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to get GID for group '%s' [%s]", group,
+            g_get_strerror());
+    }
+    else if (initgroups(user, gid) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to init groups for '%s' [%s]", user,
+            g_get_strerror());
+    }
+    else if (g_setgid(gid) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to set group to '%s' [%s]", group,
+            g_get_strerror());
+    }
+    else if (g_setuid(uid) != 0)
+    {
+        LOG(LOG_LEVEL_ERROR, "Unable to set user to '%s' [%s]", user,
+            g_get_strerror());
+    }
+    else
+    {
+        rv = 0;
+    }
+
+    return rv;
+}
+
 /*****************************************************************************/
 /* returns error, zero is success, non zero is error */
 /* does not work in win32 */
diff --git a/common/os_calls.h b/common/os_calls.h
index 9fdbad8b1b..99e339b3c2 100644
--- a/common/os_calls.h
+++ b/common/os_calls.h
@@ -338,6 +338,7 @@ void     g_signal_pipe(void (*func)(int));
 void     g_signal_usr1(void (*func)(int));
 int      g_fork(void);
 int      g_setgid(int pid);
+int      g_drop_privileges(const char *user, const char *group);
 int      g_initgroups(const char *user);
 int      g_getuid(void);
 int      g_getgid(void);
diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c
index 9646d9e2d8..33aeb7b76f 100644
--- a/xrdp/xrdp.c
+++ b/xrdp/xrdp.c
@@ -416,6 +416,38 @@ xrdp_sanity_check(void)
     return 0;
 }
 
+/*****************************************************************************/
+static int
+check_drop_privileges(struct xrdp_startup_params *startup_params)
+{
+    int rv = 1;
+    const char *user = startup_params->runtime_user;
+    const char *group = startup_params->runtime_group;
+
+    if (user[0] == '\0' && group[0] == '\0')
+    {
+        // Allow this for now
+        LOG(LOG_LEVEL_ALWAYS,
+            "You are running xrdp as root. This is not safe.");
+        rv = 0;
+    }
+    else if (user[0] == '\0' || group[0] == '\0')
+    {
+        LOG(LOG_LEVEL_ERROR,
+            "Both a runtime_user and a runtime_group MUST be specified");
+    }
+    else
+    {
+        rv = g_drop_privileges(user, group);
+        if (rv == 0)
+        {
+            LOG(LOG_LEVEL_INFO, "Switched user:group to %s:%s", user, group);
+        }
+    }
+
+    return rv;
+}
+
 /*****************************************************************************/
 int
 main(int argc, char **argv)
@@ -548,7 +580,7 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
-    if (!read_xrdp_ini_startup_params(&startup_params))
+    if (read_xrdp_ini_startup_params(&startup_params) != 0)
     {
         log_end();
         g_deinit();
@@ -653,7 +685,7 @@ main(int argc, char **argv)
         LOG(LOG_LEVEL_ALWAYS, "Failed to start xrdp daemon, "
             "possibly address already in use.");
     }
-    else
+    else if (check_drop_privileges(&startup_params) == 0)
     {
         g_set_threadid(tc_get_threadid());
         g_signal_user_interrupt(xrdp_shutdown); /* SIGINT */

From ce355fc2351b42bb9b10b207e960d1079573ce8c Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Wed, 28 Feb 2024 15:13:25 +0000
Subject: [PATCH 5/7] Allow for xrdp not being able to delete PID file

If xrdp is running with dropped privileges it won't be able to delete
the PID file it's created. Places where xrdp is stopped need to cater
for this.

It's prefereable to do this than make the PID file writeable by xrdp
with dropped privileges, as this can still lead to DoS attacks if an
attacker manages to modify the PID file from a compromised xrdp
process.
---
 common/os_calls.c     |   6 +++
 common/os_calls.h     |   6 +++
 instfiles/Makefile.am |   7 ++-
 instfiles/init.d/xrdp |   2 +-
 instfiles/rc.d/xrdp   |   7 +++
 xrdp/xrdp.c           | 116 ++++++++++++++++++++++++++++++------------
 6 files changed, 109 insertions(+), 35 deletions(-)

diff --git a/common/os_calls.c b/common/os_calls.c
index 53181f8c9b..afd5a95858 100644
--- a/common/os_calls.c
+++ b/common/os_calls.c
@@ -3538,6 +3538,12 @@ g_sigterm(int pid)
 #endif
 }
 
+/*****************************************************************************/
+int g_pid_is_active(int pid)
+{
+    return (kill(pid, 0) == 0);
+}
+
 /*****************************************************************************/
 /* does not work in win32 */
 int
diff --git a/common/os_calls.h b/common/os_calls.h
index 99e339b3c2..be06b07b45 100644
--- a/common/os_calls.h
+++ b/common/os_calls.h
@@ -372,6 +372,12 @@ int      g_exit(int exit_code);
 int      g_getpid(void);
 int      g_sigterm(int pid);
 int      g_sighup(int pid);
+/*
+ * Is a particular PID active?
+ * @param pid PID to check
+ * Returns boolean
+ */
+int      g_pid_is_active(int pid);
 int      g_getuser_info_by_name(const char *username, int *uid, int *gid,
                                 char **shell, char **dir, char **gecos);
 int      g_getuser_info_by_uid(int uid, char **username, int *gid,
diff --git a/instfiles/Makefile.am b/instfiles/Makefile.am
index 81275d102f..9fd5d3d512 100644
--- a/instfiles/Makefile.am
+++ b/instfiles/Makefile.am
@@ -98,6 +98,9 @@ endif
 if FREEBSD
 # must be tab below
 install-data-hook:
-	sed -i '' 's|%%PREFIX%%|$(prefix)|g' $(DESTDIR)$(sysconfdir)/rc.d/xrdp \
-                                         $(DESTDIR)$(sysconfdir)/rc.d/xrdp-sesman
+	sed -e 's|%%PREFIX%%|$(prefix)|g' \
+	    -e 's|%%LOCALSTATEDIR%%|$(localstatedir)|g' \
+		-i '' \
+		$(DESTDIR)$(sysconfdir)/rc.d/xrdp \
+		$(DESTDIR)$(sysconfdir)/rc.d/xrdp-sesman
 endif
diff --git a/instfiles/init.d/xrdp b/instfiles/init.d/xrdp
index e3607b08a7..39f25cdde3 100644
--- a/instfiles/init.d/xrdp
+++ b/instfiles/init.d/xrdp
@@ -116,7 +116,7 @@ case "$1" in
         log_progress_msg $NAME
         if pidofproc -p  $PIDDIR/$NAME.pid $DAEMON  > /dev/null; then
             start-stop-daemon --stop --quiet --oknodo --pidfile $PIDDIR/$NAME.pid \
-	    --exec $DAEMON
+	    --remove-pidfile --exec $DAEMON
             value=$?
             [ $value -gt 0 ] && exitval=$value
         else
diff --git a/instfiles/rc.d/xrdp b/instfiles/rc.d/xrdp
index b5dbcd2518..39f35d657b 100644
--- a/instfiles/rc.d/xrdp
+++ b/instfiles/rc.d/xrdp
@@ -48,6 +48,7 @@ command="%%PREFIX%%/sbin/xrdp"
 allstart_cmd="xrdp_allstart"
 allstop_cmd="xrdp_allstop"
 allrestart_cmd="xrdp_allrestart"
+stop_postcmd="xrdp_poststop"
 
 xrdp_allstart()
 {
@@ -79,4 +80,10 @@ xrdp_allrestart()
     run_rc_command "restart"
 }
 
+xrdp_poststop()
+{
+    # If running with dropped privileges, xrdp can't delete its own
+    # PID file
+    rm -f %%LOCALSTATEDIR%%/run/xrdp.pid
+}
 run_rc_command "$1"
diff --git a/xrdp/xrdp.c b/xrdp/xrdp.c
index 33aeb7b76f..0b7a453d7e 100644
--- a/xrdp/xrdp.c
+++ b/xrdp/xrdp.c
@@ -448,6 +448,83 @@ check_drop_privileges(struct xrdp_startup_params *startup_params)
     return rv;
 }
 
+/*****************************************************************************/
+static int
+read_pid_file(const char *pid_file)
+{
+    int pid = -1;
+    int fd = g_file_open_ro(pid_file); /* xrdp.pid */
+    if (fd >= 0)
+    {
+        char text[32];
+        g_memset(text, 0, sizeof(text));
+        g_file_read(fd, text, sizeof(text) - 1);
+        pid = g_atoi(text);
+        g_file_close(fd);
+    }
+
+    return pid;
+}
+
+/*****************************************************************************/
+/**
+ * Kills an active xrdp daemon
+ *
+ * It is assumed that logging is not active
+ *
+ * @param pid_file PID file
+ * @return 0 for success
+ */
+static int
+kill_daemon(const char *pid_file)
+{
+    int status = 1;
+    int pid;
+    if (g_getuid() != 0)
+    {
+        g_writeln("Must be root");
+    }
+    else if ((pid = read_pid_file(pid_file)) > 0)
+    {
+        if (!g_pid_is_active(pid))
+        {
+            g_writeln("Daemon not active");
+            status = 0;
+        }
+        else
+        {
+            g_writeln("stopping process id %d", pid);
+            int i;
+            g_sigterm(pid);
+            g_sleep(100);
+            i = 5 * 1000 / 500;
+            while (i > 0 && g_pid_is_active(pid))
+            {
+                g_sleep(500);
+                --i;
+            }
+
+            if (g_pid_is_active(pid))
+            {
+                g_writeln("Can't stop process");
+            }
+            else
+            {
+                status = 0;
+            }
+        }
+
+        if (status == 0)
+        {
+            /* delete the xrdp.pid file, as xrdp can't do this
+             * if it's running without privilege */
+            g_file_delete(pid_file);
+        }
+    }
+
+    return status;
+}
+
 /*****************************************************************************/
 int
 main(int argc, char **argv)
@@ -520,36 +597,9 @@ main(int argc, char **argv)
 
     if (startup_params.kill)
     {
-        g_writeln("stopping xrdp");
-        /* read the xrdp.pid file */
-        int fd = -1;
-
-        if (g_file_exist(pid_file)) /* xrdp.pid */
-        {
-            fd = g_file_open_ro(pid_file); /* xrdp.pid */
-        }
-
-        if (fd == -1)
-        {
-            g_writeln("cannot open %s, maybe xrdp is not running", pid_file);
-        }
-        else
-        {
-            g_memset(text, 0, 32);
-            g_file_read(fd, text, 31);
-            pid = g_atoi(text);
-            g_writeln("stopping process id %d", pid);
-
-            if (pid > 0)
-            {
-                g_sigterm(pid);
-            }
-
-            g_file_close(fd);
-        }
-
+        int status = kill_daemon(pid_file);
         g_deinit();
-        g_exit(0);
+        g_exit(status);
     }
 
     /* starting logging subsystem */
@@ -587,9 +637,10 @@ main(int argc, char **argv)
         g_exit(1);
     }
 
-    if (g_file_exist(pid_file)) /* xrdp.pid */
+    if ((pid = read_pid_file(pid_file)) > 0 && g_pid_is_active(pid))
     {
-        LOG(LOG_LEVEL_ALWAYS, "It looks like xrdp is already running.");
+        LOG(LOG_LEVEL_ALWAYS,
+            "It looks like xrdp (pid=%d) is already running.", pid);
         LOG(LOG_LEVEL_ALWAYS, "If not, delete %s and try again.", pid_file);
         log_end();
         g_deinit();
@@ -743,7 +794,8 @@ main(int argc, char **argv)
 
     if (daemon)
     {
-        /* delete the xrdp.pid file */
+        /* Try to delete the PID file, although if we've dropped
+         * privileges this won't be successful */
         g_file_delete(pid_file);
     }
 

From 48255da29a8f7fd642eb24ea82898f634170ec85 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Fri, 1 Mar 2024 14:40:35 +0000
Subject: [PATCH 6/7] Add xrdp-chkpriv script to check xrdp privileges

---
 configure.ac                  |   1 +
 docs/man/xrdp.ini.5.in        |  12 ++
 tools/Makefile.am             |   1 +
 tools/chkpriv/Makefile.am     |  31 +++++
 tools/chkpriv/xrdp-chkpriv.in | 205 ++++++++++++++++++++++++++++++++++
 tools/chkpriv/xrdp-droppriv.c |  49 ++++++++
 xrdp/xrdp.ini.in              |   7 +-
 7 files changed, 301 insertions(+), 5 deletions(-)
 create mode 100644 tools/chkpriv/Makefile.am
 create mode 100755 tools/chkpriv/xrdp-chkpriv.in
 create mode 100755 tools/chkpriv/xrdp-droppriv.c

diff --git a/configure.ac b/configure.ac
index 8b11da5146..75296f23d6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -646,6 +646,7 @@ AC_CONFIG_FILES([
   tools/Makefile
   tools/devel/Makefile
   tools/devel/tcp_proxy/Makefile
+  tools/chkpriv/Makefile
   vnc/Makefile
   xrdpapi/Makefile
   xrdp/Makefile
diff --git a/docs/man/xrdp.ini.5.in b/docs/man/xrdp.ini.5.in
index c7a228411e..039852ad02 100644
--- a/docs/man/xrdp.ini.5.in
+++ b/docs/man/xrdp.ini.5.in
@@ -127,9 +127,21 @@ User name and group to run the xrdp daemon under.
 
 After xrdp starts, it sets its UID and GID to values derived from these
 settings, so that it's running without system privilege.
+
 The \fBruntime_group\fP MUST be set to the same value as
 \fBSessionSockdirGroup\fP in \fBsesman.ini\fP if you want to run sessions.
 
+A suitable user and group can be added with a command like this (Linux):-
+
+useradd xrdp -d / -c 'xrdp daemon' -s /usr/sbin/nologin
+
+In order to establish secure connections, the xrdp daemon needs permission
+to access sensitive cryptographic files. After changing either or both
+of these values, check that xrdp has access to required files by running
+this script:-
+
+@xrdpdatadir@/xrdp-chkpriv
+
 .TP
 \fBenable_token_login\fP=\fI[true|false]\fP
 If set to \fB1\fP, \fBtrue\fP or \fByes\fP, \fBxrdp\fP will scan the user name provided by the
diff --git a/tools/Makefile.am b/tools/Makefile.am
index c4f7c462ca..ef5c72fe44 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -1,3 +1,4 @@
 
 SUBDIRS = \
+  chkpriv \
   devel
diff --git a/tools/chkpriv/Makefile.am b/tools/chkpriv/Makefile.am
new file mode 100644
index 0000000000..c6b1d04b8a
--- /dev/null
+++ b/tools/chkpriv/Makefile.am
@@ -0,0 +1,31 @@
+xrdppkgdatadir=$(datadir)/xrdp
+
+pkglibexec_PROGRAMS = \
+  xrdp-droppriv
+
+dist_xrdppkgdata_SCRIPTS = \
+  xrdp-chkpriv
+
+AM_LDFLAGS = 
+
+AM_CPPFLAGS = \
+  -I$(top_srcdir)/common
+
+xrdp_droppriv_SOURCES = xrdp-droppriv.c
+
+xrdp_droppriv_LDADD = \
+  $(top_builddir)/common/libcommon.la
+
+SUBST_VARS = sed \
+   -e 's|@pkglibexecdir[@]|$(pkglibexecdir)|g'
+
+subst_verbose = $(subst_verbose_@AM_V@)
+subst_verbose_ = $(subst_verbose_@AM_DEFAULT_V@)
+subst_verbose_0 = @echo "  SUBST    $@";
+
+SUFFIXES = .in
+.in:
+	$(subst_verbose)$(SUBST_VARS) $< > $@
+
+CLEANFILES = xrdp-chkpriv
+
diff --git a/tools/chkpriv/xrdp-chkpriv.in b/tools/chkpriv/xrdp-chkpriv.in
new file mode 100755
index 0000000000..f78bb72287
--- /dev/null
+++ b/tools/chkpriv/xrdp-chkpriv.in
@@ -0,0 +1,205 @@
+#!/bin/sh
+#
+# xrdp: A Remote Desktop Protocol server.
+#
+# Copyright (C) Jay Sorg and contributors 2004-2024
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# Program to check permissions for xrdp when running in a non-privileged
+# mode
+
+# Change these if they do not match your installation
+CONF_DIR=/etc/xrdp
+XRDP_INI="$CONF_DIR"/xrdp.ini
+SESMAN_INI="$CONF_DIR"/sesman.ini
+RSAKEYS_INI="$CONF_DIR"/rsakeys.ini
+DROPPRIV=@pkglibexecdir@/xrdp-droppriv
+
+# -----------------------------------------------------------------------------
+# G E T   I N I   V A L U E
+#
+# Gets a value from an ini file.
+#
+# Params [ini_file] [key]
+# -----------------------------------------------------------------------------
+GetIniValue()
+{
+    # Look for a line matching 'key=' with optional whitespace
+    # either side of the key. When we find one, strip everything
+    # up to and including the first '=', print it, and quit
+    #
+    # This doesn't take sections into account
+    sed -n -e '/^ *'"$2"' *=/{
+        s/^[^=]*=//p
+        q
+    }' "$1"
+}
+
+# -----------------------------------------------------------------------------
+# M A I N
+# -----------------------------------------------------------------------------
+
+if [ "$(id -u)" != 0 ]; then
+    echo "** Must run this script as root" >&2
+    exit 1
+fi
+
+OS=$(uname)
+case "$OS" in
+    FreeBSD | Linux) ;;
+    *) echo "Unsupported operating system $OS" >&2
+       exit 1
+esac
+
+errors=0
+
+runtime_user=$(GetIniValue "$XRDP_INI" runtime_user)
+runtime_group=$(GetIniValue "$XRDP_INI" runtime_group)
+certificate=$(GetIniValue "$XRDP_INI" certificate)
+key_file=$(GetIniValue "$XRDP_INI" key_file)
+SessionSockdirGroup=$(GetIniValue "$SESMAN_INI" SessionSockdirGroup)
+
+case "$certificate" in
+    '') certificate="$CONF_DIR"/cert.pem ;;
+    /*) ;;
+    *) certificate="$CONF_DIR"/"$certificate"
+esac
+
+case "$key_file" in
+    '') key_file="$CONF_DIR"/key.pem ;;
+    /*) ;;
+    *) key_file="$CONF_DIR"/"$key_file"
+esac
+
+echo "Settings"
+echo " - [xrdp.ini]   runtime_user        : $runtime_user"
+echo " - [xrdp.ini]   runtime_group       : $runtime_group"
+echo " - [xrdp.ini]   certificate         : $certificate"
+echo " - [xrdp.ini]   key_file            : $key_file"
+echo " - [sesman.ini] SessionSockdirGroup : $SessionSockdirGroup"
+echo
+
+# Basic checks on runtime user/group
+if [ -z "$runtime_user" ] && [ -z "$runtime_group" ]; then
+    echo "-Info- This system is not configured to run xrdp without privilege"
+    exit 0
+fi
+
+if [ -z "$runtime_user" ] || [ -z "$runtime_group" ]; then
+    echo "-Error- Both 'runtime_user' and 'runtime_group' must be set"
+    errors=$(( errors + 1 ))
+    exit 1
+fi
+
+if getent passwd "$runtime_user" >/dev/null ; then
+    echo "-Info- runtime_user '$runtime_user' appears to exist"
+else
+    echo "-Error- runtime_user '$runtime_user' does not exist"
+    errors=$(( errors + 1 ))
+fi
+
+GID=
+if getent group "$runtime_group" >/dev/null ; then
+    echo "-Info- runtime_group '$runtime_group' appears to exist"
+    GID=$(getent group xrdp | cut -d: -f3)
+else
+    echo "-Error- runtime_group '$runtime_group' does not exist"
+    errors=$(( errors + 1 ))
+fi
+
+# Groups agree between sesman and xrdp?
+if [ "$runtime_user" = "$SessionSockdirGroup" ]; then
+    echo "-Info- xrdp.ini and sesman.ini agree on group ownbership"
+else
+    echo "-Error- xrdp.ini and sesman.ini do not agree on group ownbership"
+    errors=$(( errors + 1 ))
+fi
+
+# Check we can access rsakeys.ini
+#
+# This is our file, so we can be completely prescriptive about
+# the permissions
+if [ -e $RSAKEYS_INI ]; then
+    # Only check if we have a GID
+    if [ -n "$GID" ]; then
+        # Get the permissions, UID and GID in $1..$3
+        case "$OS" in
+            FreeBSD)
+                # shellcheck disable=SC2046
+                set -- $(stat -f "%Lp %u %g" $RSAKEYS_INI)
+                ;;
+            *)
+                # shellcheck disable=SC2046
+                set -- $(stat -c "%a %u %g" $RSAKEYS_INI)
+        esac
+        if [ "$1/$2/$3" = "640/0/$GID" ]; then
+            echo "-Info- $RSAKEYS_INI has correct permissions"
+        else
+            if [ "$1" != 640 ]; then
+                echo "-Error- $RSAKEYS_INI should have permissions  -rw-r-----"
+                errors=$(( errors + 1 ))
+            fi
+            if [ "$2" != 0 ]; then
+                echo "-Error- $RSAKEYS_INI should be owned by root"
+                errors=$(( errors + 1 ))
+            fi
+            if [ "$3" != "$GID" ]; then
+                echo "-Error- $RSAKEYS_INI should be in the $runtime_group group"
+                errors=$(( errors + 1 ))
+            fi
+        fi
+    fi
+else
+    echo "-Error- $RSAKEYS_INI does not exist"
+    errors=$(( errors + 1 ))
+fi
+
+# Are cert and key readable by the user?
+#
+# These aren't necessarily our files, so we can't be prescriptive about
+# privileges.  On Debian for example, we might be using the 'ssl-cert'
+# group to obtain access to /etc/ssl/private/ssl-cert-snakeoil.key
+if ! [ -e $certificate ]; then
+    echo "-Error- $certificate does not exist"
+    errors=$(( errors + 1 ))
+elif $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -r '"$certificate"' ]'
+then
+    echo "-Info- $certificate is readable by $runtime_user:$runtime_group"
+else
+    echo "-Error- $certificate is not readable by $runtime_user:$runtime_group"
+    errors=$(( errors + 1 ))
+fi
+
+if ! [ -e $key_file ]; then
+    echo "-Error- $key_file does not exist"
+    errors=$(( errors + 1 ))
+elif $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -r '"$key_file"' ]'
+    sh -c '[ -r '"$key_file"' ]'
+then
+    echo "-Info- $key_file is readable by $runtime_user:$runtime_group"
+else
+    echo "-Error- $key_file is not readable by $runtime_user:$runtime_group"
+    errors=$(( errors + 1 ))
+fi
+
+echo
+if [ $errors -eq 0 ]; then
+    echo "-Summary- Permissions appear to be correct to run xrdp unprivileged"
+    status=0
+else
+    echo "-Summary- $errors error(s) found. Please correct these and try again"
+    status=1
+fi
+
+exit $status
diff --git a/tools/chkpriv/xrdp-droppriv.c b/tools/chkpriv/xrdp-droppriv.c
new file mode 100755
index 0000000000..185f575b10
--- /dev/null
+++ b/tools/chkpriv/xrdp-droppriv.c
@@ -0,0 +1,49 @@
+/*
+ *
+ * xrdp: A Remote Desktop Protocol server.
+ *
+ * Copyright (C) Jay Sorg and contributors 2004-2024
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Shell around the g_drop_privileges() call
+ */
+
+#if defined(HAVE_CONFIG_H)
+#include "config_ac.h"
+#endif
+
+#include "os_calls.c"
+#include "log.h"
+
+int main(int argc, char *argv[])
+{
+    struct log_config *logging;
+    int status = 1;
+    logging = log_config_init_for_console(LOG_LEVEL_WARNING,
+                                          g_getenv("DROPPRIV_LOG_LEVEL"));
+    log_start_from_param(logging);
+    log_config_free(logging);
+
+    if (argc < 4)
+    {
+        LOG(LOG_LEVEL_ERROR, "Usage : %s [user] [group] [cmd...]\n", argv[0]);
+    }
+    else if (g_drop_privileges(argv[1], argv[2]) == 0)
+    {
+        status = g_execvp(argv[3], &argv[3]);
+    }
+
+    log_end();
+    return status;
+}
diff --git a/xrdp/xrdp.ini.in b/xrdp/xrdp.ini.in
index d2998a3c3a..efa25fc60d 100644
--- a/xrdp/xrdp.ini.in
+++ b/xrdp/xrdp.ini.in
@@ -28,11 +28,8 @@ port=3389
 use_vsock=false
 
 ; Unprivileged User name and group to run the xrdp daemon.
-; It is HIGHLY RECOMMENDED you set these values.
-; A suitable user and group can be added with a command like this (Linux):-
-;     useradd xrdp -d / -c 'xrdp daemon' -s /usr/sbin/nologin
-; Be aware that runtime_group here, and SessionSockdirGroup in sesman.ini
-; MUST be the same if you want to run sessions.
+; It is HIGHLY RECOMMENDED you set these values. See the xrdp.ini(5)
+; manpage for more information on setting and checking these.
 #runtime_user=xrdp
 #runtime_group=xrdp
 

From 0ebf4cff13a4235ad990af4e89c66c0ef7150da9 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Mon, 1 Jul 2024 14:25:23 +0100
Subject: [PATCH 7/7] Check unprivileged user can't write TLS files

The unprivileged user needs to be able to read the certificate and
key files to offer TLS, but should not be able to write to then.

This commit checks the TLS files are read-only, rather than
simply readable
---
 tools/chkpriv/xrdp-chkpriv.in | 46 +++++++++++++++--------------------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/tools/chkpriv/xrdp-chkpriv.in b/tools/chkpriv/xrdp-chkpriv.in
index f78bb72287..3193e0d3af 100755
--- a/tools/chkpriv/xrdp-chkpriv.in
+++ b/tools/chkpriv/xrdp-chkpriv.in
@@ -120,9 +120,9 @@ fi
 
 # Groups agree between sesman and xrdp?
 if [ "$runtime_user" = "$SessionSockdirGroup" ]; then
-    echo "-Info- xrdp.ini and sesman.ini agree on group ownbership"
+    echo "-Info- xrdp.ini and sesman.ini agree on group ownership"
 else
-    echo "-Error- xrdp.ini and sesman.ini do not agree on group ownbership"
+    echo "-Error- xrdp.ini and sesman.ini do not agree on group ownership"
     errors=$(( errors + 1 ))
 fi
 
@@ -165,33 +165,27 @@ else
     errors=$(( errors + 1 ))
 fi
 
-# Are cert and key readable by the user?
+# Are cert and key readable (but NOT writeable) by the user?
 #
-# These aren't necessarily our files, so we can't be prescriptive about
+# These aren't necessarily our files, so we can't be too prescriptive about
 # privileges.  On Debian for example, we might be using the 'ssl-cert'
 # group to obtain access to /etc/ssl/private/ssl-cert-snakeoil.key
-if ! [ -e $certificate ]; then
-    echo "-Error- $certificate does not exist"
-    errors=$(( errors + 1 ))
-elif $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -r '"$certificate"' ]'
-then
-    echo "-Info- $certificate is readable by $runtime_user:$runtime_group"
-else
-    echo "-Error- $certificate is not readable by $runtime_user:$runtime_group"
-    errors=$(( errors + 1 ))
-fi
-
-if ! [ -e $key_file ]; then
-    echo "-Error- $key_file does not exist"
-    errors=$(( errors + 1 ))
-elif $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -r '"$key_file"' ]'
-    sh -c '[ -r '"$key_file"' ]'
-then
-    echo "-Info- $key_file is readable by $runtime_user:$runtime_group"
-else
-    echo "-Error- $key_file is not readable by $runtime_user:$runtime_group"
-    errors=$(( errors + 1 ))
-fi
+for file in "$certificate" "$key_file"; do
+    if ! [ -e $file ]; then
+        echo "-Error- $file does not exist"
+        errors=$(( errors + 1 ))
+    elif ! $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -r '"$file"' ]'
+    then
+        echo "-Error- $file is not readable by $runtime_user:$runtime_group"
+        errors=$(( errors + 1 ))
+    elif $DROPPRIV "$runtime_user" "$runtime_group" sh -c '[ -w '"$file"' ]'
+    then
+        echo "-Error- $file is writeable by $runtime_user:$runtime_group"
+        errors=$(( errors + 1 ))
+    else
+        echo "-Info- $file is read-only for $runtime_user:$runtime_group"
+    fi
+done
 
 echo
 if [ $errors -eq 0 ]; then