| Message ID | 20230306053346.796992-2-selva.nair@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,1/2] Do not save pointer to 'struct passwd' returned by getpwnam etc. | expand |
Hi, On Mon, Mar 06, 2023 at 12:33:46AM -0500, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@gmail.com> > > - When management-client-group is in use, allow access if any of > the supplementary groups of the user matches the specified group. > > Currently only the effective gid of the peer socket is checked > which is normally the primary group of user. As unprivileged users > have no easy way of changing the effective gid of a process, > group based access control is of very limited use without this change. I'm not really convinced this is a good way forward - it's yet another extra function for a very niche use case, as in "the code has been like this for 100 years, and nobody has ever used it". What you do is also not exactly "the user connecting has this gid at the time of connection" but "the uid reported by getpeerid() has this group listed in /etc/groups", which will be the same in many cases, but is still checking something different. I would propose to extend the documentation with "this only checks the primary gid at time of connection, if this does not work for your use case, put the socket into a directory with the right gid and mode 750 and do not use that openvpn option" - as you wrote in the github ticket. > - Also accept if uid = 0 irrespective of the group. This part is fine for me, as it follows the traditional unix way of "root may pass". gert
Hi, On Mon, Mar 6, 2023 at 3:24 AM Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Mon, Mar 06, 2023 at 12:33:46AM -0500, selva.nair@gmail.com wrote: > > From: Selva Nair <selva.nair@gmail.com> > > > > - When management-client-group is in use, allow access if any of > > the supplementary groups of the user matches the specified group. > > > > Currently only the effective gid of the peer socket is checked > > which is normally the primary group of user. As unprivileged users > > have no easy way of changing the effective gid of a process, > > group based access control is of very limited use without this change. > > I'm not really convinced this is a good way forward - it's yet another > extra function for a very niche use case, as in "the code has been like > this for 100 years, and nobody has ever used it". > > What you do is also not exactly "the user connecting has this gid at the > time of connection" but "the uid reported by getpeerid() has this group > listed in /etc/groups", which will be the same in many cases, but is > still checking something different. > I was of the same opinion when I first saw issue #264, but I think the original intent of "--management-client-group has to be what I've implemented or something similar. Docs refer to it as group (not effective gid of the socket), and restricting based on gid reported by getpeerid is kind of useless. Why use "group" if it can't be used to restrict access to a set of users using a supplementary group... I would propose to extend the documentation with "this only checks the > primary gid at time of connection, if this does not work for your use > case, put the socket into a directory with the right gid and mode 750 > and do not use that openvpn option" - as you wrote in the github ticket. > It's impossible to save that option by documenting. In that case, let's just make it a no-op and document that this option has no effect. And suggest the above for how to achieve group-based access control.. > > - Also accept if uid = 0 irrespective of the group. > > This part is fine for me, as it follows the traditional unix way of > "root may pass". > I'm not keen on touching some code and ignore that a closely related code stays crippled (IMO). Unless we can make --management-client-group a no-op as mentioned above. As you say this is an obscure option -- we can leave it "as is". Selva
Hi FTR, I just noticed that the patch is missing an endgrent() call: On Mon, Mar 6, 2023 at 12:33 AM <selva.nair@gmail.com> wrote: > > + struct group *gr = getgrent(); > + char **members = NULL; > + while (gr) > + { > + if (gr->gr_gid == gid) > + { > + /* found the group -- check user is a member */ > + members = gr->gr_mem; > + for (char *s = *members; s && !ret; s++) > + { > + ret = !strcmp(s, pw->pw_name); > + } > + break; /* out of the while loop */ > + } > + gr = getgrent(); > + } > endgrent(); > +#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */ > + return ret; > +} > + > Will delay v2 depending on the fate of this patch. Selva
diff --git a/configure.ac b/configure.ac index 67f680b2..979903a8 100644 --- a/configure.ac +++ b/configure.ac @@ -659,7 +659,7 @@ AC_CHECK_FUNCS([ \ daemon chroot getpwnam setuid nice system dup dup2 \ syslog openlog mlockall getrlimit getgrnam setgid \ setgroups flock readv writev time gettimeofday \ - setsid chdir \ + setsid chdir getpwuid getgrent \ chsize ftruncate execve getpeereid basename dirname access \ epoll_create strsep \ ]) diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index db88e347..73dc7d57 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -1771,6 +1771,42 @@ man_new_connection_post(struct management *man, const char *description) } #if UNIX_SOCK_SUPPORT +/** + * Return true if user uid is a member of group gid. + * On error or lack of platform support, we return false. + */ + +static bool +is_user_in_group(uid_t uid, gid_t gid) +{ + bool ret = false; +#if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) + struct passwd *pw = getpwuid(uid); + if (!pw) + { + return ret; + } + + struct group *gr = getgrent(); + char **members = NULL; + while (gr) + { + if (gr->gr_gid == gid) + { + /* found the group -- check user is a member */ + members = gr->gr_mem; + for (char *s = *members; s && !ret; s++) + { + ret = !strcmp(s, pw->pw_name); + } + break; /* out of the while loop */ + } + gr = getgrent(); + } +#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */ + return ret; +} + static bool man_verify_unix_peer_uid_gid(struct management *man, const socket_descriptor_t sd) { @@ -1780,13 +1816,18 @@ man_verify_unix_peer_uid_gid(struct management *man, const socket_descriptor_t s int uid, gid; if (unix_socket_get_peer_uid_gid(man->connection.sd_cli, &uid, &gid)) { + if (uid == 0) /* accept if root */ + { + return true; + } if (man->settings.client_uid != -1 && man->settings.client_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); return false; } - if (man->settings.client_gid != -1 && man->settings.client_gid != gid) + if (man->settings.client_gid != -1 && man->settings.client_gid != gid + && !is_user_in_group(uid, man->settings.client_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);