Merge pull request #2731 from matt335672/update_sockdir_security

Update sockdir security

common/Makefile.am

@@ -27,8 +27,7 @@ AM_CPPFLAGS = \
   -DXRDP_SBIN_PATH=\"${sbindir}\" \
   -DXRDP_SHARE_PATH=\"${datadir}/xrdp\" \
   -DXRDP_PID_PATH=\"${localstatedir}/run\" \
-  -DXRDP_LOG_PATH=\"${localstatedir}/log\" \
-  -DXRDP_SOCKET_PATH=\"${socketdir}\"
+  -DXRDP_LOG_PATH=\"${localstatedir}/log\"
 
 # -no-suppress is an automake-specific flag which is needed
 # to prevent us missing compiler errors in some circumstances

common/os_calls.c

@@ -150,28 +150,6 @@ g_rm_temp_dir(void)
     return 0;
 }
 
-/*****************************************************************************/
-int
-g_mk_socket_path(void)
-{
-    if (!g_directory_exist(XRDP_SOCKET_PATH))
-    {
-        if (!g_create_path(XRDP_SOCKET_PATH"/"))
-        {
-            /* if failed, still check if it got created by someone else */
-            if (!g_directory_exist(XRDP_SOCKET_PATH))
-            {
-                LOG(LOG_LEVEL_ERROR,
-                    "g_mk_socket_path: g_create_path(%s) failed",
-                    XRDP_SOCKET_PATH);
-                return 1;
-            }
-        }
-        g_chmod_hex(XRDP_SOCKET_PATH, 0x1777);
-    }
-    return 0;
-}
-
 /*****************************************************************************/
 void
 g_init(const char *app_name)
@@ -2666,7 +2644,7 @@ g_create_dir(const char *dirname)
 #if defined(_WIN32)
     return CreateDirectoryA(dirname, 0); // test this
 #else
-    return mkdir(dirname, (mode_t) - 1) == 0;
+    return mkdir(dirname, 0777) == 0;
 #endif
 }
 

common/os_calls.h

@@ -53,7 +53,6 @@ struct list;
 #define g_close_wait_obj g_delete_wait_obj
 
 int      g_rm_temp_dir(void);
-int      g_mk_socket_path(void);
 void     g_init(const char *app_name);
 void     g_deinit(void);
 void g_printf(const char *format, ...) printflike(1, 2);

common/xrdp_sockets.h

@@ -21,16 +21,39 @@
 #if !defined(XRDP_SOCKETS_H)
 #define XRDP_SOCKETS_H
 
-/* basename of socket files */
+/* XRDP_SOCKET_ROOT_PATH must be defined to include this file */
+#ifdef __cppcheck__
+/*   avoid syntax errors */
+#  define XRDP_SOCKET_ROOT_PATH "/dummy"
+#elif !defined(XRDP_SOCKET_ROOT_PATH)
+#  error "XRDP_SOCKET_ROOT_PATH must be defined"
+#endif
+
+/* Buffer size for code for fullpath declarations
+ *
+ * This needs to fit in the sun_path field of a sockaddr_un. POSIX
+ * does not define this size, so the value below is the lower of
+ * the FreeBSD/OpenBSD/NetBSD(104) and Linux(108) values */
+#define XRDP_SOCKETS_MAXPATH 104
+
+/* The socketdir is rooted at XRDP_SOCKET_ROOT_PATH. User-specific
+ * sockets live in a user-specific sub-directory of this called
+ * XRDP_SOCKET_PATH. The sub-directory is the UID of the user */
+#define XRDP_SOCKET_PATH      XRDP_SOCKET_ROOT_PATH "/%d"
+
+/* Sockets in XRDP_SOCKET_ROOT_PATH */
+#define SCP_LISTEN_PORT_BASE_STR   "sesman.socket"
+
+/* names of socket files within XRDP_SOCKET_PATH, qualified by
+ * display number */
 #define XRDP_CHANSRV_BASE_STR      "xrdp_chansrv_socket_%d"
 #define CHANSRV_PORT_OUT_BASE_STR  "xrdp_chansrv_audio_out_socket_%d"
 #define CHANSRV_PORT_IN_BASE_STR   "xrdp_chansrv_audio_in_socket_%d"
 #define CHANSRV_API_BASE_STR       "xrdpapi_%d"
 #define XRDP_X11RDP_BASE_STR       "xrdp_display_%d"
 #define XRDP_DISCONNECT_BASE_STR   "xrdp_disconnect_display_%d"
-#define SCP_LISTEN_PORT_BASE_STR   "sesman.socket"
 
-/* fullpath of sockets */
+/* fullpath declarations */
 #define XRDP_CHANSRV_STR      XRDP_SOCKET_PATH "/" XRDP_CHANSRV_BASE_STR
 #define CHANSRV_PORT_OUT_STR  XRDP_SOCKET_PATH "/" CHANSRV_PORT_OUT_BASE_STR
 #define CHANSRV_PORT_IN_STR   XRDP_SOCKET_PATH "/" CHANSRV_PORT_IN_BASE_STR

configure.ac

@@ -62,12 +62,6 @@ AC_ARG_WITH([socketdir],
                   [], [with_socketdir="$runstatedir/xrdp"])
 AC_SUBST([socketdir], [$with_socketdir])
 
-AC_ARG_WITH([sesmanruntimedir],
-  [AS_HELP_STRING([--with-sesmanruntimedir=DIR],
-                  [Use directory for sesman runtime data (default: RUNSTATEDIR/xrdp-sesman)])],
-                  [], [with_sesmanruntimedir="$runstatedir/xrdp-sesman"])
-AC_SUBST([sesmanruntimedir], [$with_sesmanruntimedir])
-
 AC_ARG_WITH([systemdsystemunitdir],
         AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files, no to disable]),
         [], [
@@ -655,7 +649,6 @@ echo "  pamconfdir              $pamconfdir"
 echo "  localstatedir           $localstatedir"
 echo "  runstatedir             $runstatedir"
 echo "  socketdir               $socketdir"
-echo "  sesmanruntimedir        $sesmanruntimedir"
 echo ""
 echo "  unit tests performable  $perform_unit_tests"
 echo ""

docs/man/Makefile.am

@@ -27,7 +27,6 @@ SUBST_VARS = sed \
    -e 's|@localstatedir[@]|$(localstatedir)|g' \
    -e 's|@sysconfdir[@]|$(sysconfdir)|g' \
    -e 's|@socketdir[@]|$(socketdir)|g' \
-   -e 's|@sesmanruntimedir[@]|$(sesmanruntimedir)|g' \
    -e 's|@xrdpconfdir[@]|$(sysconfdir)/xrdp|g' \
    -e 's|@xrdpdatadir[@]|$(datadir)/xrdp|g' \
    -e 's|@xrdphomeurl[@]|http://www.xrdp.org/|g'

docs/man/sesman.ini.5.in

@@ -57,7 +57,7 @@ In this instance, the system administrator is responsible for ensuring
 the socket can only be created by a suitably privileged process.
 .PP
 If the parameter does not start with a '/', a name within
-@sesmanruntimedir@ is used.
+@socketdir@ is used.
 .RE
 
 .TP
@@ -312,6 +312,11 @@ 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.
 
+.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.
+
 .SH "X11 SERVER"
 Following parameters can be used in the \fB[Xvnc]\fR and
 \fB[Xorg]\fR sections.

docs/man/xrdp-sesadmin.8.in

@@ -28,7 +28,7 @@ Retained for compatibility, but ignored.
 .BI \-i= port
 The sesman \fIUNIX domain socket\fP to connect to.
 Defaults to \fBsesman.socket\fP.
-If no path is specified for the socket, a default of @sesmanruntimedir@ is used.
+If no path is specified for the socket, a default of @socketdir@ is used.
 
 .TP
 .BI \-c= command

docs/man/xrdp-sesman.8.in

@@ -70,7 +70,7 @@ not running \fBxrdp\-sesman\fR as a daemon.
 .br
 @localstatedir@/run/xrdp\-sesman.pid
 .br
-@sesmanruntimedir@/sesman.socket
+@socketdir@/sesman.socket
 
 .SH "AUTHORS"
 Jay Sorg <[email protected]>

libipm/Makefile.am

@@ -1,6 +1,6 @@
 
 AM_CPPFLAGS = \
-  -DSESMAN_RUNTIME_PATH=\"${sesmanruntimedir}\" \
+  -DXRDP_SOCKET_ROOT_PATH=\"${socketdir}\" \
   -I$(top_srcdir)/common
 
 module_LTLIBRARIES = \

libipm/scp.c

@@ -27,6 +27,8 @@
 #include <config_ac.h>
 #endif
 
+#include <ctype.h>
+
 #include "scp.h"
 #include "libipm.h"
 #include "guid.h"
@@ -76,6 +78,23 @@ scp_msgno_to_str(enum scp_msg_code n, char *buff, unsigned int buff_size)
     return buff;
 }
 
+/*****************************************************************************/
+/**
+ * Helper function returning 1 if the passed-in string is an integer >= 0
+ */
+static int is_positive_int(const char *s)
+{
+    for ( ; *s != '\0' ; ++s)
+    {
+        if (!isdigit(*s))
+        {
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
 /*****************************************************************************/
 int
 scp_port_to_unix_domain_path(const char *port, char *buff,
@@ -111,7 +130,7 @@ scp_port_to_unix_domain_path(const char *port, char *buff,
         {
             port = SCP_LISTEN_PORT_BASE_STR;
         }
-        else if (g_strcmp(port, "3350") == 0)
+        else if (is_positive_int(port))
         {
             /* Version v0.9.x and earlier of xrdp used a TCP port
              * number. If we come across this, we'll ignore it for
@@ -121,7 +140,7 @@ scp_port_to_unix_domain_path(const char *port, char *buff,
             port = SCP_LISTEN_PORT_BASE_STR;
         }
 
-        result = g_snprintf(buff, bufflen, SESMAN_RUNTIME_PATH "/%s", port);
+        result = g_snprintf(buff, bufflen, XRDP_SOCKET_ROOT_PATH "/%s", port);
     }
 
     return result;
@@ -332,37 +351,50 @@ scp_get_sys_login_request(struct trans *trans,
 int
 scp_send_login_response(struct trans *trans,
                         enum scp_login_status login_result,
-                        int server_closed)
+                        int server_closed,
+                        int uid)
 {
     return libipm_msg_out_simple_send(
                trans,
                (int)E_SCP_LOGIN_RESPONSE,
-               "ib",
+               "ibi",
                login_result,
-               (server_closed != 0)); /* Convert to 0/1 */
+               (server_closed != 0), /* Convert to 0/1 */
+               uid);
 }
 
 /*****************************************************************************/
 
 int
 scp_get_login_response(struct trans *trans,
                        enum scp_login_status *login_result,
-                       int *server_closed)
+                       int *server_closed,
+                       int *uid)
 {
     int32_t i_login_result = 0;
+    int32_t i_uid = 0;
     int dummy;
+
     /* User can pass in NULL for server_closed if they're trying an
-     * login method like UDS for which all fails are fatal */
+     * login method like UDS for which all fails are fatal. Likewise
+     * they may be uninterested in the uid */
     if (server_closed == NULL)
     {
         server_closed = &dummy;
     }
+    if (uid == NULL)
+    {
+        uid = &dummy;
+    }
 
-    int rv = libipm_msg_in_parse(trans, "ib", &i_login_result, server_closed);
+    int rv = libipm_msg_in_parse(trans, "ibi",
+                                 &i_login_result, server_closed, &i_uid);
     if (rv == 0)
     {
         *login_result = (enum scp_login_status)i_login_result;
+        *uid = i_uid;
     }
+
     return rv;
 }
 

libipm/scp.h

@@ -284,12 +284,14 @@ scp_get_sys_login_request(struct trans *trans,
  * @param login_result What happened to the login
  * @param server_closed If login fails, whether server has closed connection.
  *        If not, a retry can be made.
+ * @param uid UID for a successful login
  * @return != 0 for error
  */
 int
 scp_send_login_response(struct trans *trans,
                         enum scp_login_status login_result,
-                        int server_closed);
+                        int server_closed,
+                        int uid);
 
 /**
  * Parses an incoming E_SCP_LOGIN_RESPONSE (SCP client)
@@ -298,12 +300,17 @@ scp_send_login_response(struct trans *trans,
  * @param[out] login_result 0 for success, PAM error code otherwise
  * @param[out] server_closed If login fails, whether server has closed
  *             connection. If not a retry can be made.
+ * @param[out] uid UID for a successful login
+ *
+ * server_closed and uid can be passed NULL if the caller isn't interested.
+ *
  * @return != 0 for error
  */
 int
 scp_get_login_response(struct trans *trans,
                        enum scp_login_status *login_result,
-                       int *server_closed);
+                       int *server_closed,
+                       int *uid);
 
 /**
  * Send an E_SCP_LOGOUT_REQUEST (SCP client)

sesman/Makefile.am

@@ -6,8 +6,7 @@ AM_CPPFLAGS = \
   -DXRDP_SBIN_PATH=\"${sbindir}\" \
   -DXRDP_LIBEXEC_PATH=\"${libexecdir}/xrdp\" \
   -DXRDP_PID_PATH=\"${localstatedir}/run\" \
-  -DXRDP_SOCKET_PATH=\"${socketdir}\" \
-  -DSESMAN_RUNTIME_PATH=\"${sesmanruntimedir}\" \
+  -DXRDP_SOCKET_ROOT_PATH=\"${socketdir}\" \
   -I$(top_srcdir)/sesman/libsesman \
   -I$(top_srcdir)/common \
   -I$(top_srcdir)/libipm

sesman/chansrv/Makefile.am

@@ -8,7 +8,7 @@ AM_CPPFLAGS = \
   -DXRDP_SBIN_PATH=\"${sbindir}\" \
   -DXRDP_SHARE_PATH=\"${datadir}/xrdp\" \
   -DXRDP_PID_PATH=\"${localstatedir}/run\" \
-  -DXRDP_SOCKET_PATH=\"${socketdir}\" \
+  -DXRDP_SOCKET_ROOT_PATH=\"${socketdir}\" \
   -I$(top_srcdir)/sesman/libsesman \
   -I$(top_srcdir)/common
 

sesman/chansrv/chansrv.c

@@ -1238,7 +1238,7 @@ my_api_trans_conn_in(struct trans *trans, struct trans *new_trans)
 static int
 setup_listen(void)
 {
-    char port[256];
+    char port[XRDP_SOCKETS_MAXPATH];
     int error = 0;
 
     if (g_lis_trans != 0)
@@ -1248,7 +1248,7 @@ setup_listen(void)
 
     g_lis_trans = trans_create(TRANS_MODE_UNIX, 8192, 8192);
     g_lis_trans->is_term = g_is_term;
-    g_snprintf(port, 255, XRDP_CHANSRV_STR, g_display_num);
+    g_snprintf(port, sizeof(port), XRDP_CHANSRV_STR, g_getuid(), g_display_num);
 
     g_lis_trans->trans_conn_in = my_trans_conn_in;
     error = trans_listen(g_lis_trans, port);
@@ -1267,12 +1267,12 @@ setup_listen(void)
 static int
 setup_api_listen(void)
 {
-    char port[256];
+    char port[XRDP_SOCKETS_MAXPATH];
     int error = 0;
 
     g_api_lis_trans = trans_create(TRANS_MODE_UNIX, 8192 * 4, 8192 * 4);
     g_api_lis_trans->is_term = g_is_term;
-    g_snprintf(port, 255, CHANSRV_API_STR, g_display_num);
+    g_snprintf(port, sizeof(port), CHANSRV_API_STR, g_getuid(), g_display_num);
     g_api_lis_trans->trans_conn_in = my_api_trans_conn_in;
     error = trans_listen(g_api_lis_trans, port);
 

sesman/chansrv/sound.c

@@ -1894,11 +1894,11 @@ sound_sndsrvr_source_data_in(struct trans *trans)
 static int
 sound_start_source_listener(void)
 {
-    char port[1024];
+    char port[XRDP_SOCKETS_MAXPATH];
 
     g_audio_l_trans_in = trans_create(TRANS_MODE_UNIX, 128 * 1024, 8192);
     g_audio_l_trans_in->is_term = g_is_term;
-    g_snprintf(port, 255, CHANSRV_PORT_IN_STR, g_display_num);
+    g_snprintf(port, sizeof(port), CHANSRV_PORT_IN_STR, g_getuid(), g_display_num);
     g_audio_l_trans_in->trans_conn_in = sound_sndsrvr_source_conn_in;
     if (trans_listen(g_audio_l_trans_in, port) != 0)
     {
@@ -1913,11 +1913,11 @@ sound_start_source_listener(void)
 static int
 sound_start_sink_listener(void)
 {
-    char port[1024];
+    char port[XRDP_SOCKETS_MAXPATH];
 
     g_audio_l_trans_out = trans_create(TRANS_MODE_UNIX, 128 * 1024, 8192);
     g_audio_l_trans_out->is_term = g_is_term;
-    g_snprintf(port, 255, CHANSRV_PORT_OUT_STR, g_display_num);
+    g_snprintf(port, sizeof(port), CHANSRV_PORT_OUT_STR, g_getuid(), g_display_num);
     g_audio_l_trans_out->trans_conn_in = sound_sndsrvr_sink_conn_in;
     if (trans_listen(g_audio_l_trans_out, port) != 0)
     {