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

[Ruslan Kuprieiev]

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

>
>> Maybe we should add gete(u/g)id,gets(u/g)id and use check from
>> kill_ok_by_cred?
>> It looks easy to implement with cmdline usage of criu, but with service
>> we can't get all
>> id's(id, eid, sid) through the socket. But, maybe we could parse
>> /proc/pid/status to get all of them?
>> What do you all think about that?