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

[Tsantilas Christos]

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?

>
> 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.

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?)


>
>
> 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)

>   We should be fixing that broken code. Either to not need the system
> suid privilege at all, or to call enter/leave_suid only around the
> sensitive operation - while also ensuring those suid calls will work at
> the point they are used.
>
> Amos