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

[Pavel Emelyanov]

On 06/17/2014 06:18 PM, Ruslan Kuprieiev wrote:
> On 17.06.2014 15:08, Pavel Emelyanov wrote:
>> On 06/17/2014 11:23 AM, Ruslan Kuprieiev wrote:
>>> 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
>> CRIU doesn't chown imgs_dir.
>>
>>> to root and perms 640, user won't be able to access log.
>> Then user can specify workdir where logs will be put.
>>
>>> 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?
>> In the end, it's up to root. If root gives to users permissions to
>> put image files somewhere, it should care about their removal.
>>
>> By the way, what if criu just creates files with rw-r--r-- permissions
>> owned by root. Regular user will be able to read them, but will have
>> no rights to modify. From _such_ files criu can restore anything.
> 
> So, just to be sure that I get it right. Here is the plan:
> -- make criu create images with rw-r--r-- perms owned by root
> -- on restore, make sure that images are rw-r--r-- and owned by root, and if
>      smth wrong, don't allow non-root to restore

No, don't allow restoration of "extended prio" like changed xids and
non zero caps.

> -- use that check from kill_ok_by_creds to determine whether or not 
> non-root user
>      is allowed to dump/restore

No, regular user cannot change to another user or raise its caps by
changing images. This has nothing to do with kill_ok_by_creds.

Thanks,
Pavel