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

[Ruslan Kuprieiev]

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
-- use that check from kill_ok_by_creds to determine whether or not 
non-root user
     is allowed to dump/restore

Right?

> Thanks,
> Pavel