From eadae51341dbf80c83e827bb4011e80dfcbc6927 Mon Sep 17 00:00:00 2001 From: Frank Lichtenheld Date: Fri, 3 Oct 2025 12:06:02 +0200 Subject: [PATCH] platform: Do not assume uid_t/gid_t are signed uid_t/gid_t are int on many platform but unsigned on at least Linux. So rewrite the code in a way that does not make any assumptions about the types. Mainly this means storing the information whether the value is valid in a separate bool and not in the value itself. Note that this changes the return behavior of platform_{user,group}_get but a review of the callers determined that this makes no actual difference. Change-Id: Ie6b4c41d13544d5ba71d441cc794c7abd12408f3 Signed-off-by: Frank Lichtenheld Acked-by: MaxF Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1206 Message-Id: <20251003100602.375062-1-frank@lichtenheld.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg33266.html Signed-off-by: Gert Doering --- src/openvpn/manage.c | 31 ++++++++++++------------------- src/openvpn/manage.h | 4 ++-- src/openvpn/platform.c | 26 +++++++++++--------------- src/openvpn/platform.h | 28 ++-------------------------- src/openvpn/socket.c | 2 +- src/openvpn/socket.h | 2 +- 6 files changed, 29 insertions(+), 64 deletions(-) diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 5a41a0f85..1cb5c6388 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -1847,25 +1847,26 @@ man_new_connection_post(struct management *man, const char *description) static bool man_verify_unix_peer_uid_gid(struct management *man, const socket_descriptor_t sd) { - if (socket_defined(sd) && (man->settings.client_uid != -1 || man->settings.client_gid != -1)) + if (socket_defined(sd) && (man->settings.user.user_valid || man->settings.group.group_valid)) { static const char err_prefix[] = "MANAGEMENT: unix domain socket client connection rejected --"; - int uid, gid; + uid_t uid; + gid_t gid; if (unix_socket_get_peer_uid_gid(man->connection.sd_cli, &uid, &gid)) { - if (man->settings.client_uid != -1 && man->settings.client_uid != uid) + if (man->settings.user.user_valid && man->settings.user.uid != uid) { msg(D_MANAGEMENT, "%s UID of socket peer (%d) doesn't match required value (%d) as given by --management-client-user", - err_prefix, uid, man->settings.client_uid); + err_prefix, uid, man->settings.user.uid); return false; } - if (man->settings.client_gid != -1 && man->settings.client_gid != gid) + if (man->settings.group.group_valid && man->settings.group.gid != gid) { msg(D_MANAGEMENT, "%s GID of socket peer (%d) doesn't match required value (%d) as given by --management-client-group", - err_prefix, gid, man->settings.client_gid); + err_prefix, gid, man->settings.group.gid); return false; } } @@ -2542,8 +2543,6 @@ man_settings_init(struct man_settings *ms, const char *addr, const char *port, CLEAR(*ms); ms->flags = flags; - ms->client_uid = -1; - ms->client_gid = -1; /* * Get username/password @@ -2553,27 +2552,21 @@ man_settings_init(struct man_settings *ms, const char *addr, const char *port, get_user_pass(&ms->up, pass_file, "Management", GET_USER_PASS_PASSWORD_ONLY); } +#if UNIX_SOCK_SUPPORT /* * lookup client UID/GID if specified */ if (client_user) { - struct platform_state_user s; - platform_user_get(client_user, &s); - ms->client_uid = platform_state_user_uid(&s); - msg(D_MANAGEMENT, "MANAGEMENT: client_uid=%d", ms->client_uid); - ASSERT(ms->client_uid >= 0); + ASSERT(platform_user_get(client_user, &ms->user)); + msg(D_MANAGEMENT, "MANAGEMENT: client_uid=%d", ms->user.uid); } if (client_group) { - struct platform_state_group s; - platform_group_get(client_group, &s); - ms->client_gid = platform_state_group_gid(&s); - msg(D_MANAGEMENT, "MANAGEMENT: client_gid=%d", ms->client_gid); - ASSERT(ms->client_gid >= 0); + ASSERT(platform_group_get(client_group, &ms->group)); + msg(D_MANAGEMENT, "MANAGEMENT: client_gid=%d", ms->group.gid); } -#if UNIX_SOCK_SUPPORT if (ms->flags & MF_UNIX_SOCK) { sockaddr_unix_init(&ms->local_unix, addr); diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index bff96d328..a31eb0681 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -242,14 +242,14 @@ struct man_settings struct addrinfo *local; #if UNIX_SOCK_SUPPORT struct sockaddr_un local_unix; + struct platform_state_user user; + struct platform_state_group group; #endif bool management_over_tunnel; struct user_pass up; int log_history_cache; int echo_buffer_size; int state_buffer_size; - int client_uid; - int client_gid; /* flags for handling the management interface "signal" command */ #define MANSIG_IGNORE_USR1_HUP (1u << 0) diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c index 880d14e4b..a1ffdafb9 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -39,7 +39,7 @@ #include "platform.h" -#if _WIN32 +#ifdef _WIN32 #include #endif @@ -79,12 +79,10 @@ platform_chroot(const char *path) bool platform_user_get(const char *username, struct platform_state_user *state) { - bool ret = false; CLEAR(*state); if (username) { #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) - state->uid = -1; const struct passwd *pw = getpwnam(username); if (!pw) { @@ -93,23 +91,23 @@ platform_user_get(const char *username, struct platform_state_user *state) else { state->uid = pw->pw_uid; + state->user_valid = true; } state->username = username; - ret = true; #else /* if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) */ msg(M_FATAL, "cannot get UID for user %s -- platform lacks getpwname() or setuid() system calls", username); #endif } - return ret; + return state->user_valid; } static void platform_user_set(const struct platform_state_user *state) { #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) - if (state->username && state->uid >= 0) + if (state->username && state->user_valid) { if (setuid(state->uid)) { @@ -125,12 +123,10 @@ platform_user_set(const struct platform_state_user *state) bool platform_group_get(const char *groupname, struct platform_state_group *state) { - bool ret = false; CLEAR(*state); if (groupname) { #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) - state->gid = -1; const struct group *gr = getgrnam(groupname); if (!gr) { @@ -139,23 +135,23 @@ platform_group_get(const char *groupname, struct platform_state_group *state) else { state->gid = gr->gr_gid; + state->group_valid = true; } state->groupname = groupname; - ret = true; #else /* if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) */ msg(M_FATAL, "cannot get GID for group %s -- platform lacks getgrnam() or setgid() system calls", groupname); #endif } - return ret; + return state->group_valid; } static void platform_group_set(const struct platform_state_group *state) { #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) - if (state->groupname && state->gid >= 0) + if (state->groupname && state->group_valid) { if (setgid(state->gid)) { @@ -237,13 +233,13 @@ platform_user_group_set(const struct platform_state_user *user_state, * new_uid/new_gid defaults to -1, which will not make * libcap-ng change the UID/GID unless configured */ - if (group_state->groupname && group_state->gid >= 0) + if (group_state->groupname && group_state->group_valid) { - new_gid = group_state->gid; + new_gid = (int)group_state->gid; } - if (user_state->username && user_state->uid >= 0) + if (user_state->username && user_state->user_valid) { - new_uid = user_state->uid; + new_uid = (int)user_state->uid; } /* Prepare capabilities before dropping UID/GID */ diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h index f1a2b0186..0cb25f5c4 100644 --- a/src/openvpn/platform.h +++ b/src/openvpn/platform.h @@ -64,9 +64,8 @@ struct platform_state_user #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) const char *username; uid_t uid; -#else - int dummy; #endif + bool user_valid; }; /* Get/Set GID of process */ @@ -76,9 +75,8 @@ struct platform_state_group #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) const char *groupname; gid_t gid; -#else - int dummy; #endif + bool group_valid; }; bool platform_user_get(const char *username, struct platform_state_user *state); @@ -89,28 +87,6 @@ void platform_user_group_set(const struct platform_state_user *user_state, const struct platform_state_group *group_state, struct context *c); -/* - * Extract UID or GID - */ - -static inline int -platform_state_user_uid(const struct platform_state_user *s) -{ -#if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) - return s->uid; -#endif - return -1; -} - -static inline int -platform_state_group_gid(const struct platform_state_group *s) -{ -#if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) - return s->gid; -#endif - return -1; -} - void platform_chroot(const char *path); void platform_nice(int niceval); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 5fcf82037..f71d891e7 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -3082,7 +3082,7 @@ socket_delete_unix(const struct sockaddr_un *local) } bool -unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, int *uid, int *gid) +unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, uid_t *uid, gid_t *gid) { #ifdef HAVE_GETPEEREID uid_t u; diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index e45981fa3..2c79a1164 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -371,7 +371,7 @@ const char *sockaddr_unix_name(const struct sockaddr_un *local, const char *null void socket_delete_unix(const struct sockaddr_un *local); -bool unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, int *uid, int *gid); +bool unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, uid_t *uid, gid_t *gid); #endif /* if UNIX_SOCK_SUPPORT */ -- 2.47.3