[PATCH] groups: don't return unmapped gids in getgroups(2)

[PATCH] groups: don't return unmapped gids in getgroups(2)

[Aleksa Sarai]

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" <ebiederm@xmission.com>
Cc: <dev@opencontainers.org>
Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 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

Re: [PATCH] groups: don't return unmapped gids in getgroups(2)

[Eric W. Biederman]

Added a few more relevant mailing-lists to the CC list.

Aleksa Sarai <asarai@suse.de> 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" <ebiederm@xmission.com>
> Cc: <dev@opencontainers.org>
> Signed-off-by: Aleksa Sarai <asarai@suse.de>
> ---
>  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)

[Aleksa Sarai]

>> 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, &some_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<gid_t[]> 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 modify the apt source code. 
There are almost certainly more examples (searching github found things 
like qmail also appear to have some similar logic, and I think some 
components of Varnish might too).

Also, I found that this "secure coding standards" document recommends 
this sort of practice when writing software that needs to drop 
privileges[2]. I'm sure there exists plenty of userspace software which 
implements this kind of logic.

The reason you haven't seen bug reports (or people shouting on the ML) 
for this is that people are still not really running rootless containers 
yet -- the work I'm doing in runC is what's uncovering these sorts of 
issues.

> I can be persauded but I need a better argument than this change makes
> one applicaiton work for me.

I need to get better at writing commit messages, sorry about that. As I 
mentioned above, the biggest issue is the ambiguity of 65534. The fact 
that apt doesn't react in a sane way is a symptom of that issue, and 
there are almost certainly more cases of programs that act in this way.

[1]: https://github.com/cyphar/remainroot
[2]: 
https://www.securecoding.cert.org/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH] groups: don't return unmapped gids in getgroups(2)

[Andy Lutomirski]

On Fri, Feb 17, 2017 at 12:44 AM, Aleksa Sarai <asarai@suse.de> 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)

[Aleksa Sarai]

 >>>> 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)

[Mike Frysinger]

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)

[Djalal Harouni]

On Thu, Feb 16, 2017 at 7:19 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Added a few more relevant mailing-lists to the CC list.
>
> Aleksa Sarai <asarai@suse.de> 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 0x0000EF00
and 0x0000FFEF 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" <ebiederm@xmission.com>
>> Cc: <dev@opencontainers.org>
>> Signed-off-by: Aleksa Sarai <asarai@suse.de>
>> ---
>>  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