"Serge E. Hallyn" <se...@hallyn.com> writes: > On Tue, Oct 13, 2020 at 05:17:36PM +0200, Giuseppe Scrivano wrote: >> "Serge E. Hallyn" <se...@hallyn.com> writes: >> >> > On Mon, Oct 12, 2020 at 07:05:10PM +0200, Giuseppe Scrivano wrote: >> >> Josh Triplett <j...@joshtriplett.org> writes: >> >> >> >> > On Fri, Oct 09, 2020 at 11:26:06PM -0500, Serge E. Hallyn wrote: >> >> >> > 3. Find a way to allow setgroups() in a user namespace while keeping >> >> >> > in mind the case of groups used for negative access control. >> >> >> > This was suggested by Josh Triplett and Geoffrey Thomas. Their >> >> >> > idea was to >> >> >> > investigate adding a prctl() to allow setgroups() to be called in >> >> >> > a user >> >> >> > namespace at the cost of restricting paths to the most restrictive >> >> >> > permission. So if something is 0707 it needs to be treated as if >> >> >> > it's 0000 >> >> >> > even though the caller is not in its owning group which is used >> >> >> > for negative >> >> >> > access control (how these new semantics will interact with ACLs >> >> >> > will also >> >> >> > need to be looked into). >> >> >> >> >> >> I should probably think this through more, but for this problem, would >> >> >> it >> >> >> not suffice to add a new prevgroups grouplist to the struct cred, maybe >> >> >> struct group_info *locked_groups, and every time an unprivileged task >> >> >> creates >> >> >> a new user namespace, add all its current groups to this list? >> >> > >> >> > So, effectively, you would be allowed to drop permissions, but >> >> > locked_groups would still be checked for restrictions? >> >> > >> >> > That seems like it'd introduce a new level of complexity (a new facet of >> >> > permission) to manage. Not opposed, but it does seem more complex than >> >> > just opting out of using groups for negative permissions. >> >> >> >> I have played with something similar in the past. At that time I've >> >> discussed it only privately with Eric and we agreed it wasn't worth the >> >> extra complexity: >> >> >> >> https://github.com/giuseppe/linux/commit/7e0701b389c497472d11fab8570c153a414050af >> > >> > Hi, you linked the setgroups patch, but do you also have a link to the >> > attempt which you deemed was not worth it? >> >> it was just part of a private discussion; but was 4 years ago so we can >> probably revisit and accept the additional complexity since setgroups() >> is still an issue with user namespaces. >> >> >> >> instead of a prctl, I've added a new mode to /proc/PID/setgroups that >> >> allows setgroups in a userns locking the current gids. >> >> >> >> What do you think about using /proc/PID/setgroups instead of a new >> >> prctl()? >> > >> > It's better than not having it, but two concerns - >> > >> > 1. some userspace, especially testsuites, could become confused by the fact >> > that they can't drop groups no matter how hard they try, since these will >> > all >> > still show up as regular groups. >> >> I forgot to send a link to a second patch :-) that completes the feature: >> https://github.com/giuseppe/linux/commit/1c5fe726346b216293a527719e64f34e6297f0c2 >> >> When the new mode is used, the gids that are not known in the userns do >> not show up in userspace. > > Ah, right - and of course those gids better not be mapped into the namespace > :) > > But so, this is the patch you said you agreed was not worth the extra > complexity?
yes, these two patches are what looked too complex at that time. The problem still exists though, we could perhaps reconsider if the extra-complexity is acceptable to address it. Regards, Giuseppe
