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

[Amos Jeffries]

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?

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


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

Amos