Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

Andy Lutomirski
2014-11-17T18:47:23Z

On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>>> setgroups at all. In particular, processes with a set of supplementary
>>> groups cannot further drop permissions without obtaining elevated
>>> permissions first.
>>
>> Has anyone put any thought into how this will interact with
>> POSIX ACLs? I don't see that anywhere in the discussion.
>
> That means that user namespaces are a problem, too, and we need to fix
> it. Or we should add some control to turn unprivileged user namespace
> creation on and off and document that turning it on defeats POSIX ACLs
> with a group entry that is more restrictive than the other entry.
>

This is a significant enough issue that I posted it to oss-security:

http://www.openwall.com/lists/oss-security/2014/11/17/19

It's not at all obvious to me how to fix it. We could disallow userns
creation of any supplementary groups don't match fsuid, or we could
keep negative-only groups around in the userns.

It may be worth adding a sysctl to change the behavior, too. IMO it's
absurd to use groups to deny permissions that are otherwise available.

--Andy

> --Andy
>
>>
>> Tizen takes advantage of the fact you can't drop groups. If
>> a process can drop itself out of groups without privilege
>> it can circumvent the system security policy.
>>
>> Back when the LSM was introduced a choice was made between
>> authoritative hooks (which would have allowed this sort of thing)
>> and restrictive hooks (which would not). Authoritative hooks were
>> rejected because they would have "broken Linux". I hope that the
>> people who spoke up then will speak up now.
>>
>> This is going to introduce a whole class of vulnerabilities.
>> Don't even think of arguing that the reduction in use of privilege
>> will make up for that. Developers have enough trouble with the
>> difference between setuid() and seteuid() to expect them to use
>> group dropping properly.
>>
>>>
>>> Allow unprivileged processes to call setgroups with a subset of their
>>> current groups; only require CAP_SETGID to add a group the process does
>>> not currently have.
>>>
>>> The kernel already maintains the list of supplementary group IDs in
>>> sorted order, and setgroups already needs to sort the new list, so this
>>> just requires a linear comparison of the two sorted lists.
>>>
>>> This moves the CAP_SETGID test from setgroups into set_current_groups.
>>>
>>> Tested via the following test program:
>>>
>>> #include <err.h>
>>> #include <grp.h>
>>> #include <stdio.h>
>>> #include <sys/types.h>
>>> #include <unistd.h>
>>>
>>> void run_id(void)
>>> {
>>> pid_t p = fork();
>>> switch (p) {
>>> case -1:
>>> err(1, "fork");
>>> case 0:
>>> execl("/usr/bin/id", "id", NULL);
>>> err(1, "exec");
>>> default:
>>> if (waitpid(p, NULL, 0) < 0)
>>> err(1, "waitpid");
>>> }
>>> }
>>>
>>> int main(void)
>>> {
>>> gid_t list1[] = { 1, 2, 3, 4, 5 };
>>> gid_t list2[] = { 2, 3, 4 };
>>> run_id();
>>> if (setgroups(5, list1) < 0)
>>> err(1, "setgroups 1");
>>> run_id();
>>> if (setresgid(1, 1, 1) < 0)
>>> err(1, "setresgid");
>>> if (setresuid(1, 1, 1) < 0)
>>> err(1, "setresuid");
>>> run_id();
>>> if (setgroups(3, list2) < 0)
>>> err(1, "setgroups 2");
>>> run_id();
>>> if (setgroups(5, list1) < 0)
>>> err(1, "setgroups 3");
>>> run_id();
>>>
>>> return 0;
>>> }
>>>
>>> Without this patch, the test program gets EPERM from the second
>>> setgroups call, after dropping root privileges. With this patch, the
>>> test program successfully drops groups 1 and 5, but then gets EPERM from
>>> the third setgroups call, since that call attempts to add groups the
>>> process does not currently have.
>>>
>>> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
>>> ---
>>> kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>>> kernel/uid16.c | 2 --
>>> 2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/groups.c b/kernel/groups.c
>>> index f0667e7..fe7367d 100644
>>> --- a/kernel/groups.c
>>> +++ b/kernel/groups.c
>>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>> return 0;
>>> }
>>>
>>> +/* Compare two sorted group lists; return true if the first is a subset of the
>>> + * second.
>>> + */
>>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
>>> +{
>>> + unsigned int i, j;
>>> +
>>> + for (i = 0, j = 0; i < g1->ngroups; i++) {
>>> + kgid_t gid1 = GROUP_AT(g1, i);
>>> + kgid_t gid2;
>>> + for (; j < g2->ngroups; j++) {
>>> + gid2 = GROUP_AT(g2, j);
>>> + if (gid_lte(gid1, gid2))
>>> + break;
>>> + }
>>> + if (j >= g2->ngroups || !gid_eq(gid1, gid2))
>>> + return false;
>>> + j++;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> /**
>>> * set_groups_sorted - Change a group subscription in a set of credentials
>>> * @new: The newly prepared set of credentials to alter
>>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>>> {
>>> struct cred *new;
>>>
>>> + groups_sort(group_info);
>>> new = prepare_creds();
>>> if (!new)
>>> return -ENOMEM;
>>> + if (!ns_capable(current_user_ns(), CAP_SETGID)
>>> + && !is_subset(group_info, new->group_info)) {
>>> + abort_creds(new);
>>> + return -EPERM;
>>> + }
>>>
>>> - set_groups(new, group_info);
>>> + set_groups_sorted(new, group_info);
>>> return commit_creds(new);
>>> }
>>>
>>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>>> struct group_info *group_info;
>>> int retval;
>>>
>>> - if (!ns_capable(current_user_ns(), CAP_SETGID))
>>> - return -EPERM;
>>> if ((unsigned)gidsetsize > NGROUPS_MAX)
>>> return -EINVAL;
>>>
>>> diff --git a/kernel/uid16.c b/kernel/uid16.c
>>> index 602e5bb..b27e167 100644
>>> --- a/kernel/uid16.c
>>> +++ b/kernel/uid16.c
>>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>>> struct group_info *group_info;
>>> int retval;
>>>
>>> - if (!ns_capable(current_user_ns(), CAP_SETGID))
>>> - return -EPERM;
>>> if ((unsigned)gidsetsize > NGROUPS_MAX)
>>> return -EINVAL;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-api" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC

--
Andy Lutomirski
AMA Capital Management, LLC