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

[Pavel Emelyanov]

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.

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