Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Thu, Feb 16, 2017 at 7:19 PM, Eric W. Biedermanwrote: > > Added a few more relevant mailing-lists to the CC list. > > Aleksa Sarai writes: > >> One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to >> disable setgroups on a per user namespace basis") is that because >> setgroups(2) no longer works in user namespaces it doesn't make any >> sense to be returning weird group IDs that the process cannot do >> anything with. > > This code works the same weather or not setgroups is enabled. > >> This change, along with the other changes made to require unprivileged >> users to always disable setgroups(2), means that userspace programs such >> as apt break inside rootless containers. While this change does change >> the userspace ABI, any userspace program that has to deal with >> getgroups(2) would have to filter out these "fake" group IDs anyway. >> This just makes it so that less applications will have to handle this >> broken API. > > Is it broken? Unless I am mistaken if we have a 16bit getgroups > call and we 32bit group ids getgroups it behaves exactly the same way. > > The value we is (u16)-2. The traditional linux group id for this > purpose. > > In all other contexts the best we can do for applications has been to > return the user id or group id that says the value you are looking for > does not fit in this context. Which makes me suspect this is not the > right solution for getgroups. > > I don't know why apt breaks. You have not described that. Perhaps apt > is seeing something misconfigured and complaining properly. > > I can be persauded but I need a better argument than this change makes > one applicaiton work for me. We have already code that takes into account the returned overflowed UID from inside, what we are also doing is from outside to let services/containers run with dynamic unique UIDs between 0xEF00 and 0xFFEF which is a nice replacement feature for the "nobody" mess from outside, but also it allows to map the dynamic same IDs to themselves and everything else inside is "nobody". So the code that wants to check if the user is in real mapped GIDs has also to check what /proc/sys/kernel/overflowgid means. Thanks! > > >> Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user >> namespace basis") >> Cc: "Eric W. Biederman" >> Cc: >> Signed-off-by: Aleksa Sarai >> --- >> kernel/groups.c | 25 ++--- >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/kernel/groups.c b/kernel/groups.c >> index 8dd7a61b7115..ebd01fff37d6 100644 >> --- a/kernel/groups.c >> +++ b/kernel/groups.c >> @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, >> const struct group_info *group_info) >> { >> struct user_namespace *user_ns = current_user_ns(); >> - int i; >> + int i, j = 0; >> unsigned int count = group_info->ngroups; >> >> for (i = 0; i < count; i++) { >> + kgid_t kgid = group_info->gid[i]; >> gid_t gid; >> - gid = from_kgid_munged(user_ns, group_info->gid[i]); >> - if (put_user(gid, grouplist+i)) >> + >> + /* >> + * Don't return unmapped gids, since there's nothing userspace >> + * can do about them and they are very confusing -- since >> + * setgroups(2) is disabled in user namespaces. >> + */ >> + if (!kgid_has_mapping(user_ns, kgid)) >> + continue; >> + >> + gid = from_kgid(user_ns, kgid); >> + if (put_user(gid, grouplist+j)) >> return -EFAULT; >> + j++; >> } >> - return 0; >> + return j; >> } >> >> /* fill a group_info from a user-space array - it must be allocated already >> */ >> @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t >> __user *, grouplist) >> i = -EINVAL; >> goto out; >> } >> - if (groups_to_user(grouplist, cred->group_info)) { >> - i = -EFAULT; >> + >> + i = groups_to_user(grouplist, cred->group_info); >> + if (i < 0) >> goto out; >> - } >> } >> out: >> return i; -- tixxdz
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Thu, Feb 16, 2017 at 7:19 PM, Eric W. Biederman wrote: > > Added a few more relevant mailing-lists to the CC list. > > Aleksa Sarai writes: > >> One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to >> disable setgroups on a per user namespace basis") is that because >> setgroups(2) no longer works in user namespaces it doesn't make any >> sense to be returning weird group IDs that the process cannot do >> anything with. > > This code works the same weather or not setgroups is enabled. > >> This change, along with the other changes made to require unprivileged >> users to always disable setgroups(2), means that userspace programs such >> as apt break inside rootless containers. While this change does change >> the userspace ABI, any userspace program that has to deal with >> getgroups(2) would have to filter out these "fake" group IDs anyway. >> This just makes it so that less applications will have to handle this >> broken API. > > Is it broken? Unless I am mistaken if we have a 16bit getgroups > call and we 32bit group ids getgroups it behaves exactly the same way. > > The value we is (u16)-2. The traditional linux group id for this > purpose. > > In all other contexts the best we can do for applications has been to > return the user id or group id that says the value you are looking for > does not fit in this context. Which makes me suspect this is not the > right solution for getgroups. > > I don't know why apt breaks. You have not described that. Perhaps apt > is seeing something misconfigured and complaining properly. > > I can be persauded but I need a better argument than this change makes > one applicaiton work for me. We have already code that takes into account the returned overflowed UID from inside, what we are also doing is from outside to let services/containers run with dynamic unique UIDs between 0xEF00 and 0xFFEF which is a nice replacement feature for the "nobody" mess from outside, but also it allows to map the dynamic same IDs to themselves and everything else inside is "nobody". So the code that wants to check if the user is in real mapped GIDs has also to check what /proc/sys/kernel/overflowgid means. Thanks! > > >> Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user >> namespace basis") >> Cc: "Eric W. Biederman" >> Cc: >> Signed-off-by: Aleksa Sarai >> --- >> kernel/groups.c | 25 ++--- >> 1 file changed, 18 insertions(+), 7 deletions(-) >> >> diff --git a/kernel/groups.c b/kernel/groups.c >> index 8dd7a61b7115..ebd01fff37d6 100644 >> --- a/kernel/groups.c >> +++ b/kernel/groups.c >> @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, >> const struct group_info *group_info) >> { >> struct user_namespace *user_ns = current_user_ns(); >> - int i; >> + int i, j = 0; >> unsigned int count = group_info->ngroups; >> >> for (i = 0; i < count; i++) { >> + kgid_t kgid = group_info->gid[i]; >> gid_t gid; >> - gid = from_kgid_munged(user_ns, group_info->gid[i]); >> - if (put_user(gid, grouplist+i)) >> + >> + /* >> + * Don't return unmapped gids, since there's nothing userspace >> + * can do about them and they are very confusing -- since >> + * setgroups(2) is disabled in user namespaces. >> + */ >> + if (!kgid_has_mapping(user_ns, kgid)) >> + continue; >> + >> + gid = from_kgid(user_ns, kgid); >> + if (put_user(gid, grouplist+j)) >> return -EFAULT; >> + j++; >> } >> - return 0; >> + return j; >> } >> >> /* fill a group_info from a user-space array - it must be allocated already >> */ >> @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t >> __user *, grouplist) >> i = -EINVAL; >> goto out; >> } >> - if (groups_to_user(grouplist, cred->group_info)) { >> - i = -EFAULT; >> + >> + i = groups_to_user(grouplist, cred->group_info); >> + if (i < 0) >> goto out; >> - } >> } >> out: >> return i; -- tixxdz
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Fri, Feb 17, 2017 at 12:53 PM, Aleksa Sarai wrote: > One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to > disable setgroups on a per user namespace basis") is that because > setgroups(2) no longer works in user namespaces it doesn't make any > sense to be returning weird group IDs that the process cannot do > anything with. > >>> > >>> > > > >> bool DropPrivileges() > >> { > >> /* ... */ > >> // Verify that the user isn't still in any supplementary groups > > > > But the user *is* still in a supplementary group. Your proposed > > change would break the intent of this code. > > I was about to say that "being in an unmapped supplementary group does > not count as privileges", but decided to test it first and realised that > this is not true? How is this not a blatant security vulnerability? whether they're mapped is irrelevant. if you make modifications to the FS, the permission check counts against the meaning of the ids in the parent namespaces. otherwise your argument is like saying "i mapped my UID to 0 in this new usernamespace, so how is that not a blatant security vuln". if you started out with access to the group, then keeping that access doesn't grant you any new privs you didn't already have. > % touch somefile > % chmod 660 somefile > % sudo chown root:wheel somefile > % unshare -r > % cat somefile > % # no EACCES... you didn't show `id` before the `unshare`. if you're already in the wheel group, what you've shown here is not a bug. -mike
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Fri, Feb 17, 2017 at 12:53 PM, Aleksa Sarai wrote: > One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to > disable setgroups on a per user namespace basis") is that because > setgroups(2) no longer works in user namespaces it doesn't make any > sense to be returning weird group IDs that the process cannot do > anything with. > >>> > >>> > > > >> bool DropPrivileges() > >> { > >> /* ... */ > >> // Verify that the user isn't still in any supplementary groups > > > > But the user *is* still in a supplementary group. Your proposed > > change would break the intent of this code. > > I was about to say that "being in an unmapped supplementary group does > not count as privileges", but decided to test it first and realised that > this is not true? How is this not a blatant security vulnerability? whether they're mapped is irrelevant. if you make modifications to the FS, the permission check counts against the meaning of the ids in the parent namespaces. otherwise your argument is like saying "i mapped my UID to 0 in this new usernamespace, so how is that not a blatant security vuln". if you started out with access to the group, then keeping that access doesn't grant you any new privs you didn't already have. > % touch somefile > % chmod 660 somefile > % sudo chown root:wheel somefile > % unshare -r > % cat somefile > % # no EACCES... you didn't show `id` before the `unshare`. if you're already in the wheel group, what you've shown here is not a bug. -mike
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. >>> >>> > >> bool DropPrivileges() >> { >> /* ... */ >> // Verify that the user isn't still in any supplementary groups > > But the user *is* still in a supplementary group. Your proposed > change would break the intent of this code. I was about to say that "being in an unmapped supplementary group does not count as privileges", but decided to test it first and realised that this is not true? How is this not a blatant security vulnerability? I understand the `chmod 707` usecase and that being able to *block* access is useful with supplementary groups, but I would _never_ have guessed that *unmapped* supplementary groups *allow you to have access to files*. And not only would I have never guessed that to be the case, this makes the fact that getgroups(2) returns 65534 even _more_ concerning -- how on earth is a userspace process meant to know what secret privileges it has? How can it make sane decisions about security with this setup? % touch somefile % chmod 660 somefile % sudo chown root:wheel somefile % unshare -r % cat somefile % # no EACCES... Please someone tell me this is a regression and it's not meant to be this way... -- Aleksa Sarai Software Engineer (Containers) SUSE Linux GmbH https://www.cyphar.com/
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. >>> >>> > >> bool DropPrivileges() >> { >> /* ... */ >> // Verify that the user isn't still in any supplementary groups > > But the user *is* still in a supplementary group. Your proposed > change would break the intent of this code. I was about to say that "being in an unmapped supplementary group does not count as privileges", but decided to test it first and realised that this is not true? How is this not a blatant security vulnerability? I understand the `chmod 707` usecase and that being able to *block* access is useful with supplementary groups, but I would _never_ have guessed that *unmapped* supplementary groups *allow you to have access to files*. And not only would I have never guessed that to be the case, this makes the fact that getgroups(2) returns 65534 even _more_ concerning -- how on earth is a userspace process meant to know what secret privileges it has? How can it make sane decisions about security with this setup? % touch somefile % chmod 660 somefile % sudo chown root:wheel somefile % unshare -r % cat somefile % # no EACCES... Please someone tell me this is a regression and it's not meant to be this way... -- Aleksa Sarai Software Engineer (Containers) SUSE Linux GmbH https://www.cyphar.com/
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Fri, Feb 17, 2017 at 12:44 AM, Aleksa Saraiwrote: >>> One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to >>> disable setgroups on a per user namespace basis") is that because >>> setgroups(2) no longer works in user namespaces it doesn't make any >>> sense to be returning weird group IDs that the process cannot do >>> anything with. >> >> > bool DropPrivileges() > { > /* ... */ > // Verify that the user isn't still in any supplementary groups But the user *is* still in a supplementary group. Your proposed change would break the intent of this code.
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
On Fri, Feb 17, 2017 at 12:44 AM, Aleksa Sarai wrote: >>> One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to >>> disable setgroups on a per user namespace basis") is that because >>> setgroups(2) no longer works in user namespaces it doesn't make any >>> sense to be returning weird group IDs that the process cannot do >>> anything with. >> >> > bool DropPrivileges() > { > /* ... */ > // Verify that the user isn't still in any supplementary groups But the user *is* still in a supplementary group. Your proposed change would break the intent of this code.
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. This code works the same weather or not setgroups is enabled. Yes. Sorry, I explain the reasoning for this better below. But the point is that "65534" is ambiguous in this context and can lead to confusion for a userspace program. However, you have a point that when setgroups is enabled then this code will effectively hide groups which the process may wish to drop. I'm not sure what you would like to do in this instance -- but I'd prefer if we first agree on whether this is a worthwhile issue to solve in the kernel or if userspace should just hack around it (which I've already partially done[1]). This change, along with the other changes made to require unprivileged users to always disable setgroups(2), means that userspace programs such as apt break inside rootless containers. While this change does change the userspace ABI, any userspace program that has to deal with getgroups(2) would have to filter out these "fake" group IDs anyway. This just makes it so that less applications will have to handle this broken API. Is it broken? Unless I am mistaken if we have a 16bit getgroups call and we 32bit group ids getgroups it behaves exactly the same way. When I say "broken" what I mean is that getgroups(2) is telling userspace that "you are a member of group 65534". But you now have two different (confusing) cases that can result: 1. That group is not mapped. Which means that anything that application assumes about getgroups(2) returning sane results now needs to be cross-checked with /proc/self/uid_map and checking whether setgroups will actually work. Apt is an example of such a program -- it attempts to drop all privileges using setgroups(2) because getgroups(2) tells it that has some supplementary groups. 2. That group _is_ mapped. Now the application has no way of knowing whether it actually is in group 65534 (aside from experimenting with files and trying to see what its _actual_ groups are). Note that in both cases it is not simple to be completely sure whether the 65534 that getgroups(2) returned actually means "unmapped" or The value we is (u16)-2. The traditional linux group id for this purpose. Okay, but 65534 is a valid group ID as well. So while in principle it's reserved for this purpose, returning an "invalid" group ID that is actually a valid value is just confusing. In all other contexts the best we can do for applications has been to return the user id or group id that says the value you are looking for does not fit in this context. Which makes me suspect this is not the right solution for getgroups. While with getuid() and getgid() I understand this logic (though I strongly feel there should be an EUNMAPPED, I understand why it doesn't exist). With get{u,g}id() there is an implicit statement that overflow_{u,g}id should be semantically equivalent to "unmapped {U,G}ID". However, this doesn't (IMO) apply to getgroups(2). getgroups(2) tells you what groups you are a member of, and there is a semantic difference to "you are a member of group #overflow_gid" and "you are in an unmapped group". I don't know why apt breaks. You have not described that. Perhaps apt is seeing something misconfigured and complaining properly. I investigated this quite a bit while trying to get rootless containers to work with runC. After reading the code again, apt actually does two things: 1. Unconditionally does setgroups(1, _gid). This obviously breaks in rootless containers, but can be fixed. 2. It then has some checks to verify that it has dropped privileges correctly. Now, you _can_ disable these checks but I would prefer not to (many other programs have similar code to make sure they are safely dropping privileges). If you look in apt-pkg/contrib/fileutl.cc you'll see this [abbreviated] function: bool DropPrivileges() { /* ... */ // Verify that the user isn't still in any supplementary groups long const ngroups_max = sysconf(_SC_NGROUPS_MAX); std::unique_ptrgidlist(new gid_t[ngroups_max]); if (unlikely(gidlist == NULL)) return _error->Error("Allocation of a list of size %lu for getgroups failed", ngroups_max); ssize_t gidlist_nr; if ((gidlist_nr = getgroups(ngroups_max, gidlist.get())) < 0) return _error->Errno("getgroups", "Could not get new groups (%lu)", ngroups_max); for (ssize_t i = 0; i < gidlist_nr; ++i) if (gidlist[i] != pw->pw_gid) return _error->Error("Could not switch group, user %s is still in group %d", toUser.c_str(), gidlist[i]); /* ... */ } Which will fail in rootless containers, and there's nothing you can do as a user other than disable the
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. This code works the same weather or not setgroups is enabled. Yes. Sorry, I explain the reasoning for this better below. But the point is that "65534" is ambiguous in this context and can lead to confusion for a userspace program. However, you have a point that when setgroups is enabled then this code will effectively hide groups which the process may wish to drop. I'm not sure what you would like to do in this instance -- but I'd prefer if we first agree on whether this is a worthwhile issue to solve in the kernel or if userspace should just hack around it (which I've already partially done[1]). This change, along with the other changes made to require unprivileged users to always disable setgroups(2), means that userspace programs such as apt break inside rootless containers. While this change does change the userspace ABI, any userspace program that has to deal with getgroups(2) would have to filter out these "fake" group IDs anyway. This just makes it so that less applications will have to handle this broken API. Is it broken? Unless I am mistaken if we have a 16bit getgroups call and we 32bit group ids getgroups it behaves exactly the same way. When I say "broken" what I mean is that getgroups(2) is telling userspace that "you are a member of group 65534". But you now have two different (confusing) cases that can result: 1. That group is not mapped. Which means that anything that application assumes about getgroups(2) returning sane results now needs to be cross-checked with /proc/self/uid_map and checking whether setgroups will actually work. Apt is an example of such a program -- it attempts to drop all privileges using setgroups(2) because getgroups(2) tells it that has some supplementary groups. 2. That group _is_ mapped. Now the application has no way of knowing whether it actually is in group 65534 (aside from experimenting with files and trying to see what its _actual_ groups are). Note that in both cases it is not simple to be completely sure whether the 65534 that getgroups(2) returned actually means "unmapped" or The value we is (u16)-2. The traditional linux group id for this purpose. Okay, but 65534 is a valid group ID as well. So while in principle it's reserved for this purpose, returning an "invalid" group ID that is actually a valid value is just confusing. In all other contexts the best we can do for applications has been to return the user id or group id that says the value you are looking for does not fit in this context. Which makes me suspect this is not the right solution for getgroups. While with getuid() and getgid() I understand this logic (though I strongly feel there should be an EUNMAPPED, I understand why it doesn't exist). With get{u,g}id() there is an implicit statement that overflow_{u,g}id should be semantically equivalent to "unmapped {U,G}ID". However, this doesn't (IMO) apply to getgroups(2). getgroups(2) tells you what groups you are a member of, and there is a semantic difference to "you are a member of group #overflow_gid" and "you are in an unmapped group". I don't know why apt breaks. You have not described that. Perhaps apt is seeing something misconfigured and complaining properly. I investigated this quite a bit while trying to get rootless containers to work with runC. After reading the code again, apt actually does two things: 1. Unconditionally does setgroups(1, _gid). This obviously breaks in rootless containers, but can be fixed. 2. It then has some checks to verify that it has dropped privileges correctly. Now, you _can_ disable these checks but I would prefer not to (many other programs have similar code to make sure they are safely dropping privileges). If you look in apt-pkg/contrib/fileutl.cc you'll see this [abbreviated] function: bool DropPrivileges() { /* ... */ // Verify that the user isn't still in any supplementary groups long const ngroups_max = sysconf(_SC_NGROUPS_MAX); std::unique_ptr gidlist(new gid_t[ngroups_max]); if (unlikely(gidlist == NULL)) return _error->Error("Allocation of a list of size %lu for getgroups failed", ngroups_max); ssize_t gidlist_nr; if ((gidlist_nr = getgroups(ngroups_max, gidlist.get())) < 0) return _error->Errno("getgroups", "Could not get new groups (%lu)", ngroups_max); for (ssize_t i = 0; i < gidlist_nr; ++i) if (gidlist[i] != pw->pw_gid) return _error->Error("Could not switch group, user %s is still in group %d", toUser.c_str(), gidlist[i]); /* ... */ } Which will fail in rootless containers, and there's nothing you can do as a user other than disable the checks and
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
Added a few more relevant mailing-lists to the CC list. Aleksa Saraiwrites: > One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to > disable setgroups on a per user namespace basis") is that because > setgroups(2) no longer works in user namespaces it doesn't make any > sense to be returning weird group IDs that the process cannot do > anything with. This code works the same weather or not setgroups is enabled. > This change, along with the other changes made to require unprivileged > users to always disable setgroups(2), means that userspace programs such > as apt break inside rootless containers. While this change does change > the userspace ABI, any userspace program that has to deal with > getgroups(2) would have to filter out these "fake" group IDs anyway. > This just makes it so that less applications will have to handle this > broken API. Is it broken? Unless I am mistaken if we have a 16bit getgroups call and we 32bit group ids getgroups it behaves exactly the same way. The value we is (u16)-2. The traditional linux group id for this purpose. In all other contexts the best we can do for applications has been to return the user id or group id that says the value you are looking for does not fit in this context. Which makes me suspect this is not the right solution for getgroups. I don't know why apt breaks. You have not described that. Perhaps apt is seeing something misconfigured and complaining properly. I can be persauded but I need a better argument than this change makes one applicaiton work for me. Eric > Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user > namespace basis") > Cc: "Eric W. Biederman" > Cc: > Signed-off-by: Aleksa Sarai > --- > kernel/groups.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/kernel/groups.c b/kernel/groups.c > index 8dd7a61b7115..ebd01fff37d6 100644 > --- a/kernel/groups.c > +++ b/kernel/groups.c > @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, > const struct group_info *group_info) > { > struct user_namespace *user_ns = current_user_ns(); > - int i; > + int i, j = 0; > unsigned int count = group_info->ngroups; > > for (i = 0; i < count; i++) { > + kgid_t kgid = group_info->gid[i]; > gid_t gid; > - gid = from_kgid_munged(user_ns, group_info->gid[i]); > - if (put_user(gid, grouplist+i)) > + > + /* > + * Don't return unmapped gids, since there's nothing userspace > + * can do about them and they are very confusing -- since > + * setgroups(2) is disabled in user namespaces. > + */ > + if (!kgid_has_mapping(user_ns, kgid)) > + continue; > + > + gid = from_kgid(user_ns, kgid); > + if (put_user(gid, grouplist+j)) > return -EFAULT; > + j++; > } > - return 0; > + return j; > } > > /* fill a group_info from a user-space array - it must be allocated already > */ > @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t > __user *, grouplist) > i = -EINVAL; > goto out; > } > - if (groups_to_user(grouplist, cred->group_info)) { > - i = -EFAULT; > + > + i = groups_to_user(grouplist, cred->group_info); > + if (i < 0) > goto out; > - } > } > out: > return i;
Re: [PATCH] groups: don't return unmapped gids in getgroups(2)
Added a few more relevant mailing-lists to the CC list. Aleksa Sarai writes: > One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to > disable setgroups on a per user namespace basis") is that because > setgroups(2) no longer works in user namespaces it doesn't make any > sense to be returning weird group IDs that the process cannot do > anything with. This code works the same weather or not setgroups is enabled. > This change, along with the other changes made to require unprivileged > users to always disable setgroups(2), means that userspace programs such > as apt break inside rootless containers. While this change does change > the userspace ABI, any userspace program that has to deal with > getgroups(2) would have to filter out these "fake" group IDs anyway. > This just makes it so that less applications will have to handle this > broken API. Is it broken? Unless I am mistaken if we have a 16bit getgroups call and we 32bit group ids getgroups it behaves exactly the same way. The value we is (u16)-2. The traditional linux group id for this purpose. In all other contexts the best we can do for applications has been to return the user id or group id that says the value you are looking for does not fit in this context. Which makes me suspect this is not the right solution for getgroups. I don't know why apt breaks. You have not described that. Perhaps apt is seeing something misconfigured and complaining properly. I can be persauded but I need a better argument than this change makes one applicaiton work for me. Eric > Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user > namespace basis") > Cc: "Eric W. Biederman" > Cc: > Signed-off-by: Aleksa Sarai > --- > kernel/groups.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/kernel/groups.c b/kernel/groups.c > index 8dd7a61b7115..ebd01fff37d6 100644 > --- a/kernel/groups.c > +++ b/kernel/groups.c > @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, > const struct group_info *group_info) > { > struct user_namespace *user_ns = current_user_ns(); > - int i; > + int i, j = 0; > unsigned int count = group_info->ngroups; > > for (i = 0; i < count; i++) { > + kgid_t kgid = group_info->gid[i]; > gid_t gid; > - gid = from_kgid_munged(user_ns, group_info->gid[i]); > - if (put_user(gid, grouplist+i)) > + > + /* > + * Don't return unmapped gids, since there's nothing userspace > + * can do about them and they are very confusing -- since > + * setgroups(2) is disabled in user namespaces. > + */ > + if (!kgid_has_mapping(user_ns, kgid)) > + continue; > + > + gid = from_kgid(user_ns, kgid); > + if (put_user(gid, grouplist+j)) > return -EFAULT; > + j++; > } > - return 0; > + return j; > } > > /* fill a group_info from a user-space array - it must be allocated already > */ > @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t > __user *, grouplist) > i = -EINVAL; > goto out; > } > - if (groups_to_user(grouplist, cred->group_info)) { > - i = -EFAULT; > + > + i = groups_to_user(grouplist, cred->group_info); > + if (i < 0) > goto out; > - } > } > out: > return i;
[PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. This change, along with the other changes made to require unprivileged users to always disable setgroups(2), means that userspace programs such as apt break inside rootless containers. While this change does change the userspace ABI, any userspace program that has to deal with getgroups(2) would have to filter out these "fake" group IDs anyway. This just makes it so that less applications will have to handle this broken API. Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") Cc: "Eric W. Biederman"Cc: Signed-off-by: Aleksa Sarai --- kernel/groups.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/kernel/groups.c b/kernel/groups.c index 8dd7a61b7115..ebd01fff37d6 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, const struct group_info *group_info) { struct user_namespace *user_ns = current_user_ns(); - int i; + int i, j = 0; unsigned int count = group_info->ngroups; for (i = 0; i < count; i++) { + kgid_t kgid = group_info->gid[i]; gid_t gid; - gid = from_kgid_munged(user_ns, group_info->gid[i]); - if (put_user(gid, grouplist+i)) + + /* +* Don't return unmapped gids, since there's nothing userspace +* can do about them and they are very confusing -- since +* setgroups(2) is disabled in user namespaces. +*/ + if (!kgid_has_mapping(user_ns, kgid)) + continue; + + gid = from_kgid(user_ns, kgid); + if (put_user(gid, grouplist+j)) return -EFAULT; + j++; } - return 0; + return j; } /* fill a group_info from a user-space array - it must be allocated already */ @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t __user *, grouplist) i = -EINVAL; goto out; } - if (groups_to_user(grouplist, cred->group_info)) { - i = -EFAULT; + + i = groups_to_user(grouplist, cred->group_info); + if (i < 0) goto out; - } } out: return i; -- 2.11.0
[PATCH] groups: don't return unmapped gids in getgroups(2)
One thing overlooked by commit 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") is that because setgroups(2) no longer works in user namespaces it doesn't make any sense to be returning weird group IDs that the process cannot do anything with. This change, along with the other changes made to require unprivileged users to always disable setgroups(2), means that userspace programs such as apt break inside rootless containers. While this change does change the userspace ABI, any userspace program that has to deal with getgroups(2) would have to filter out these "fake" group IDs anyway. This just makes it so that less applications will have to handle this broken API. Fixes: 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis") Cc: "Eric W. Biederman" Cc: Signed-off-by: Aleksa Sarai --- kernel/groups.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/kernel/groups.c b/kernel/groups.c index 8dd7a61b7115..ebd01fff37d6 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -41,16 +41,27 @@ static int groups_to_user(gid_t __user *grouplist, const struct group_info *group_info) { struct user_namespace *user_ns = current_user_ns(); - int i; + int i, j = 0; unsigned int count = group_info->ngroups; for (i = 0; i < count; i++) { + kgid_t kgid = group_info->gid[i]; gid_t gid; - gid = from_kgid_munged(user_ns, group_info->gid[i]); - if (put_user(gid, grouplist+i)) + + /* +* Don't return unmapped gids, since there's nothing userspace +* can do about them and they are very confusing -- since +* setgroups(2) is disabled in user namespaces. +*/ + if (!kgid_has_mapping(user_ns, kgid)) + continue; + + gid = from_kgid(user_ns, kgid); + if (put_user(gid, grouplist+j)) return -EFAULT; + j++; } - return 0; + return j; } /* fill a group_info from a user-space array - it must be allocated already */ @@ -177,10 +188,10 @@ SYSCALL_DEFINE2(getgroups, int, gidsetsize, gid_t __user *, grouplist) i = -EINVAL; goto out; } - if (groups_to_user(grouplist, cred->group_info)) { - i = -EFAULT; + + i = groups_to_user(grouplist, cred->group_info); + if (i < 0) goto out; - } } out: return i; -- 2.11.0