Hardening xrdp with AppArmor

common/file.c

@@ -349,7 +349,7 @@ file_by_name_read_sections(const char *file_name, struct list *names)
         return 1;
     }
 
-    fd = g_file_open_ex(file_name, 1, 0, 0, 0);
+    fd = g_file_open_ro(file_name);
 
     if (fd < 0)
     {
@@ -390,7 +390,7 @@ file_by_name_read_section(const char *file_name, const char *section,
         return 1;
     }
 
-    fd = g_file_open_ex(file_name, 1, 0, 0, 0);
+    fd = g_file_open_ro(file_name);
 
     if (fd < 0)
     {

common/log.c

@@ -689,7 +689,7 @@ log_config_init_from_config(const char *iniFilename,
         return NULL;
     }
 
-    fd = g_file_open_ex(iniFilename, 1, 0, 0, 0);
+    fd = g_file_open_ro(iniFilename);
 
     if (-1 == fd)
     {

common/os_calls.c

@@ -53,6 +53,9 @@
 #include <sys/stat.h>
 #include <sys/ipc.h>
 #include <sys/shm.h>
+#if defined(HAVE_SYS_PRCTL_H)
+#include <sys/prctl.h>
+#endif
 #include <dlfcn.h>
 #include <arpa/inet.h>
 #include <netdb.h>
@@ -2086,24 +2089,14 @@ g_memcmp(const void *s1, const void *s2, int len)
 /*****************************************************************************/
 /* returns -1 on error, else return handle or file descriptor */
 int
-g_file_open(const char *file_name)
+g_file_open_rw(const char *file_name)
 {
 #if defined(_WIN32)
     return (int)CreateFileA(file_name, GENERIC_READ | GENERIC_WRITE,
                             FILE_SHARE_READ | FILE_SHARE_WRITE,
                             0, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);
 #else
-    int rv;
-
-    rv =  open(file_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
-
-    if (rv == -1)
-    {
-        /* can't open read / write, try to open read only */
-        rv =  open(file_name, O_RDONLY);
-    }
-
-    return rv;
+    return open(file_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
 #endif
 }
 
@@ -2145,6 +2138,14 @@ g_file_open_ex(const char *file_name, int aread, int awrite,
 #endif
 }
 
+/*****************************************************************************/
+/* returns -1 on error, else return handle or file descriptor */
+int
+g_file_open_ro(const char *file_name)
+{
+    return g_file_open_ex(file_name, 1, 0, 0, 0);
+}
+
 /*****************************************************************************/
 /* returns error, always 0 */
 int
@@ -3956,3 +3957,19 @@ g_tcp6_bind_address(int sck, const char *port, const char *address)
     return -1;
 #endif
 }
+
+/*****************************************************************************/
+/* returns error, zero is success, non zero is error */
+/* only works in linux */
+int
+g_no_new_privs(void)
+{
+#if defined(HAVE_SYS_PRCTL_H) && defined(PR_SET_NO_NEW_PRIVS)
+    /*
+     * PR_SET_NO_NEW_PRIVS requires Linux kernel 3.5 and newer.
+     */
+    return prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+#else
+    return 0;
+#endif
+}

common/os_calls.h

@@ -206,9 +206,10 @@ int      g_obj_wait(tintptr *read_objs, int rcount, tintptr *write_objs,
 void     g_random(char *data, int len);
 int      g_abs(int i);
 int      g_memcmp(const void *s1, const void *s2, int len);
-int      g_file_open(const char *file_name);
+int      g_file_open_rw(const char *file_name);
 int      g_file_open_ex(const char *file_name, int aread, int awrite,
                         int acreate, int atrunc);
+int      g_file_open_ro(const char *file_name);
 int      g_file_close(int fd);
 /**
  * Returns 1 if a file is open (i.e. the file descriptor is valid)
@@ -332,6 +333,7 @@ int      g_tcp4_socket(void);
 int      g_tcp4_bind_address(int sck, const char *port, const char *address);
 int      g_tcp6_socket(void);
 int      g_tcp6_bind_address(int sck, const char *port, const char *address);
+int      g_no_new_privs(void);
 
 /* glib-style wrappers */
 #define g_new(struct_type, n_structs) \

docs/man/sesman.ini.5.in

@@ -303,6 +303,15 @@ if the group specified in \fBTerminalServerUsers\fR doesn't exist.
 \fBAllowAlternateShell\fR=\fI[true|false]\fR
 If set to \fB0\fR, \fBfalse\fR or \fBno\fR, prevent usage of alternate shells by users.
 
+.TP
+\fBXorgNoNewPrivileges\fR=\fI[true|false]\fR
+Only applicable on Linux. If set to \fB0\fR, \fBfalse\fR or \fBno\fR, do
+not use the kernel's \fIno_new_privs\fR restriction when invoking the Xorg
+X11 server. The use of \fIno_new_privs\fR is intended to prevent issues due
+to a setuid Xorg executable. However, if a kernel security module (such as
+AppArmor) is used to confine xrdp, \fIno_new_privs\fR may interfere with
+transitions between confinement domains.
+
 .SH "X11 SERVER"
 Following parameters can be used in the \fB[Xvnc]\fR and
 \fB[Xorg]\fR sections.

fontutils/fv1.c

@@ -170,7 +170,7 @@ fv1_file_load(const char *filename)
             int fd;
             make_stream(s);
             init_stream(s, file_size + 1024);
-            fd = g_file_open(filename);
+            fd = g_file_open_ro(filename);
 
             if (fd < 0)
             {

fontutils/windows/fontdump.c

@@ -155,7 +155,7 @@ font_dump(void)
     g_snprintf(filename, 255, "%s-%d.fv1", g_font_name, g_font_size);
     msg("creating file %s", filename);
     g_file_delete(filename);
-    fd = g_file_open(filename);
+    fd = g_file_open_rw(filename);
     g_file_write(fd, "FNT1", 4);
     strlen1 = g_strlen(g_font_name);
     g_file_write(fd, g_font_name, strlen1);

keygen/keygen.c

@@ -367,7 +367,7 @@ save_all(const char *e_data, int e_len, const char *n_data, int n_len,
         }
     }
 
-    fd = g_file_open(filename);
+    fd = g_file_open_rw(filename);
 
     if (fd != -1)
     {

libxrdp/xrdp_sec.c

@@ -420,7 +420,7 @@ xrdp_load_keyboard_layout(struct xrdp_client_info *client_info)
     g_snprintf(keyboard_cfg_file, 255, "%s/xrdp_keyboard.ini", XRDP_CFG_PATH);
     LOG(LOG_LEVEL_DEBUG, "keyboard_cfg_file %s", keyboard_cfg_file);
 
-    fd = g_file_open(keyboard_cfg_file);
+    fd = g_file_open_ro(keyboard_cfg_file);
 
     if (fd >= 0)
     {

sesman/chansrv/chansrv_config.c

@@ -231,7 +231,7 @@ config_read(int use_logger, const char *sesman_ini)
     log_func_t logmsg = (use_logger) ? log_message : log_to_stdout;
     int fd;
 
-    fd = g_file_open_ex(sesman_ini, 1, 0, 0, 0);
+    fd = g_file_open_ro(sesman_ini);
     if (fd < 0)
     {
         logmsg(LOG_LEVEL_ERROR, "Can't open config file %s", sesman_ini);

sesman/chansrv/clipboard_file.c

@@ -462,7 +462,7 @@ clipboard_send_file_data(int streamId, int lindex,
               "nPositionLow %d cbRequested %d", streamId, lindex,
               nPositionLow, cbRequested);
     g_snprintf(full_fn, 255, "%s/%s", cfi->pathname, cfi->filename);
-    fd = g_file_open_ex(full_fn, 1, 0, 0, 0);
+    fd = g_file_open_ro(full_fn);
     if (fd == -1)
     {
         LOG(LOG_LEVEL_ERROR, "clipboard_send_file_data: file open [%s] failed: %s",

sesman/libsesman/sesman_config.c

@@ -70,6 +70,7 @@
 #define SESMAN_CFG_SEC_RESTRICT_OUTBOUND_CLIPBOARD "RestrictOutboundClipboard"
 #define SESMAN_CFG_SEC_RESTRICT_INBOUND_CLIPBOARD  "RestrictInboundClipboard"
 #define SESMAN_CFG_SEC_ALLOW_ALTERNATE_SHELL       "AllowAlternateShell"
+#define SESMAN_CFG_SEC_XORG_NO_NEW_PRIVILEGES      "XorgNoNewPrivileges"
 
 #define SESMAN_CFG_SESSIONS          "Sessions"
 #define SESMAN_CFG_SESS_MAX          "MaxSessions"
@@ -310,6 +311,7 @@ config_read_security(int file, struct config_security *sc,
     sc->restrict_outbound_clipboard = 0;
     sc->restrict_inbound_clipboard = 0;
     sc->allow_alternate_shell = 1;
+    sc->xorg_no_new_privileges = 1;
 
     file_read_section(file, SESMAN_CFG_SECURITY, param_n, param_v);
 
@@ -383,6 +385,11 @@ config_read_security(int file, struct config_security *sc,
                 g_text2bool((char *)list_get_item(param_v, i));
         }
 
+        if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_XORG_NO_NEW_PRIVILEGES))
+        {
+            sc->xorg_no_new_privileges =
+                g_text2bool((char *)list_get_item(param_v, i));
+        }
     }
 
     return 0;
@@ -583,7 +590,7 @@ config_read(const char *sesman_ini)
         if ((cfg->sesman_ini = g_strdup(sesman_ini)) != NULL)
         {
             int fd;
-            if ((fd = g_file_open_ex(cfg->sesman_ini, 1, 0, 0, 0)) != -1)
+            if ((fd = g_file_open_ro(cfg->sesman_ini)) != -1)
             {
                 struct list *sec;
                 struct list *param_n;
@@ -670,6 +677,9 @@ config_dump(struct config_sesman *config)
     g_writeln("    MaxLoginRetry:             %d", sc->login_retry);
     g_writeln("    AlwaysGroupCheck:          %d", sc->ts_always_group_check);
     g_writeln("    AllowAlternateShell:       %d", sc->allow_alternate_shell);
+#ifdef HAVE_SYS_PRCTL_H
+    g_writeln("    XorgNoNewPrivileges:       %d", sc->xorg_no_new_privileges);
+#endif
     sesman_clip_restrict_mask_to_string(sc->restrict_outbound_clipboard,
                                         restrict_s, sizeof(restrict_s));
     g_writeln("    RestrictOutboundClipboard: %s", restrict_s);

sesman/libsesman/sesman_config.h

@@ -103,6 +103,12 @@ struct config_security
      *          If not specified, 'YES' is assumed.
      */
     int allow_alternate_shell;
+
+    /*
+     * @var xorg_no_new_privileges
+     * @brief if the Xorg X11 server should be started with no_new_privs (Linux only)
+     */
+    int xorg_no_new_privileges;
 };
 
 /**

sesman/lock_uds.c

@@ -87,7 +87,7 @@ lock_uds(const char *sockname)
             *p++ = '\0';
 
             saved_umask = g_umask_hex(0x77);
-            fd = g_file_open(filename);
+            fd = g_file_open_rw(filename);
             g_umask_hex(saved_umask);
             if (fd < 0)
             {

sesman/sesexec/session.c

@@ -33,10 +33,6 @@
 #include "config_ac.h"
 #endif
 
-#ifdef HAVE_SYS_PRCTL_H
-#include <sys/prctl.h>
-#endif
-
 #include <errno.h>
 
 #include "arch.h"
@@ -56,10 +52,6 @@
 #include "xwait.h"
 #include "xrdp_sockets.h"
 
-#ifndef PR_SET_NO_NEW_PRIVS
-#define PR_SET_NO_NEW_PRIVS 38
-#endif
-
 struct session_data
 {
     pid_t x_server; ///< PID of X server
@@ -347,21 +339,18 @@ prepare_xorg_xserver_params(const struct session_parameters *s,
     {
         params->auto_free = 1;
 
-#ifdef HAVE_SYS_PRCTL_H
         /*
          * Make sure Xorg doesn't run setuid root. Root access is not
          * needed. Xorg can fail when run as root and the user has no
          * console permissions.
-         * PR_SET_NO_NEW_PRIVS requires Linux kernel 3.5 and newer.
          */
-        if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) < 0)
+        if (g_cfg->sec.xorg_no_new_privileges && g_no_new_privs() != 0)
         {
             LOG(LOG_LEVEL_WARNING,
                 "[session start] (display %u): Failed to disable "
                 "setuid on X server: %s",
                 s->display, g_get_strerror());
         }
-#endif
 
         g_snprintf(screen, sizeof(screen), ":%u", s->display);
 

sesman/sesman.c

@@ -658,7 +658,7 @@ read_pid_file(const char *pid_file, int *pid)
     {
         g_printf("sesman is not running (pid file not found - %s)\n", pid_file);
     }
-    else if ((fd = g_file_open(pid_file)) < 0)
+    else if ((fd = g_file_open_ro(pid_file)) < 0)
     {
         g_printf("error opening pid file[%s]: %s\n", pid_file, g_get_strerror());
     }
@@ -838,15 +838,15 @@ main(int argc, char **argv)
         g_file_close(1);
         g_file_close(2);
 
-        if (g_file_open("/dev/null") < 0)
+        if (g_file_open_rw("/dev/null") < 0)
         {
         }
 
-        if (g_file_open("/dev/null") < 0)
+        if (g_file_open_rw("/dev/null") < 0)
         {
         }
 
-        if (g_file_open("/dev/null") < 0)
+        if (g_file_open_rw("/dev/null") < 0)
         {
         }
     }
@@ -905,7 +905,7 @@ main(int argc, char **argv)
     {
         /* writing pid file */
         char pid_s[32];
-        int fd = g_file_open(pid_file);
+        int fd = g_file_open_rw(pid_file);
 
         if (-1 == fd)
         {

sesman/sesman.ini.in

@@ -39,6 +39,11 @@ RestrictOutboundClipboard=none
 RestrictInboundClipboard=none
 ; Set to 'no' to prevent users from logging in with alternate shells
 #AllowAlternateShell=true
+; On Linux systems, the Xorg X11 server is normally invoked using
+; no_new_privs to avoid problems if the executable is suid. This may,
+; however, interfere with the use of security modules such as AppArmor.
+; Leave this unset unless you need to disable it.
+#XorgNoNewPrivileges=true
 
 [Sessions]
 ;; X11DisplayOffset - x11 display number offset

sesman/session_list.c

@@ -33,10 +33,6 @@
 #include "config_ac.h"
 #endif
 
-#ifdef HAVE_SYS_PRCTL_H
-#include <sys/prctl.h>
-#endif
-
 #include "arch.h"
 #include "session_list.h"
 #include "trans.h"