[CRIU] [PATCH] security: check additional groups

[Ruslan Kuprieiev]

On 02.07.2014 15:21, Ruslan Kuprieiev wrote:
> On 02.07.2014 15:17, Ruslan Kuprieiev wrote:
>> On 02.07.2014 14:58, Pavel Emelyanov wrote:
>>>> -void restrict_uid(unsigned int uid, unsigned int gid)
>>>> +int restrict_uid(unsigned int uid, unsigned int gid)
>>>>   {
>>>> -    pr_info("Restrict C/R with %u:%u uid\n", uid, gid);
>>>> +    struct passwd *pwd;
>>>> +    unsigned int buf[NGROUPS_MAX];
>>>> +    int nbuf;
>>>> +
>>>> +    pr_info("Restrict C/R with %u:%u uid:gid\n", uid, gid);
>>>>       cr_uid = uid;
>>>>       cr_gid = gid;
>>>> +
>>>> +    pwd = getpwuid(uid);
>>>> +    if (!pwd) {
>>>> +        pr_perror("Can't get password file entry");
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    nbuf = NGROUPS_MAX;
>>>> +    if (getgrouplist(pwd->pw_name, pwd->pw_gid, buf, &nbuf) < 0) {
>>> There's no need in getting the caller's groups. It's enough just
>>> to check the group it currently lives in.
>>
>> User's current group may not match all gids of the process. Just as 
>> it was with
>> that game. When user's group was 1000, but process had (1000,20,20).
>>
>
> Oh, are you suggesting to check that process grouplist contains 
> caller's gid?
>

But this method would be bad on restore.

Doing as it is in the patch, we can pretty much trust images and not 
check that
they are owned by root, as user isn't able to fix groups field in the 
image and gain
more access than he already has.

>> And we should check that user isn't restoring process which has more 
>> groups
>> than user already has, shouldn't we?
>>
>>>> +        pr_perror("Can't get group list");
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    cr_ngroups = nbuf;
>>>> +    cr_groups = xmalloc(cr_ngroups*sizeof(*cr_groups));
>>>> +    if (!cr_groups)
>>>> +        return -1;
>>>> +
>>>> +    memcpy(cr_groups, buf, cr_ngroups*sizeof(*cr_groups));
>>>> +
>>>> +    return 0;
>>>>   }
>>
>