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

[Ruslan Kuprieiev]

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

I don't know yet=).

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

But what if user didn't specify logfile out of imgs_dir? If criu chown 
whole imgs_dir
to root and perms 640, user won't be able to access log.
Maybe we should check if owner == root and perms == 640 for every single
image file, not whole imgs_dir?

Btw, if perms == 640, how user should delete imgs if he don't need them?

>
> Thanks,
> Pavel