[CRIU] [PATCH] security: check additional groups

[Pavel Emelyanov]

On 07/02/2014 04:30 PM, Ruslan Kuprieiev wrote:
> 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.

OK, I see. Yes, this makes sense. I will post comments to patch soon.