Bug#889144: stricter PIDfile handling breaks several daemons

[Simon Kelley]

On 04/02/18 20:26, Sven Hartge wrote:

> Does dnsmasq need a PIDfile when running under systemd? Can't it just
> not double fork, stay in the foreground using a Type=simple systemd unit?
> 
> That way the whole problem could be avoided all together.
> 

Sending signals to the dnsmasq process cause it to take various actions,
so it's likely that there are scripts out there that do the equivalent of

kill - `cat /run/dnsmasq/dnsmasq.pid`

The double-fork code in dnsmasq is also quite careful about error
handling; for instance passing an error code back through a pipe from
the forked process to the original process, so that any failure gets
reflected in the return code when the original process exits. That's
relevant to Michael's point, I think.

Cheers,

Simon.

Bug#889144: stricter PIDfile handling breaks several daemons

[Michael Biebl]

Am 04.02.2018 um 21:26 schrieb Sven Hartge:
> On Sun, 4 Feb 2018 15:41:37 + Simon Kelley 
> wrote:
> 
>> With my dnsmasq maintainer hat on, the current arrangement looks like this.
>>
>> 1) /run/dnsmasq is a directory owned by dnsmasq:nogroup
>> 2) /run/dnsmasq/dnsmasq.pid gets written by dnsmasq before it drops
>> root, so is root:root
>> 3) The reason /run/dnsmasq is owned by dnsmasq is so that dnsmasq can
>> unlink the pidfile at shutdown, after it has dropped root and is running
>> as 'dnsmasq'
> 
> Does dnsmasq need a PIDfile when running under systemd? Can't it just
> not double fork, stay in the foreground using a Type=simple systemd unit?
> 
> That way the whole problem could be avoided all together.

If other services depend on dnsmasq, please keep
https://www.lucas-nussbaum.net/blog/?p=877 in mind




-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Bug#889144: stricter PIDfile handling breaks several daemons

[Sven Hartge]

On Sun, 4 Feb 2018 15:41:37 + Simon Kelley 
wrote:

> With my dnsmasq maintainer hat on, the current arrangement looks like this.
> 
> 1) /run/dnsmasq is a directory owned by dnsmasq:nogroup
> 2) /run/dnsmasq/dnsmasq.pid gets written by dnsmasq before it drops
> root, so is root:root
> 3) The reason /run/dnsmasq is owned by dnsmasq is so that dnsmasq can
> unlink the pidfile at shutdown, after it has dropped root and is running
> as 'dnsmasq'

Does dnsmasq need a PIDfile when running under systemd? Can't it just
not double fork, stay in the foreground using a Type=simple systemd unit?

That way the whole problem could be avoided all together.

Grüße,
Sven.



signature.asc
Description: OpenPGP digital signature

Bug#889144: stricter PIDfile handling breaks several daemons

[Sven Hartge]

On 04.02.2018 17:25, Michael Biebl wrote:
> Am 03.02.2018 um 14:35 schrieb Sven Hartge:
>> Um 14:00 Uhr am 03.02.18 schrieb Michael Biebl:

>>> The alternative afaics would be, that the daemon writes the pid file as
>>> munin:munin then (or ulog:ulog for the above case).
>>
>> No, this would open a potential DoS vector.
>>
>> Image an attacker gaining access to the munin user. He would then be able
>> to write any PID to the PIDfile and the init system would kill the other
>> process when the munin-node service is stopped/restarted.
>>
> 
> I don't think this applies to systemd though. If the process id listed
> in the pid file is not found in the service cgroup, systemd should not
> kill the process listed in the pid file. I suspect that MainPID will not
> be properly set and systemd will complain about it.

But it applies to SysV-Init. If the init-script does not use
start-stop-daemon correctly to check if the PID in the PIDfile belongs
to the executable to be killed or if the init-script uses some other
method of killing the daemon, it might easily kill a different program.

I know, this is not systemds concern whether other init implementations
behave correctly, but if you change the behaviour of a program because
of a behaviour change in systemd and then break other init systems or
increase the insecurity when used with other init systems because of
this, it will fall back negatively on systemd.

Grüße,
Sven.



signature.asc
Description: OpenPGP digital signature

Bug#889144: stricter PIDfile handling breaks several daemons

[Michael Biebl]

Am 03.02.2018 um 14:35 schrieb Sven Hartge:
> Um 14:00 Uhr am 03.02.18 schrieb Michael Biebl:

>> The alternative afaics would be, that the daemon writes the pid file as
>> munin:munin then (or ulog:ulog for the above case).
> 
> No, this would open a potential DoS vector.
> 
> Image an attacker gaining access to the munin user. He would then be able
> to write any PID to the PIDfile and the init system would kill the other
> process when the munin-node service is stopped/restarted.
> 

I don't think this applies to systemd though. If the process id listed
in the pid file is not found in the service cgroup, systemd should not
kill the process listed in the pid file. I suspect that MainPID will not
be properly set and systemd will complain about it.

Michael


-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Bug#889144: stricter PIDfile handling breaks several daemons

[Simon Kelley]

With my dnsmasq maintainer hat on, the current arrangement looks like this.

1) /run/dnsmasq is a directory owned by dnsmasq:nogroup
2) /run/dnsmasq/dnsmasq.pid gets written by dnsmasq before it drops
root, so is root:root
3) The reason /run/dnsmasq is owned by dnsmasq is so that dnsmasq can
unlink the pidfile at shutdown, after it has dropped root and is running
as 'dnsmasq'


There's a potential security hole here, since an attacker who can become
user dnsmasq, can create a symlink at /run/dnsmasq/dnsmasq.pid to
anywhere, and have the target of the symlink overwritten (as root) at
startup. The dnsmasq PID-file creation code detects and blocks this
case: see src/dnsmasq.c around line 507.

I think that this can be fixed in dnsmasq by chown()ing the pid file to
the same user dnsmasq is about to drop privs too, but I'm not sure is
that's enough to keep the new systemd checks happy.

Cheers,

Simon.

Bug#889144: stricter PIDfile handling breaks several daemons

[Michael Biebl]

Control: forwarded -1 https://github.com/systemd/systemd/issues/8085

Hi Sven,

I filed an upstream issue at
https://github.com/systemd/systemd/issues/8085 trying to summarize what
the issue is afaiu from reading this and related bug reports.

If you have further feedback or corrections, please directly follow up
on the upstream bug report.

Sorry for the inconvenience. Unfortunately a git revert is non-trivial,
so this will probably need input from upstream.

Regards,
Michael
-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Processed: Re: Bug#889144: stricter PIDfile handling breaks several daemons

[Debian Bug Tracking System]

Processing control commands:

> forwarded -1 https://github.com/systemd/systemd/issues/8085
Bug #889144 [systemd] stricter PIDfile handling breaks several daemons
Set Bug forwarded-to-address to 
'https://github.com/systemd/systemd/issues/8085'.

-- 
889144: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=889144
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems

Bug#889144: stricter PIDfile handling breaks several daemons

[Sven Hartge]

Um 14:00 Uhr am 03.02.18 schrieb Michael Biebl:
> Am 03.02.2018 um 13:27 schrieb Sven Hartge:
>> Um 03:02 Uhr am 03.02.18 schrieb Michael Biebl:

>>> Does munin-node have the same mismatch?
 
>> It has:

>> But, as you can see, the directory is also used by the munin-updater
>> which is run as user "munin" so you can't make the directory owned by
>> root.
 
> The alternative afaics would be, that the daemon writes the pid file as
> munin:munin then (or ulog:ulog for the above case).

No, this would open a potential DoS vector.

Image an attacker gaining access to the munin user. He would then be able
to write any PID to the PIDfile and the init system would kill the other
process when the munin-node service is stopped/restarted.

See https://security-tracker.debian.org/tracker/CVE-2017-14610 for
example.

Acknowleged, this is a a bit unlikely issue, but I don't think it would be
wise to propagate it further.

Also looking at the bigger picture here: This change in systemd, while
with security in mind, changes the way PIDfiles have been handled for the
last ... forever.

Debian packages one might be able to fix but what about all the other 3rd
party packages and locally written code out there? It is susceptible to
break upon upgrade to Buster and who will be blamed (again)? systemd of
course, "breaking all the things again".

Grüße,
Sven.

Bug#889144: stricter PIDfile handling breaks several daemons

[Michael Biebl]

Am 03.02.2018 um 13:27 schrieb Sven Hartge:
> Um 03:02 Uhr am 03.02.18 schrieb Michael Biebl:
> 
>> Am 02.02.2018 um 20:07 schrieb Sven Hartge:
> 
>>> ulogd2 drops its priviliges on its own. It needs to start as root to
>>> connect to the netlink sockets.
>  
>> So, ulogd2 creates a directory /run/ulog which is owned by ulog:ulog but
>> then creates the pid file /run/ulog/ulog.pid owned by root:root.
> 
> Yes.
> 
>> I assume if you overwrite /usr/lib/tmpfiles.d/ulogd2.conf by creating a
>> /etc/tmpfiles.d/ulogd2.conf containing
>>
>> d /run/ulog 0755 root root - -
>>
>> ulogd2 will start properly.
> 
> It does. But there must be a reason for the directory to be owned by
> ulog:ulog, no? 

No idea. This is something the ulogd maintainer would have to answer.

What consequences does it have changing it? It may work for
> my simple setup but then break again for other people.
> 
>> I assume, ulogd2 should either ensure the pidfile is owned ulog:ulog or
>> change the run directory to match the permissions of the pid file.
>>
>> Does munin-node have the same mismatch?
> 
> It has:
> 

Ok, thanks for checking.

> But, as you can see, the directory is also used by the munin-updater
> which is run as user "munin" so you can't make the directory owned by
> root.

The alternative afaics would be, that the daemon writes the pid file as
munin:munin then (or ulog:ulog for the above case).

-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Bug#889144: stricter PIDfile handling breaks several daemons

[Sven Hartge]

Um 03:02 Uhr am 03.02.18 schrieb Michael Biebl:

> Am 02.02.2018 um 20:07 schrieb Sven Hartge:

>> ulogd2 drops its priviliges on its own. It needs to start as root to
>> connect to the netlink sockets.
 
> So, ulogd2 creates a directory /run/ulog which is owned by ulog:ulog but
> then creates the pid file /run/ulog/ulog.pid owned by root:root.

Yes.

> I assume if you overwrite /usr/lib/tmpfiles.d/ulogd2.conf by creating a
> /etc/tmpfiles.d/ulogd2.conf containing
> 
> d /run/ulog 0755 root root - -
> 
> ulogd2 will start properly.

It does. But there must be a reason for the directory to be owned by
ulog:ulog, no? What consequences does it have changing it? It may work for
my simple setup but then break again for other people.

> I assume, ulogd2 should either ensure the pidfile is owned ulog:ulog or
> change the run directory to match the permissions of the pid file.
> 
> Does munin-node have the same mismatch?

It has:

,
| ds9:/run/munin# ls -al
| total 8
| drwxr-xr-x  2 munin root80 Feb  3 13:15 .
| drwxr-xr-x 55 root  root  1880 Feb  3 02:57 ..
| -rw-r--r--  1 munin munin7 Feb  3 13:15 
munin-feds.ath.cx-skuld.feds.ath.cx.lock
| -rw-r--r--  1 munin munin7 Feb  3 13:15 
munin-svenhartge.de-www.svenhartge.de.lock
`

But, as you can see, the directory is also used by the munin-updater
which is run as user "munin" so you can't make the directory owned by
root.

S°

Bug#889144: stricter PIDfile handling breaks several daemons

[Michael Biebl]

Am 02.02.2018 um 20:07 schrieb Sven Hartge:

> ulogd2 drops its priviliges on its own. It needs to start as root to
> connect to the netlink sockets.

So, ulogd2 creates a directory /run/ulog which is owned by ulog:ulog but
then creates the pid file /run/ulog/ulog.pid owned by root:root.

I assume if you overwrite /usr/lib/tmpfiles.d/ulogd2.conf by creating a
/etc/tmpfiles.d/ulogd2.conf containing

d /run/ulog 0755 root root - -

ulogd2 will start properly

I assume, ulogd2 should either ensure the pidfile is owned ulog:ulog or
change the run directory to match the permissions of the pid file.

Does munin-node have the same mismatch?
-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?



signature.asc
Description: OpenPGP digital signature

Processed: Re: Bug#889144: stricter PIDfile handling breaks several daemons

[Debian Bug Tracking System]

Processing control commands:

> severity -1 serious
Bug #889144 [systemd] stricter PIDfile handling breaks several daemons
Severity set to 'serious' from 'important'

-- 
889144: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=889144
Debian Bug Tracking System
Contact ow...@bugs.debian.org with problems