[squid-dev] [PATCH] start workers as root

[Tsantilas Christos]

A patch which solves this problem applied to trunk as rev13984.
The patch is different that the patch I posted here.
It just adds  enter_suid() calls after the writePidFile and removePidFile()
inside the watch_child() function.

Regards,
   Christos

On 03/09/2015 11:09 AM, Tsantilas Christos wrote:
> On 03/08/2015 05:57 AM, Amos Jeffries wrote:
>> On 8/03/2015 6:34 a.m., Tsantilas Christos wrote:
>>> On 03/07/2015 07:18 AM, Amos Jeffries wrote:
>>>> On 7/03/2015 12:18 a.m., Tsantilas Christos wrote:
>>>>> SMP workers in trunk start without root privileges. This results in
>>>>> startup  failures when workers need to use a privileged port 
>>>>> (e.g., 443)
>>>>> or other  root-only features such as TPROXY.
>>>>>
>>>>> This bug added with my "Moved PID file management from Coordinator to
>>>>> Master" patch.
>>>>>
>>>>> The problem is inside watch_child function which called after a
>>>>> enter_suid() call, but the  writePidFile() call, inside the
>>>>> watch_child(), will leave suid mode before exit.
>>>>>
>>>>> This patch removes the enter_suid/leave_suid cals from the 
>>>>> writePidFile
>>>>>    and make the caller responsible for setting the root privileges if
>>>>> required.
>>>>
>>>> I think this is wrong approach.
>>>>
>>>> Firstly, what are processes without SUID ability doing writing to 
>>>> secure
>>>> system files?
>>>
>>> What do you mean here?
>>
>> After your patch that makes the master file the one writing the PID file
>> what are the workers (non-master) doing writing to it?
>
> The workers does not write the PID file.
> A single-process squid  (eg when started with the "-N" parameter) 
> still need to write PID file.
>
>
>>
>>>
>>>>
>>>> Secondly, I thought the entire point of the earlier patch was to make
>>>> the *MASTER* process was the one writing the PID file. Not
>>>> low-privileged workers.
>>>
>>> Yes the master process writing the pid file, not the workers.
>>>
>>>>
>>>> Thirdly, the enter/leave_suid calls mean "dangerous security stuff 
>>>> about
>>>> to happen" and should only be called if absolutely necessary, AND only
>>>> around the (block of) system calls which require them.
>>>
>>> I agree.
>>> However the watch_child which is implements the master process, is
>>> designed to run in suid mode. This was not changed by the PID patch.
>>> Just due to a bug added with the PID patch this function leaves the 
>>> suid
>>> mode.
>>
>>
>> I think I understand you there. You mean this process:
>>
>>   enter_suid()
>>   ...
>>   watch_child()
>>    ...
>>    writePidFile() {
>>     enter_squid() // no-op
>>     ...
>>     leave_suid()
>>   }
>>   ... something that needs suid. oops.
> Yep.
>
>>
>>
>>>
>>> Maybe we want to fix master process to not run in suid mode, but I 
>>> believe:
>>>    - This is not a scope for this patch
>>>    - The master process does not do a lot of thinks. Probably we do not
>>> need to make it run with low privileges. Moreover we may have problems
>>> with the kids. (Is it possible for kids to run with different
>>> cache_effective_user parameter?)
>>>
>>
>> Yes, more than possible - probable that somebody will or is already
>> doing so. Just the other day I saw someone running workers with
>> different PID files.
>> Thats the kind of thing that happens with these directives available in
>> via squid.conf per-worker.
>>
>>
>>>>
>>>> Your description sounds like some part of the code in worker scope is
>>>> using enter_suid doing a lot of Squid stuff - plus incidentally some
>>>> root system stuff, then leave_suid. That is broken code. None of the
>>>> general "Squid stuff" are security sentitive system calls needing root
>>>> privileges.
>>>
>>> No, please forget the workers. This patch does not change anything in a
>>> worker.
>>> Please take a look into the writePidFile function, and into watch_child
>>> function (which implements the master process)
>>>
>>
>> The place your patch is changing are all in the workers code.
>>
>> Instead of moving the enter_suid() outside of writePidFile() in several
>> places the master code should call enter only to preserve the part of
>> code *it* needs suid. eg.
>>
>>   watch_child()
>>     ...
>>     writePidFile()
>>     enter_suid(); // writePidFile() uses leave_suid()
>>     ...
>>
>> Much simpler patch, and fewer places overall using enter/leave_suid.
>
>
> Well, still a writePidFile call in the future may cause similar bug.
> The need of calling an "enter_suid()" after writePidFile,  is not 
> something a developer expects.
>
> However I have no problem for this patch too, it is OK for me.
> If you prefer it I will apply this patch instead.
>
>>
>> Amos
>>
>> _______________________________________________
>> squid-dev mailing list
>> squid-dev at lists.squid-cache.org
>> http://lists.squid-cache.org/listinfo/squid-dev
>>
>