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

[Pavel Emelyanov]

On 06/17/2014 10:41 AM, Ruslan Kuprieiev wrote:
> 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?

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

If _either_ current user is root _or_ images come from it in
"reliable" form, we restore anything. Otherwise, we check that
ids and caps are "safe".

Thanks,
Pavel