[CRIU] [PATCH] security: check_ids - return true if [se]?[ug]id is the same as task id

[Pavel Emelyanov]

On 06/17/2014 10:06 AM, Ruslan Kuprieiev wrote:
> On 16.06.2014 18:18, Pavel Emelyanov wrote:
>> On 06/16/2014 04:10 PM, Ruslan Kuprieiev wrote:
>>> On 16.06.2014 14:23, Pavel Emelyanov wrote:
>>>> On 06/16/2014 09:55 AM, Ruslan Kuprieiev wrote:
>>>>> And again sorry for necromailing=).
>>>>>
>>>>> After a bit more thinking, here is what I figured out.
>>>>>
>>>>> Current check if(crid == id && crid == eid && crid == sid) doesn't work
>>>>> right.
>>>>> Andrew was able to show it in "[CRIU] RPC support for --shell-job
>>>>> missing on restore"
>>>>> thread.
>>>>> I wasn't able to make it work only with crid available.
>>>>>
>>>>> Andrew also suggested that we should be able to dump/restore task if we
>>>>> are able to kill it. He also attached a function from the kernel:
>>>>>
>>>>> static int kill_ok_by_cred(struct task_struct *t)
>>>>> {
>>>>>            const struct cred *cred = current_cred();
>>>>>            const struct cred *tcred = __task_cred(t);
>>>>>
>>>>>            if (uid_eq(cred->euid, tcred->suid) ||
>>>>>                uid_eq(cred->euid, tcred->uid)  ||
>>>>>                uid_eq(cred->uid,  tcred->suid) ||
>>>>>                uid_eq(cred->uid,  tcred->uid))
>>>>>                    return 1;
>>>>>
>>>>>            if (ns_capable(tcred->user_ns, CAP_KILL))
>>>>>                    return 1;
>>>>>
>>>>>            return 0;
>>>>> }
>>>> Not sure this would work for us. If I'm user 12 dump a task, then
>>>> edit its creds to have id == 12, suid == 0 and caps == <all>, then
>>>> I'm effectively allowed to restore a process with root caps.
>>> Oh, I see. You are talking about manipulating images?
>> Exactly.
>>
>>> Yes, that looks like a potential big security issue.
>>>
>>>> This is bad.
>>>>
>>>> AFAIU the problem we're trying to fix is called "we cannot dump and
>>>> restore the suid-ed process", isn't it?
>>> Yes, kind of. Process is sgid-ed, but belongs to user's gid == process
>>> gid. And that user
>>> can't dump his own process. It doesn't look right.
>>>
>>> Maybe it's time to think about securing images? Maybe, encrypt it with smth?
>> I have two possibilities in my head. The first is to check, that
>> images belong to a directory the user that requests restore doesn't
>> have write access to. In that case we probably can imply, that the
>> images were not modified.
> 
> But what if user wants to write images to dir_name and restore right after?
> For RPC he would need to provide fd and how is he going to do that, when
> on restore we will check if he can't write there?

We open a 0700 directory with O_PATH (can we?) and ask service
to write there.

>>
>> The second is more sophisticated -- when restoring a process whose
>> suid differs from the uid one, we should check that a) the executable
>> and libraries we link to it belong to this suid and b) the code that
>> would get executed sits in one of these files. But this way looks
>> quite nasty.
> Sophisticated it is.
> 
> Btw, images are owned by root when created. If permissions are right like
> 640 or so, to not let non-root user to mess with them, then we could
> protect from image manipulation just by checking permissions on restore.
> 
> So, maybe check on restore that images owner == root and perms == 640?

This makes sense to me.

> And then add check from kill_ok_by_cred?

This would be excessive, I think. If images came from root and
nobody except him can write them, then they can be of any contents.

Thanks,
Pavel