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

[Ruslan Kuprieiev]

On 17.06.2014 09:23, Pavel Emelyanov wrote:
> 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.

Oh, cool! But it may break backward compatibility, right?
How about stick to method below?

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

We can use this excessive method only if user != root.
Smth like if (getuid() != 0) {full check like in kill_ok_by_cred}.
Just to not slow down criu by get*id() calls and parsing proc, unless 
user is non-root, when, I think, it worth it.

> Thanks,
> Pavel