[CRIU] [PATCH] security: set suid flag on crtools and check real uid on dump/restore

[Ruslan Kuprieiev]

On 02.10.2013 16:03, Pavel Emelyanov wrote:
> On 10/02/2013 06:31 PM, Ruslan Kuprieiev wrote:
>> On 02.10.2013 18:27, Ruslan Kuprieiev wrote:
>>> On 02.10.2013 14:03, Pavel Emelyanov wrote:
>>>> On 10/02/2013 05:54 PM, Ruslan Kuprieiev wrote:
>>>>> On 02.10.2013 13:25, Pavel Emelyanov wrote:
>>>>>> On 10/02/2013 05:00 PM, Ruslan Kuprieiev wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Lets set suid flag on crtools, so non-root users could dump/restore
>>>>>>> their own tasks and start service for their own tasks. On start criu
>>>>>>> will get it's real uid and will allow user to dump/restore only tasks
>>>>>>> that he own.
>>>>>>>
>>>>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>>>>>
>>>>>> I don't quite understand the logic behind security_init() +
>>>>>> restrict_uid()
>>>>>> and the need in two uids stores in security.c
>>>>> I think we can extend security_init later with some extra features (but
>>>>> I don't know with which, though:)). Also checkpatch.pl was mad about
>>>>> initializing static variables with zeros:).
>>>>>
>>>>> And restrict_uid() will be used very often in cr
>>>>> I think we may need to remember real uid, so  if non-root will start
>>>>> service, he won't be able to change his "effective" uid and
>>>>> dump/restore
>>>>> tasks with other uids.
>>>> He will not be able to do it anyway. The restrict_uid() is not "let me
>>>> dump this uid", but "don't even try to dump anything but this".
>>>>
>>>>> It looks better to me, than resolving this
>>>>> situation in cr-service. Also i do think that ruid may be in handy
>>>>> later.
>>>>> Or just use getuid() every time, instead of declaring second uid?
>>>> I thought that we just restrict_uid(getuid()) on crtools start and in
>>>> service child req-setup and that's it.
>>> If non-root starts service, on crtools we call restrict_uid(getuid())
>>> and now he is allowed to dump only his own tasks. Than, he gets a
>>> request(through rpc) to dump task with other uid and we call
>>> restcrict_uid(uid). So we need to check getuid() at restrict_uid(), so
>>> root would be able to restrict to any uid, but non-root won't be able
>>> to do this. Something like this:
>>>
>>> void restrict_uid(uid)
>>> {
>>>      if (getuid() == 0)
>>>          dumper_uid = uid;
>>> }
>>>
>>>
>> beter like this:
>>
>> void restrict_uid(uid)
>> {
>>       if (getuid() == 0 || getuid() == uid)
>>           dumper_uid = uid;
>> }
>>
>> so we will be able to use it in crtools in the very beginning.
>> .
>>
>
> If the service is started from UserA and gets request from UserB setting the
> restrict_uid() for UserB, then it will not be able to dump UserB anyway, there's
> no need in artificial checks.
Oh, we do already have if(dumper_uid == 0) return 0; in may_dump_uid() :)
Sorry, I'm so stupid:)
Thanks.

> Service started from UserA only makes sense if this user wants to dump his
> tasks. Nothing more.