[clamav-users] ClamAV 0.103.0 release candidate - systemd service start fails

[Arjen de Korte]

Citeren Michael Orlitzky via clamav-users <clamav-users at lists.clamav.net>:

> On 2020-08-21 08:11, Arjen de Korte via clamav-users wrote:
>> Citeren Michael Orlitzky via clamav-users <clamav-users at lists.clamav.net>:
>>
>>> On 2020-08-21 04:45, Arjen de Korte via clamav-users wrote:
>>>>
>>>> It is not clear to me what problem this patch intends to solve (for a
>>>> systemd service it is absolute not required from a security point of
>>>> view). The PIDFile should be writable by vscan user only anyway.
>>>>
>>>
>>> With a Type=forking service, systemd will send SIGTERM to the contents
>>> of the PID file as root.
>>
>> Not unconditionally. See the following from 'man 5 systemd.service':
>>
>>     "The PID file does not need to be owned by a privileged user, but if it
>>      is owned by an unprivileged user additional safety restrictions are
>>      enforced: the file may not be a symlink to a file owned by a different
>>      user (neither directly nor indirectly), and the PID file must refer to
>>      a process already belonging to the service."
>>
>
> That's good to hear (and is news to me), and maybe they've found another
> way to prevent this vulnerability in systemd.
>
>
>>> If the "vscan" user can put whatever he wants
>>> in the PID file, then he can kill root processes.
>>
>> See above: you're trying to fix a problem that doesn't exist.
>
> However, systemd isn't the only service manager, and the problem still
> exists in all of the other ones. Systemd is able to avail itself of
> platform-specific features in brand-new Linux kernels. SysV init,
> OpenRC, and others must stick to real or de-facto standard tools, and
> there is no standard way to implement what systemd says they've done.

That may be, but now you have replaced it with two processes that run  
in the foreground, one of them as unpriviledged user and one as root  
(probably to delete the PIDFile upon exit). I don't consider this  
progress.

>>> Are you using the upstream systemd service?
>>
>> No, we're using "Type=forking" since the clamd.service can take
>> several minutes to start and we don't want to start services that
>> depend on it before it actually finished starting up. Creating the
>> socket beforehand is not a solution, as clamd won't start serving any
>> requests until it has actually finished starting up.
>
> That's fine, now you just need to synchronize the PIDFile and PidFile
> entries in your systemd service and clamd.conf, respectively.

No, as stated before, systemd doesn't need the PIDFile at all. It  
keeps track of the processes it started without the help of a PIDFile.  
It *can* use a PIDFile if you provide it with one and the only thing  
it will do with that is to remove that file if the service doesn't do  
it itself upon exit. Nothing more, it is not used for process control.  
There is absolutely no need for a PIDFile in the clamd.service, even  
with Type=forking.

> I suggest
> /run/clamd.pid, or any other location that is writable only by root.

It's fine where it is now. See above: systemd doesn't care.

>>> It defaults to Type=simple, and runs clamd in the foreground.
>>
>> See above. Actually, with this patch clamd wil always run in the
>> foreground, as daemonizing is now completely broken. Up to and
>> including 0.102.4, starting clamd on the commandline without any
>> further options would just start the daemon and return. Now, it never
>> returns.
> That's not true, your service file just needs to know where the PID file
> lives. It always did, but somehow it managed to not crash in the past.

No, it doesn't, see above. Even if I start clamd on the commandline  
(which should then daemonize, unless told by --foreground to stay in  
the foreground) without any of this. This is a regression and has  
nothing to do with systemd.

>>> In that case, your clamd daemon
>>> shouldn't be creating a PID file at all -- systemd should take care of
>>> it when it shoves the process into the background. PidFile should be
>>> left unset in clamd.conf.
>>
>> There is no PIDFile in the clamd.service file as systemd doesn't need
>> that here (even when running as Type=forking). The same goes for
>> freshclam.service. Systemd has other ways to keep track of which
>> processes it has started and will not use the PIDFile unless you tell
>> it to do so (with the above mentioned restrictions).
>
> Well empirically that's not true, because it isn't working. Add PIDFile
> entries to your service files when using Type=forking, and synchronize
> them with the PidFile lines in clamd.conf and freshclam.conf.

Makes no difference at all. Even without using systemd, clamd doesn't  
daemonize anymore, it will always run in the foreground.