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

[Ruslan Kuprieiev]

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?

>
> 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?
And then add check from kill_ok_by_cred?

> Thanks,
> Pavel