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

[Pavel Emelyanov]

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.

Thanks,
Pavel