From 84a0befd3030df582bcd1e2d3e9fa075a67b3467 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Tue, 3 Oct 2023 10:34:41 +0100 Subject: [PATCH 1/4] Add getgrouplist() detection functionality Defines the macro HAVE_GETGROUPLIST if getgrouplist() is available, and defines the type passed to the GID array as GETGROUPS_T --- configure.ac | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 414ce3b9..2f3d6938 100644 --- a/configure.ac +++ b/configure.ac @@ -217,7 +217,10 @@ AM_COND_IF([DEVEL_DEBUG], AC_SEARCH_LIBS([setusercontext], [util]) # Define HAVE_XXXXX macros for some system functions -AC_CHECK_FUNCS([setusercontext]) +AC_CHECK_FUNCS([setusercontext getgrouplist]) + +# The type used by getgrouplist() is the same type used by getgroups() +AC_TYPE_GETGROUPS # Don't fail without working nasm if rfxcodec is not enabled if test "x$enable_rfxcodec" != xyes; then From cf677da22c8e64c6651d2a460eab12ea4fa709ea Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 2 Oct 2023 11:04:47 +0100 Subject: [PATCH 2/4] Add getgrouplist() support to os_calls On enterprise systems, using getgrouplist() (if available) is more efficient than iterating over the members of the group, and is also more likely to work --- common/os_calls.c | 87 ++++++++++++++++++++++++++++++++++++++--------- common/os_calls.h | 9 +++++ 2 files changed, 79 insertions(+), 17 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index a58051a5..1afeb570 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -3573,42 +3573,95 @@ g_getgroup_info(const char *groupname, int *gid) } /*****************************************************************************/ -/* returns error */ -/* if zero is returned, then ok is set */ -/* does not work in win32 */ +#ifdef HAVE_GETGROUPLIST +int +g_check_user_in_group(const char *username, int gid, int *ok) +{ + int rv = 1; + struct passwd *pwd_1 = getpwnam(username); + if (pwd_1 != NULL) + { + // Get number of groups for user + // + // Some implementations of getgrouplist() (i.e. muslc) don't + // allow ngroups to be <1 on entry + int ngroups = 1; + GETGROUPS_T dummy; + getgrouplist(username, pwd_1->pw_gid, &dummy, &ngroups); + + if (ngroups > 0) // Should always be true + { + GETGROUPS_T *grouplist; + grouplist = (GETGROUPS_T *)malloc(ngroups * sizeof(grouplist[0])); + if (grouplist != NULL) + { + // Now get the actual groups. The number of groups returned + // by this call is not necessarily the same as the number + // returned by the first call. + int allocgroups = ngroups; + getgrouplist(username, pwd_1->pw_gid, grouplist, &ngroups); + ngroups = MIN(ngroups, allocgroups); + + rv = 0; + *ok = 0; + + int i; + for (i = 0 ; i < ngroups; ++i) + { + if (grouplist[i] == (GETGROUPS_T)gid) + { + *ok = 1; + break; + } + } + free(grouplist); + } + } + } + return rv; +} +/*****************************************************************************/ +#else // HAVE_GETGROUPLIST int g_check_user_in_group(const char *username, int gid, int *ok) { #if defined(_WIN32) return 1; #else - struct group *groups; int i; - groups = getgrgid(gid); - - if (groups == 0) + struct passwd *pwd_1 = getpwnam(username); + struct group *groups = getgrgid(gid); + if (pwd_1 == NULL || groups == NULL) { return 1; } - *ok = 0; - i = 0; - - while (0 != groups->gr_mem[i]) + if (pwd_1->pw_gid == gid) { - if (0 == g_strcmp(groups->gr_mem[i], username)) - { - *ok = 1; - break; - } + *ok = 1; + } + else + { + *ok = 0; + i = 0; - i++; + while (0 != groups->gr_mem[i]) + { + if (0 == g_strcmp(groups->gr_mem[i], username)) + { + *ok = 1; + break; + } + + i++; + } } return 0; #endif } +#endif // HAVE_GETGROUPLIST /*****************************************************************************/ /* returns the time since the Epoch (00:00:00 UTC, January 1, 1970), diff --git a/common/os_calls.h b/common/os_calls.h index 2374df3e..2d5c18e6 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -323,6 +323,15 @@ int g_getuser_info_by_name(const char *username, int *uid, int *gid, int g_getuser_info_by_uid(int uid, char **username, int *gid, char **shell, char **dir, char **gecos); int g_getgroup_info(const char *groupname, int *gid); +/** + * Checks whether a user is in the specified group + * @param username Name of user + * @param gid GID of group + * @param[out] ok Whether user is in group + * @return Non-zero if a system error occurred. In this instance OK is not set + * + * Primary group of username is also checked + */ int g_check_user_in_group(const char *username, int gid, int *ok); int g_time1(void); int g_time2(void); From 5837deae049ca8d9658862f23616da2da4988577 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 5 Oct 2023 12:25:40 +0100 Subject: [PATCH 3/4] access_login_allowed: Remove primary group check This check is now performed within g_check_user_in_group() --- sesman/libsesman/sesman_access.c | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/sesman/libsesman/sesman_access.c b/sesman/libsesman/sesman_access.c index eb98be90..4f0973ae 100644 --- a/sesman/libsesman/sesman_access.c +++ b/sesman/libsesman/sesman_access.c @@ -40,7 +40,6 @@ int access_login_allowed(const struct config_security *cfg_sec, const char *user) { - int gid; int ok; if ((0 == g_strncmp(user, "root", 5)) && (0 == cfg_sec->allow_root)) @@ -56,18 +55,6 @@ access_login_allowed(const struct config_security *cfg_sec, const char *user) return 1; } - if (0 != g_getuser_info_by_name(user, 0, &gid, 0, 0, 0)) - { - LOG(LOG_LEVEL_ERROR, "Cannot read user info! - login denied"); - return 0; - } - - if (cfg_sec->ts_users == gid) - { - LOG(LOG_LEVEL_DEBUG, "ts_users is user's primary group"); - return 1; - } - if (0 != g_check_user_in_group(user, cfg_sec->ts_users, &ok)) { LOG(LOG_LEVEL_ERROR, "Cannot read group info! - login denied"); @@ -89,7 +76,6 @@ int access_login_mng_allowed(const struct config_security *cfg_sec, const char *user) { - int gid; int ok; if ((0 == g_strncmp(user, "root", 5)) && (0 == cfg_sec->allow_root)) @@ -106,18 +92,6 @@ access_login_mng_allowed(const struct config_security *cfg_sec, return 1; } - if (0 != g_getuser_info_by_name(user, 0, &gid, 0, 0, 0)) - { - LOG(LOG_LEVEL_ERROR, "[MNG] Cannot read user info! - login denied"); - return 0; - } - - if (cfg_sec->ts_admins == gid) - { - LOG(LOG_LEVEL_INFO, "[MNG] ts_users is user's primary group"); - return 1; - } - if (0 != g_check_user_in_group(user, cfg_sec->ts_admins, &ok)) { LOG(LOG_LEVEL_ERROR, "[MNG] Cannot read group info! - login denied"); From cf5c2718afe0da0c07988cf0ba6455cdb875593d Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 4 Oct 2023 11:43:01 +0100 Subject: [PATCH 4/4] Update logging in sesman access control Improve the built-in access checks for sesman/sesexec:- - Group existence is checked for at login-time rather than program start time - The name of the group is now included in the message Also, check for UID == 0 when checking for root, rather than just checking the name (which might be an alias) --- sesman/libsesman/sesman_access.c | 180 ++++++++++++++++++++++--------- sesman/libsesman/sesman_config.c | 63 ++++------- sesman/libsesman/sesman_config.h | 6 +- 3 files changed, 151 insertions(+), 98 deletions(-) diff --git a/sesman/libsesman/sesman_access.c b/sesman/libsesman/sesman_access.c index 4f0973ae..defb488f 100644 --- a/sesman/libsesman/sesman_access.c +++ b/sesman/libsesman/sesman_access.c @@ -28,47 +28,142 @@ #include #endif +#include + #include "arch.h" #include "sesman_access.h" #include "sesman_config.h" #include "log.h" #include "os_calls.h" -#include "string_calls.h" + +/******************************************************************************/ +/** + * Root user login check + * + * @param cfg_sec Sesman security config + * @param user user name + * @return 0 if user is root and root accesses are not allowed + */ +static int +root_login_check(const struct config_security *cfg_sec, + const char *user) +{ + int rv = 0; + if (cfg_sec->allow_root) + { + rv = 1; + } + else + { + // Check the UID of the user isn't 0 + int uid; + if (g_getuser_info_by_name(user, &uid, NULL, NULL, NULL, NULL) != 0) + { + LOG(LOG_LEVEL_ERROR, "Can't get UID for user %s", user); + } + else if (0 == uid) + { + LOG(LOG_LEVEL_ERROR, + "ROOT login attempted, but root login is disabled"); + } + else + { + rv = 1; + } + } + return rv; +} + +/******************************************************************************/ +/** + * Common access control for group checks + * @param group Group name + * @param param Where group comes from (e.g. "TerminalServerUsers") + * @param always_check_group 0 if a missing group allows a login + * @param user Username + * @return != 0 if the access is allowed */ + +static int +access_login_allowed_common(const char *group, const char *param, + int always_check_group, + const char *user) +{ + int rv = 0; + int gid; + int ok; + + if (group == NULL || group[0] == '\0') + { + /* Group is not defined. Default access depends on whether + * we must have the group or not */ + if (always_check_group) + { + LOG(LOG_LEVEL_ERROR, + "%s group is not defined. Access denied for %s", + param, user); + } + else + { + LOG(LOG_LEVEL_INFO, + "%s group is not defined. Access granted for %s", + param, user); + rv = 1; + } + } + else if (g_getgroup_info(group, &gid) != 0) + { + /* Group is defined but doesn't exist. Default access depends + * on whether we must have the group or not */ + if (always_check_group) + { + LOG(LOG_LEVEL_ERROR, + "%s group %s doesn't exist. Access denied for %s", + param, group, user); + } + else + { + LOG(LOG_LEVEL_INFO, + "%s group %s doesn't exist. Access granted for %s", + param, group, user); + rv = 1; + } + } + else if (0 != g_check_user_in_group(user, gid, &ok)) + { + LOG(LOG_LEVEL_ERROR, "Error checking %s group %s. " + "Access denied for %s", param, group, user); + } + else if (!ok) + { + LOG(LOG_LEVEL_ERROR, "User %s is not in %s group %s. Access denied", + user, param, group); + } + else + { + LOG(LOG_LEVEL_INFO, "User %s is in %s group %s. Access granted", + user, param, group); + rv = 1; + } + + return rv; +} /******************************************************************************/ int access_login_allowed(const struct config_security *cfg_sec, const char *user) { - int ok; + int rv = 0; - if ((0 == g_strncmp(user, "root", 5)) && (0 == cfg_sec->allow_root)) + if (root_login_check(cfg_sec, user)) { - LOG(LOG_LEVEL_WARNING, - "ROOT login attempted, but root login is disabled"); - return 0; + rv = access_login_allowed_common(cfg_sec->ts_users, + "TerminalServerUsers", + cfg_sec->ts_always_group_check, + user); } - if ((0 == cfg_sec->ts_users_enable) && (0 == cfg_sec->ts_always_group_check)) - { - LOG(LOG_LEVEL_INFO, "Terminal Server Users group is disabled, allowing authentication"); - return 1; - } - - if (0 != g_check_user_in_group(user, cfg_sec->ts_users, &ok)) - { - LOG(LOG_LEVEL_ERROR, "Cannot read group info! - login denied"); - return 0; - } - - if (ok) - { - return 1; - } - - LOG(LOG_LEVEL_INFO, "login denied for user %s", user); - - return 0; + return rv; } /******************************************************************************/ @@ -76,34 +171,15 @@ int access_login_mng_allowed(const struct config_security *cfg_sec, const char *user) { - int ok; + int rv = 0; - if ((0 == g_strncmp(user, "root", 5)) && (0 == cfg_sec->allow_root)) + if (root_login_check(cfg_sec, user)) { - LOG(LOG_LEVEL_WARNING, - "[MNG] ROOT login attempted, but root login is disabled"); - return 0; + rv = access_login_allowed_common(cfg_sec->ts_admins, + "TerminalServerAdmins", + 1, + user); } - if (0 == cfg_sec->ts_admins_enable) - { - LOG(LOG_LEVEL_INFO, "[MNG] Terminal Server Admin group is disabled, " - "allowing authentication"); - return 1; - } - - if (0 != g_check_user_in_group(user, cfg_sec->ts_admins, &ok)) - { - LOG(LOG_LEVEL_ERROR, "[MNG] Cannot read group info! - login denied"); - return 0; - } - - if (ok) - { - return 1; - } - - LOG(LOG_LEVEL_INFO, "[MNG] login denied for user %s", user); - - return 0; + return rv; } diff --git a/sesman/libsesman/sesman_config.c b/sesman/libsesman/sesman_config.c index b18bfdb4..568bdf6d 100644 --- a/sesman/libsesman/sesman_config.c +++ b/sesman/libsesman/sesman_config.c @@ -297,8 +297,8 @@ config_read_security(int file, struct config_security *sc, struct list *param_v) { int i; - int gid; - char *buf; + const char *buf; + const char *value; list_clear(param_v); list_clear(param_n); @@ -306,49 +306,46 @@ config_read_security(int file, struct config_security *sc, /* setting defaults */ sc->allow_root = 0; sc->login_retry = 3; - sc->ts_users_enable = 0; - sc->ts_admins_enable = 0; sc->restrict_outbound_clipboard = 0; sc->restrict_inbound_clipboard = 0; sc->allow_alternate_shell = 1; sc->xorg_no_new_privileges = 1; + sc->ts_users = g_strdup(""); + sc->ts_admins = g_strdup(""); file_read_section(file, SESMAN_CFG_SECURITY, param_n, param_v); for (i = 0; i < param_n->count; i++) { - buf = (char *)list_get_item(param_n, i); + buf = (const char *)list_get_item(param_n, i); + value = (const char *)list_get_item(param_v, i); if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_ALLOW_ROOT)) { - sc->allow_root = g_text2bool((char *)list_get_item(param_v, i)); + sc->allow_root = g_text2bool(value); } if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_LOGIN_RETRY)) { - sc->login_retry = g_atoi((char *)list_get_item(param_v, i)); + sc->login_retry = g_atoi(value); } if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_USR_GROUP)) { - if (g_getgroup_info((char *)list_get_item(param_v, i), &gid) == 0) - { - sc->ts_users_enable = 1; - sc->ts_users = gid; - } + g_free(sc->ts_users); + sc->ts_users = NULL; + sc->ts_users = g_strdup(value); } if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_ADM_GROUP)) { - if (g_getgroup_info((char *)list_get_item(param_v, i), &gid) == 0) - { - sc->ts_admins_enable = 1; - sc->ts_admins = gid; - } + g_free(sc->ts_admins); + sc->ts_admins = NULL; + sc->ts_admins = g_strdup(value); } if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_ALWAYSGROUPCHECK)) { - sc->ts_always_group_check = g_text2bool((char *)list_get_item(param_v, i)); + sc->ts_always_group_check = g_text2bool(value); } if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_RESTRICT_OUTBOUND_CLIPBOARD)) @@ -382,13 +379,13 @@ config_read_security(int file, struct config_security *sc, if (0 == g_strcasecmp(buf, SESMAN_CFG_SEC_ALLOW_ALTERNATE_SHELL)) { sc->allow_alternate_shell = - g_text2bool((char *)list_get_item(param_v, i)); + g_text2bool(value); } 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)); + g_text2bool(value); } } @@ -687,28 +684,8 @@ config_dump(struct config_sesman *config) restrict_s, sizeof(restrict_s)); g_writeln(" RestrictInboundClipboard: %s", restrict_s); - - g_printf( " TSUsersGroup: "); - if (sc->ts_users_enable) - { - g_printf("%d", sc->ts_users); - } - else - { - g_printf("(not defined)"); - } - g_writeln("%s", ""); - - g_printf( " TSAdminsGroup: "); - if (sc->ts_admins_enable) - { - g_printf("%d", sc->ts_admins); - } - else - { - g_printf("(not defined)"); - } - g_writeln("%s", ""); + g_writeln(" TSUsersGroup: %s", sc->ts_users); + g_writeln(" TSAdminsGroup: %s", sc->ts_admins); /* Xorg */ @@ -764,6 +741,8 @@ config_free(struct config_sesman *cs) list_delete(cs->xorg_params); list_delete(cs->env_names); list_delete(cs->env_values); + g_free(cs->sec.ts_users); + g_free(cs->sec.ts_admins); g_free(cs); } } diff --git a/sesman/libsesman/sesman_config.h b/sesman/libsesman/sesman_config.h index 30bd79d6..9dd6672b 100644 --- a/sesman/libsesman/sesman_config.h +++ b/sesman/libsesman/sesman_config.h @@ -70,14 +70,12 @@ struct config_security * @var ts_users * @brief Terminal Server Users group */ - int ts_users_enable; - int ts_users; + char *ts_users; /** * @var ts_admins * @brief Terminal Server Administrators group */ - int ts_admins_enable; - int ts_admins; + char *ts_admins; /** * @var ts_always_group_check * @brief if the Groups are not found deny access