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

[Ruslan Kuprieiev]

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;
}

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?

On 23.05.2014 12:33, Pavel Emelyanov wrote:
> On 05/19/2014 06:17 PM, Ruslan Kuprieiev wrote:
>> On 17.05.2014 12:00, Andrew Vagin wrote:
>>> On Fri, May 16, 2014 at 05:54:55PM +0300, Ruslan Kuprieiev wrote:
>>>> Currently there are typos in check_ids, so one can't pass this check,
>>>> unless (u/g)id == e(g/u)id == s(g/u)id == task_(g/u)id.
>> OMG, I mixed task and caller id's in this description!
> The checks you're fixing prevent from creating images with "bad"
> code and restore them into siud-ed process. What problem are we
> trying to resolve? If I get it right it is -- task executes a
> suid-ed binary belonging to some other user, then we checkpoint
> it, then try to restore and fail. Is that correct?
>
>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>> ---
>>>>    security.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/security.c b/security.c
>>>> index d4b4230..dc90208 100644
>>>> --- a/security.c
>>>> +++ b/security.c
>>>> @@ -28,7 +28,7 @@ static bool check_ids(unsigned int crid, unsigned int rid,
>>>> unsigned int eid, uns
>>>>    {
>>>>        if (crid == 0)
>>>>            return true;
>>>> -    if (crid == rid && crid == eid && crid == sid)
>>>> +    if (crid == rid || crid == eid || crid == sid)
>>> I have thought a bit more and now I am not sure about this. Could you explain why this is correct?
>> Hm... Yes, you are right. Well, crid == rid || crid == sid we get from the
>> statement, that we should be able to dump task, if we can kill it.
>> And crid == eid looks wrong.
>>
>>>>            return true;
>>>>
>>>>        pr_err("UID/GID mismatch %u != (%u,%u,%u)\n", crid, rid, eid, sid);
>>>> -- 
>>>> 1.8.1.2
>>>>
>>>> _______________________________________________
>>>> CRIU mailing list
>>>> CRIU at openvz.org
>>>> https://lists.openvz.org/mailman/listinfo/criu
>>
>