Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: stricter checks on the PID read from PIDFile= or sd_notify()'s MAINPID= #6632

Closed
shibumi opened this issue Aug 17, 2017 · 66 comments
Closed

Comments

@shibumi
Copy link
Contributor

@shibumi shibumi commented Aug 17, 2017

Submission type

[x] Bug report

systemd version the issue has been seen with

systemd 229
+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ -LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD -IDN

Used distribution

Ubuntu 16.04

Report

Hello,
Due to the last CVE for nagios-nrpe-server[1], I wanted to make sure that my systems with systemd are safe. I thought that this CVE[1] would only apply to old init systems that rely strongly on PIDfiles. So I've started my Ubuntu 16.04 Container with systemd 229 and verified that CVE[1] with systemd. And indeed it seem to stop the systemd service:

Aug 17 14:27:35 motoko systemd[1]: Stopping LSB: Start/Stop the Nagios remote plugin execution daemon...
Aug 17 14:27:35 motoko nagios-nrpe-server[4969]:  * Stopping nagios-nrpe nagios-nrpe
Aug 17 14:27:35 motoko systemd[1]: Received SIGTERM from PID 4972 (start-stop-daem).
Aug 17 14:27:35 motoko systemd[1]: Reexecuting.
Aug 17 14:27:35 motoko systemd[1]: systemd 229 running in system mode. (+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUT
LS +ACL +XZ -LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD -IDN)
Aug 17 14:27:35 motoko systemd[1]: Detected virtualization systemd-nspawn.
Aug 17 14:27:35 motoko systemd[1]: Detected architecture x86-64.

We had a discussion about this in the #systemd-Channel in Freenode. So I've tried that in my Arch Linux Container with systemd-234 as well and I didn't had this in my Logs.

but one of the guys in the channel said that this still exists due to not existing checks for sd_notify("MAINPID=...) and he has accomplished something like this:

    CGroup: /system.slice/test.service            
            └─367351 python /home/grawity/test.py 
            ‣ 367337 /usr/bin/htop                

So I guess that this is still a thing and should be fixed.

[1] http://www.openwall.com/lists/oss-security/2017/08/16/7

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Aug 30, 2017

Well, what precisely would you expect us to check there? Checking the user ID of the new process isn't really doable, as we permit daemons to change uids freely. Checking the cgroup doesn't work either, as we support damons whose main process lives outside of the original cgroup they created (that's the default in gettys after all). Note that sd_notify() is restricted already via NotifyAccess=, and it defaults to not available, unless explicitly enabled (either via NotifyAccess= set to something else than "no", or via Type=notify), hence this is already well protected I figure.

That said, I am open to more strict checks here, but I am not sure what they would look like, do you have any suggestions? @grawity maybe you?

@poettering poettering changed the title systemd doesn't verify PIDs in PIDFiles RFE: stricter checks on the PID read from PIDFile= or sd_notify()'s MAINPID= Aug 30, 2017
@grawity

This comment has been minimized.

Copy link
Contributor

@grawity grawity commented Aug 31, 2017

I'm not sure really. But, how many daemons live outside the original cgroup and try to change their mainpid? (As far as I can see, agetty definitely always lives in …/system-getty.slice/getty@.service – only its child /sbin/login process is moved out, and that happens via privileged PAM, not via sd_notify().)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Aug 31, 2017

wel, it's not just about how many there are, it's more about "do we think that daemon should be able to do that even if they currently don't do that..."

What about this: we change the logic so that one of the following two rules must hold:

a) the sender UID of MAINPID= matches the UID of the PID specified therein
b) the sender UID is either root or whatever is configured in User=

I think with that we should cover most cases safely.

Does that make sense?

@shibumi

This comment has been minimized.

Copy link
Contributor Author

@shibumi shibumi commented Sep 7, 2017

@poettering that sounds much better as the status quo! 👍

@dhke

This comment has been minimized.

Copy link

@dhke dhke commented Nov 6, 2017

I'm not sure the scope of this issue is already sufficiently covered, here, since the suggested fixed only cover MAINPID as obtained via sd_notify().

Given that e.g. ExecStop= by default runs with the privileges of the unit, I would have expected systemd to perform the kill (e.g. for KillMode=mixed) with the privileges of the unit's user, e.g. do something like what would have previously been

MAINPID=`head -1 -- "$PIDFILE"`
su -u "$USER" -c "/usr/sbin/kill -- '$MAINPID'"

or even have functionality similar to killproc, which has additional validation with regard to the path of the killed process. I would at least have assumed that it only resorts to using elevated privileges when the low privilege kill does not result in the expected outcome.

The current implementation rather seems to be the equivalent of

kill "`head -1 $PIDFILE`"

with the only validation of $PIDFILE's contents being that it contains a valid integer.

As already noted, this allows a non-privileged unit to potentially trick the system into killing an arbitrary process by writing a PID to a specific file owned by the unit's user. This may even happen by accident, if the PID file write from the daemon is not atomic and only a partial PID was written (that's probably purely a theoretical problem).

While, obviously, this requires root privileges to hand the privilege to the unit and thus doesn't seem to be a strong attack vector, it was still sufficient to lull me into a false sense of security. I fully expected the kill to happen with user privileges, given that I seemed to have an option to turn that security off (namely PermissionsStartOnly=).

Also, having less secure defaults with the rationale that systemd also caters to daemons where $MAINPID leaves the unit's cgroup seems to be at odds with the seemingly ubiquitous notion that the service cgroup isolates the processes of a service.

poettering added a commit to poettering/systemd that referenced this issue Jan 5, 2018
…sages

Let's be more restrictive when validating PID files and MAINPID=
messages: don't accept PIDs that make no sense, and if the configuration
source is not trusted, don't accept out-of-cgroup PIDs. A configuratin
source is considered trusted when the PID file is owned by root, or the
message was received from root.

This should lock things down a bit, in case service authors write out
PID files from unprivileged code or use NotifyAccess=all with
unprivileged code. Note that doing so was always problematic, just now
it's a bit less problematic.

When we open the PID file we'll now use the CHASE_SAFE chase_symlinks()
logic, to ensure that we won't follow an unpriviled-owned symlink to a
privileged-owned file thinking this was a valid privileged PID file,
even though it really isn't.

Fixes: systemd#6632
poettering added a commit to poettering/systemd that referenced this issue Jan 5, 2018
…sages

Let's be more restrictive when validating PID files and MAINPID=
messages: don't accept PIDs that make no sense, and if the configuration
source is not trusted, don't accept out-of-cgroup PIDs. A configuratin
source is considered trusted when the PID file is owned by root, or the
message was received from root.

This should lock things down a bit, in case service authors write out
PID files from unprivileged code or use NotifyAccess=all with
unprivileged code. Note that doing so was always problematic, just now
it's a bit less problematic.

When we open the PID file we'll now use the CHASE_SAFE chase_symlinks()
logic, to ensure that we won't follow an unpriviled-owned symlink to a
privileged-owned file thinking this was a valid privileged PID file,
even though it really isn't.

Fixes: systemd#6632
@yuwata yuwata added the has-pr label Jan 5, 2018
poettering added a commit to poettering/systemd that referenced this issue Jan 8, 2018
…sages

Let's be more restrictive when validating PID files and MAINPID=
messages: don't accept PIDs that make no sense, and if the configuration
source is not trusted, don't accept out-of-cgroup PIDs. A configuratin
source is considered trusted when the PID file is owned by root, or the
message was received from root.

This should lock things down a bit, in case service authors write out
PID files from unprivileged code or use NotifyAccess=all with
unprivileged code. Note that doing so was always problematic, just now
it's a bit less problematic.

When we open the PID file we'll now use the CHASE_SAFE chase_symlinks()
logic, to ensure that we won't follow an unpriviled-owned symlink to a
privileged-owned file thinking this was a valid privileged PID file,
even though it really isn't.

Fixes: systemd#6632
poettering added a commit to poettering/systemd that referenced this issue Jan 11, 2018
…sages

Let's be more restrictive when validating PID files and MAINPID=
messages: don't accept PIDs that make no sense, and if the configuration
source is not trusted, don't accept out-of-cgroup PIDs. A configuratin
source is considered trusted when the PID file is owned by root, or the
message was received from root.

This should lock things down a bit, in case service authors write out
PID files from unprivileged code or use NotifyAccess=all with
unprivileged code. Note that doing so was always problematic, just now
it's a bit less problematic.

When we open the PID file we'll now use the CHASE_SAFE chase_symlinks()
logic, to ensure that we won't follow an unpriviled-owned symlink to a
privileged-owned file thinking this was a valid privileged PID file,
even though it really isn't.

Fixes: systemd#6632
@thoger

This comment has been minimized.

Copy link

@thoger thoger commented Nov 20, 2018

Any thoughts on @dhke's question regarding why not send signal with dropped privileges instead of root when service is configured to run as non-root User/Group?

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Nov 20, 2018

Any thoughts on @dhke's question regarding why not send signal with dropped privileges instead of root when service is configured to run as non-root User/Group?

Well, changing creds in PID 1 is quite problematic, we'd need to fork off a stub which does setresuid(), does the kill(), then exits, and the parent must wait for it. Which is possible, but not easy to get right, since you need to be reasonably protected from the user doing stuff with your process after you dropped privs and stuff... you also need to protect yourself from RLIMIT_NPROC games and such. I mean, this is all doable, but one needs to be very careful with what you do.

In general though, I am not sure this is the right approach in the first place. there's suid binaries and stuff, clients can change identity as much as they want, and in general they have every right to, and we need to handle that correctly even if the user the service uses is not declared in the unit file... Hence, we need a code path that does not rely on changing identities to some known target unit, and thus it's a question why bother at all with that...

(And finally, I think it's already kinda nice that if a service uses sigwaitinfo() that the creds of the sender are actually the creds of PID 1 and not of some stub that isn't around anymore)

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 10, 2018

@poettering @shibumi Are you ok with me requesting a CVE for this issue? I think it's a local Denial of Service and it would help us, and others, keep track of the problem.

@keszybz

This comment has been minimized.

Copy link
Member

@keszybz keszybz commented Dec 10, 2018

The original issue was fixed in systemd 237, two releases ago. I don't think it makes sense to open a CVE for it now.

why not send signal with dropped privileges instead of root when service is configured to run as non-root User/Group?

This would create an easy DOS, where any unit which forks any setuid binaries becomes unkillable. Doesn't seem to be a good idea at all.

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 10, 2018

@poettering @shibumi Are you ok with me requesting a CVE for this issue? I think it's a local Denial of Service and it would help us, and others, keep track of the problem.

It's not really a vulnerability. the newer checks are tighter than the old ones, but the NotifyAccess= logic was already an effective access check in this regard. We didn't allow arbitrary service processes to do whatever they want with the main PID, you had to be privileged already to install the service file, and you'd then also have to open up access to the sd_notify() logic as a second step to do anything with this.

So, this is not really vulnerability in the first place I'd say, and has been made stricter a long time ago. As long as CVEs are however what they are (i.e. considered a form of currency that security people deal in?), and as long as every minor CVE in systemd is something big tech news sites think it's worth doing long articles about I'd prefer not having CVEs assigned to stuff that isn't really a vulnerability and has been made stricter long time ago. I hope this makes sense?

@dhke

This comment has been minimized.

Copy link

@dhke dhke commented Dec 10, 2018

For what it's worth, please note my original post:

I would at least have assumed that it only resorts to using elevated privileges when the low privilege kill does not result in the expected outcome.

The rationale behind this being, that one should always try flipping the switch before using a hammer to shut down things for good. One might disagree with that, but this discussion has by now significantly digressed from my initial comment.

Well, changing creds in PID 1 is quite problematic, we'd need to fork off a stub which does setresuid(), does the kill(), then exits, and the parent must wait for it.

Isn't that what it needs to happen, if

ExecStop=/usr/sbin/kill -TERM $MAINPID

is specified and don't the mentioned issues apply here, too?

@shibumi

This comment has been minimized.

Copy link
Contributor Author

@shibumi shibumi commented Dec 10, 2018

I agree with @poettering. Scheduling a CVE for this is a little bit late. The issue has been fixed since weeks or maybe even months. Moreover killing the systemd main pid process would just result into a restart of pid 1 and please don't forget that you need enough permissions to place the malicious pid file and you have to wait for the root user to send the kill signal to the maliciously placed pid.

@bl33pbl0p

This comment has been minimized.

Copy link
Contributor

@bl33pbl0p bl33pbl0p commented Dec 10, 2018

Moreover killing the systemd main pid process would just result into a restart of pid 1

That seems incorrect, usually if an assertion is hit, PID1 freezes itself and (since recently) continues reaping orphans. This means you cannot talk to systemd anymore, control none of the services, lose all of its functionality (watchdog, sockets, timers, path events, etc), but still continue to use the system. OTOH, the kernel reboots the system anyway if PID1 goes down. You cannot "kill" PID1 on Linux unless it wants itself to be killed.

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 11, 2018

It's not really a vulnerability. the newer checks are tighter than the old ones, but the NotifyAccess= logic was already an effective access check in this regard. We didn't allow arbitrary service processes to do whatever they want with the main PID, you had to be privileged already to install the service file, and you'd then also have to open up access to the sd_notify() logic as a second step to do anything with this.

I was mainly referring to the content's replacement of the pid file of a non-root service. For that, I think there was no check in place. And that's not really something systemd can do anything about. You can't prevent admins from allowing a service to run as a different user (you do support that indeed), so the mainpid file could be writable by an un-privileged user (either because the unprivileged user has local access and can directly overwrite the mainpid file content or because, maybe, the service is vulnerable and allows somehow an attacker to replace the mainpid file's content).

That said, CVEs are just a way to give names to security related issues and they are not always a big deal (like in this case, for example), so not something to be aware of. The issue was fixed a couple of releases ago, but that would not block us from requesting a CVE. It would only help people who use systemd, and in particular those that still use an older version of systemd (e.g. RHEL, Debian, etc.), to keep track of things that may be security-relevant and choose for themselves what they want to do with them.

WRT having to be privileged already to install the service file, that's true but the flaw is not in installing the service file. That would just make the flaw harder to exploit and decrease even more its impact, because an attacker would have to find already some configurations/services in place. However, this is, I think, the only point that may prevent us from requesting a CVE and consider this just security hardening.

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 11, 2018

I was mainly referring to the content's replacement of the pid file of a non-root service. For that, I think there was no check in place. And that's not really something systemd can do anything about. You can't prevent admins from allowing a service to run as a different user (you do support that indeed), so the mainpid file could be writable by an un-privileged user (either because the unprivileged user has local access and can directly overwrite the mainpid file content or because, maybe, the service is vulnerable and allows somehow an attacker to replace the mainpid file's content).

Well, the daemon code is not entirely untrusted. It's chosen by the admin, and the PID file is carefully configured by the admin (at least that's the idea). I mean, it's good validating everything here, but it's not that anyone on sysv ever validated this stuff this thoroughly, and PID files are ultimately merely sysv compat. We are actively breaking compat here if you so will because we are more strictly validating PID files than any sysv script ever did...

Quite frankly, PID files shouldn't exist anymore. We support them for sysv compat, and they come with most of the pitfalls, security issues, lifecycle issues, PID race issues they always came with, and that's certainly not systemd's fault.

Or to say this differently: it's a bit like assigning a CVE to the fact that when systemd runs a SysV script it doesn't turn on PrivateTmp=. Because, yes, we don't do that, but SysV compat is sysv compat, and you should compare the security of real sysv implementations with our compat layer and you'll notice we are aleady tons more careful there already, and we can't do anything about the fact that sysv services worked the way sysv services worked.

That said, CVEs are just a way to give names to security related issues and they are not always a big deal (like in this case, for example), so not something to be aware of.

But that's really not how they are used or understood by the wider industry. They are used as a currency, they are counted and traded. And yes, systemd is under some extra scrutiny there, for some reason I don't fully understand. I am not sure we should needlessly play a part in that if there's no real reason to, because well, we are more strict now than the original concept of PID files ever was.

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 11, 2018

(What about this: please file a CVE against sysvinit/the sysv script concept/various sysv services and their PID file handling. Then we could claim that we fix all that with systemd, because that's what we do...)

@shibumi

This comment has been minimized.

Copy link
Contributor Author

@shibumi shibumi commented Dec 11, 2018

Moreover killing the systemd main pid process would just result into a restart of pid 1

That seems incorrect, usually if an assertion is hit, PID1 freezes itself and (since recently) continues reaping orphans. This means you cannot talk to systemd anymore, control none of the services, lose all of its functionality (watchdog, sockets, timers, path events, etc), but still continue to use the system. OTOH, the kernel reboots the system anyway if PID1 goes down. You cannot "kill" PID1 on Linux unless it wants itself to be killed.

Did you see my initial post? There I did exactly this. I placed the pid 1 in the pid file and systemd received a sigterm and reexecuted itself:

Aug 17 14:27:35 motoko systemd[1]: Stopping LSB: Start/Stop the Nagios remote plugin execution daemon...
Aug 17 14:27:35 motoko nagios-nrpe-server[4969]:  * Stopping nagios-nrpe nagios-nrpe
Aug 17 14:27:35 motoko systemd[1]: Received SIGTERM from PID 4972 (start-stop-daem).
Aug 17 14:27:35 motoko systemd[1]: Reexecuting.
Aug 17 14:27:35 motoko systemd[1]: systemd 229 running in system mode. (+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUT
LS +ACL +XZ -LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD -IDN)
Aug 17 14:27:35 motoko systemd[1]: Detected virtualization systemd-nspawn.
Aug 17 14:27:35 motoko systemd[1]: Detected architecture x86-64.
@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 18, 2018

@evverx We have a mail for security issues: security@archlinux.org + our dedicated flyspray bugtracker that allows us to close issues (but we will switch to another bugtracker in the future).

I'm sure you have a way to handle security issues. What I was trying to say is that it'd be great if you could update https://github.com/systemd/systemd/blob/master/docs/CONTRIBUTING.md#security-vulnerability-reports.

I see only three options here

I'm not sure why you excluded a private repository on GitHub :-) Keeping everything as is on GitHub and paying for a private repository is an option too. I kind of understand that it's expected that open source developers should suffer and that their time apparently costs nothing and can be wasted right and left, but, come on.

@keszybz

This comment has been minimized.

Copy link
Member

@keszybz keszybz commented Dec 18, 2018

if you could update https://github.com/systemd/systemd/blob/master/docs/CONTRIBUTING.md#security-vulnerability-reports.

No, actually please don't. The intent wasn't to provide for many or most distributions, but instead to have one or two ways to do it. Two and not just one mostly to provide redundancy. I think that current text with 3 way is too much already. My preference would be to cut it down to two, and put Fedora as the recommended one. The reason is that many core contributors are tied most closely to Fedora (@yuwata, @poettering, me, ...), so the response will be quicker and more direct this way. (Please don't see this as a way to diminish the role of Arch. I just don't think having multiple mechanism here is useful.)

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 18, 2018

No, actually please don't. The intent wasn't to provide for many or most distributions, but instead to have one or two ways to do it.

Which effectively means that Arch/NixOS/... users should spin up a distribution they never use to report a vulnerability. No wonder they are likely to go public.

@keszybz

This comment has been minimized.

Copy link
Member

@keszybz keszybz commented Dec 18, 2018

"Sign up" only means that they have to create an account. I have accounts of on Ubuntu, Debian, RHEL, OpenSuse, Arch, and probably some others, and it only means that I interacted and reported some bugs there in the past. I don't consider this too onerous.

Although, maybe we should consider creating a private google group for this. We have one for the code of conduct (completely dormant actually).

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 18, 2018

"Sign up" only means that they have to create an account.

I'm not sure I understand. Are you saying that it's OK for NixOS users to report bugs to, for example, the Fedora project?

Although, maybe we should consider creating a private google group for this.

That sounds great. Though, it doesn't help with private PRs from GitLab that don't pass CI on GitHub. It's a bummer to backport a fix downstream just to find out that it doesn't exactly work and backport another fix to fix the fix.

@shibumi

This comment has been minimized.

Copy link
Contributor Author

@shibumi shibumi commented Dec 18, 2018

Although, maybe we should consider creating a private google group for this. We have one for the code of conduct (completely dormant actually).

Yes, please or a private mailing list (like the strongswan devs have. They inform all big linux distributions and provide patches for security vulnerabilities a few weeks before release)

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 18, 2018

We have one for the code of conduct (completely dormant actually).

I was thinking about it and that's actually usually a bad sign when nobody reports anything (it's like ever-green monitoring that is nice to brag about but usually indicates some problems).

Anyway, regarding handling security issues, as usual, I don't think I can convince anybody of anything so let's leave it at that. If everybody else thinks that everything is fine who am I to say otherwise? The last stack buffer overflow I hid in a commit message about a year ago went unnoticed among a lot of disguised heap-buffer-overflows and memory-leaks ASan kept reporting :-)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 18, 2018

(Let me stress I don't think the security workflow we have now was great at all, it has serious flaws [such as the no-CI thing]. However it's also not a complete failure either. my ideal solution would be if github would care about open source projects enough and provide us with some much needed functionality in this area, such as private PRs/issues with CI and stuff. I am totally happy with switching to a better a approach for all of this if one pops up. In fact I even considered moving the whole shebang to GitLab because of this, but I am quite concerned about losing drive-by submitters that way — and also the work...)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 18, 2018

(adding a private mailing list sounds good to me, if anyone wants to set that up)

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 18, 2018

@poettering GitHub kind of provides a way to do it but it isn't free (which is the main problem here as far as I can see and I get it. That's exactly where caring about security stops in most cases). And I agree that moving to GitLab isn't as easy as it might seem to be. Just to be clear, I have no problem with that and it isn't the worst process I've ever seen. At some point I just realized that changing anything might make things worse so I just decided to stop keep bothering everybody. Sorry for the noise.

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 21, 2018

Bringing the discussion back to the original issue.. :)

After talking with other people about it, I'm probably going to ask for a CVE about it. The reason being that it is possible for a unprivileged user to request killing of a privileged process.

I agree that PIDFile comes from somewhat an older time, but it's still very well supported and sometimes it's the only way to work with some services that do fork (thus you need Type=forking AFAIK). Moreover, that's not about being better/worse than sysv. Systemd has the ability to stop this (it did, indeed), so it should. Assigning a CVE will make the tracking/backport (where deemed necessary) much easier. As already said, it will probably be a low/medium impact flaw, but nonetheless a flaw, because the privileges boundaries should not be crossed (otherwise, why would I run a service as user A and not directly as root, if not for privilege separation?)

Quite frankly, PID files shouldn't exist anymore. We support them for sysv compat, and they come with most of the pitfalls, security issues, lifecycle issues, PID race issues they always came with, and that's certainly not systemd's fault.

As previously said, that's true, but we shouldn't allow an unprivileged process to kill a privileged one. Yes, it could kill another process belonging to the unprivileged user, but that's not a big problem, because if we think the user is malicious, he can already kill his own processes. The important bit should be that a service with User=A, would not be able to kill a random process for User B (or at least not from root).

Well, changing creds in PID 1 is quite problematic, we'd need to fork off a stub which does setresuid(), does the kill(), then exits, and the parent must wait for it. Which is possible, but not easy to get right, since you need to be reasonably protected from the user doing stuff with your process after you dropped privs and stuff... you also need to protect yourself from RLIMIT_NPROC games and such. I mean, this is all doable, but one needs to be very careful with what you do.

Again, as said by others, I think a change of creds is happening when you do ExecStop=, right? And yes, many trick may be done by the user, but if we assume the user is malicious, he can do already whatever he wants with his own processes (e.g. he could just re-run that process, if he doesn't want to be killed by systemd etc.)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 21, 2018

After talking with other people about it, I'm probably going to ask for a CVE about it. The reason being that it is possible for a unprivileged user to request killing of a privileged process.

Well, only if both hold:

  1. PIDFile= is used, which requires privs, as it is encoded in the unit file
  2. It points to a file in a directory chown()ed to some non-root user (which also requires extra privs, as chown()ing is privileged)

Or in other words: this is only exploitable for daemons that are poorly written and write out their PID files from unprivileged code. Correct SysV daemon code will not do that: it will fork in the parent, drop privs in the child, write out the PID file in the parent, exit in the parent. Your vulnerability is in the daemons that don#t do it that way. Please file bugs against those! Any daemon that writes out PID files from unprivileged code, i.e. that needs their PID files' directory to be writable to unpriv code is doing it wrong, hence please file bugs against those services. It's in the interest of everybody, because it means you fix the bugs where they are then, and other systems benefit too which still rely on double-forking+pid-files to work correctly, i.e. RHEL 5, RHEL 6, all the BSDs, and so on.

Anyway, please understand that I strongly disagree with sticking a CVE on anything in your path. I really don't appreciate the internet bs that follows each time, that we are the target of.

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 21, 2018

Any daemon that writes out PID files from unprivileged code, i.e. that needs their PID files' directory to be writable to unpriv code is doing it wrong, hence please file bugs against those services

Why does systemd support User=unprivileged-user + Type=forking + PIDFile? Why does the documentation says that The PID file does not need to be owned by a privileged user,? It's for legacy, probably, but whatever are the reasons, it seems it needs to support them and the fact that systemd kills any process without looking at the user it belongs to is (actually, was) a flaw.

Assuming that you do need to support PIDFile for services that belong to an unprivileged user, there may be multiple ways to write to the PID file: the attacker is able to login as that user, the service is exploited somehow, etc. Now, if I exploit a service running as user X and I'm able to trick systemd into killing a service running as root, that's a flaw for me.

PIDFile= is used, which requires privs, as it is encoded in the unit file

An attacker doesn't need to write PIDFile= himself. He just needs to write into the PID file of a service that uses PIDFile=

It points to a file in a directory chown()ed to some non-root user (which also requires extra privs, as chown()ing is privileged)

The attacker doesn't need to setup the directory himself.

What I assume for the attack to be successful is: there's a service running as User X, that uses PIDFile. The attacker is somehow able to write files as user X and overwrites the content of the pid file with a random root-owned process pid. This is, I think, a supported scenario (as said, if you do not suppose that you can have a service run with User=X, then I think you should completely ignore PID files not owned by root)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 21, 2018

legacy, probably, but whatever are the reasons, it seems it needs to support them and the fact that systemd kills any process without looking at the user it belongs to is (actually, was) a flaw.

Yeah, it's a flaw in the PID file logic, how it was and still is implemented in many sysv services. They don't validate hence we didn't validate either, when we first added compat support for this.

Anyway, attaching CVEs to issues where systemd is now stricter than than sysvinit, and hence sending the signal that systemd is bad while we are actually doing good here by being better than sysvinit now, is what I think sucks hard.

I understand that for you CVE is just a handle to some security issue, but it's really not for the rest of the world. And you are in the comfortable position that any CVE you attach to stuff gets you credit points. For us it just means even more bad nasty bs on the internet. Do what you need to do, but please be aware that stuff like this just makes upstream developers unhappy, and be very protective with the information they let flow in your direction because they know that whatever trivial thing happens will be turned from a mouse into an elephant.

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 21, 2018

And you are in the comfortable position that any CVE you attach to stuff gets you credit points.

Actually that doesn't give me any point nor credit :) It's not like I discovered the flaw or anything. I just request the CVE, that's all. Nobody is going to say "ehi Riccardo has discovered flaw XXXX in systemd".

I have to do this because it is important for many people that can't follow upstream development closely, so they can see what's security relevant and what's not and in that way distributions can backport these fixes too.

And I don't think there is going to be such a storm for a Low/Medium impact flaw. I can't just hide my head under the sand and pretend I didn't see anything. The flaw is there, an attacker can use it (under the right circumstances), so pretending this is not what it is will just hurt many projects. Flaws are everywhere... you are not bad if you admit you have some.

I absolutely don't want to make systemd looks bad. The opposite! I really think this is a great project!

@kavol

This comment has been minimized.

Copy link

@kavol kavol commented Dec 21, 2018

Anyway, attaching CVEs to issues where systemd is now stricter than than sysvinit, and hence sending the signal that systemd is bad while we are actually doing good here by being better than sysvinit now, is what I think sucks hard.

well ... I don't get this logic ... a bug existed (still exists in many distributed versions) in systemd, how does a comparison to sysvinit help here? - looks like a textbook example of red herring to me ...

while testing a CVE for mysql/mariadb, I have found that systemd suffers the same problem

so I thought if this was CVE-worthy for mysql/mariadb, it's automatically CVE-worthy for systemd (the more that it affects a subset of services in the distro, not just one badly written initscript)

but it didn't get assigned/linked to my bugreport for RHEL, and when I'm checking what happened to the bug >a month after being reported, I came into middle of this discussion

now we're not considering CVE by the security impact, but rather if it "makes upstream developers unhappy" ... great, bravo, once again I'm left speechless ...

@kavol

This comment has been minimized.

Copy link

@kavol kavol commented Dec 21, 2018

I can't just hide my head under the sand and pretend I didn't see anything. The flaw is there, an attacker can use it (under the right circumstances), so pretending this is not what it is will just hurt many projects.

+1 ... I see this was opened - discussions about killing wrong process started at least - over a year ago

if it went through standard security channels, we could have already fixed it, it wouldn't need to wait for someone to discover that our distro is vulnerable as a sideeffect of other testing, like it happened to me ...

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 21, 2018

if it went through standard security channels, we could have already fixed it, it wouldn't need to wait for someone to discover that our distro is vulnerable as a sideeffect of other testing, like it happened to me

@kavol just out of curiosity, could you expand on what those "standard security channels" are? Looks like currently it's just sitting and waiting for someone else to report a security issue (ideally, with a CVE assigned) or looking for the word "CVE" on GitHub. How is this different from what is happening here?

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 21, 2018

And I don't think there is going to be such a storm for a Low/Medium impact flaw.

@ret2libc I rarely agree with @poettering but he is right here. If you're going to request a CVE, could you drop a comment here when it's assigned so that we could block the issue? Listening to every security expert from all over the world talking about how speechless they are or that systemd is going to ruin (or more likely has already ruined) the internet won't be productive and unlikely to help anybody.

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Dec 22, 2018

If you're going to request a CVE, could you drop a comment here when it's assigned so that we could block the issue?

Sure, I will.

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 22, 2018

@ret2libc thank you.

@kavol I apologize for the tone of my comments. It was absolutely uncalled for.

Don't get me wrong, in fact, I kind of like it when security researchers analyze the code and the more they do it, the better. If they think that CVEs should be assigned, then so be it (I personally have no problem with that) :-) In the end, it's systemd that's getting better bit by bit. To quote docs/HACKING.md, happy hacking!

@shibumi

This comment has been minimized.

Copy link
Contributor Author

@shibumi shibumi commented Dec 22, 2018

@ret2libc will you open CVEs for any other init system as well? They might have the same issue ;)

@eli-schwartz

This comment has been minimized.

Copy link

@eli-schwartz eli-schwartz commented Dec 23, 2018

@poettering On the security reporting thing I think the only practical solution would be to have a "real" bugtracker for systemd, because Github issues simply aren't a bugtracker -- as this discussion demonstrates. Even being able to transfer issues to/from a private repo is kind of a terrible hack.

Maybe it would be possible/make sense to get a dedicated "Community projects" subproject on the redhat bugtracker and simply use this? Especially if you're recommending people use that very tracker as basically an upstream tracker anyway.

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 23, 2018

@eli-schwartz I'm not sure what a subproject on the redhat bugtracker is and how people who have nothing to do with RH can get access to it. Would it help to make sure that patches from that bugtracker will pass CI on GitHub when they are made public?

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 23, 2018

Plus as far as I understand it's already possible to open private issues on GitLab (it's just not documented anywhere).

@eli-schwartz

This comment has been minimized.

Copy link

@eli-schwartz eli-schwartz commented Dec 23, 2018

https://bugzilla.redhat.com it's a bugzilla with an extension called Bugzilla::Extension::ExternalBugs that allows you to track the statuses of external trackers from a wide variety of sources including Github issues/PRs. It should be feasible to handle PRs here, and issues there -- assuming systemd wishes to do so and can get the necessary bureaucratic bits together to create a https://bugzilla.readthedocs.io/en/5.0/administering/categorization.html#products with an associated group for the systemd developers with edit rights.

Or set up a systemd-specific external tracker with more capability than the github one -- the problem is finding one that you don't have to self-host. :p

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 23, 2018

@eli-schwartz thank you. I see. In principle it could be used instead of a private mailing list suggested earlier to report issues privately and track them everywhere at the same time. But after spending some time trying to log in there (unsuccessfully) I think it's unnecessarily complicated. In theory, the process of reporting an issue should be as easy as possible and a private mailing list would probably be more useful.

@evverx

This comment has been minimized.

Copy link
Member

@evverx evverx commented Dec 23, 2018

@eli-schwartz just to clarify, it's not that I can't create an account there. I've just received a confirmation email so I'm all set but I have already forgotten what I wanted to report :-)

@poettering

This comment has been minimized.

Copy link
Member

@poettering poettering commented Dec 27, 2018

Maybe it would be possible/make sense to get a dedicated "Community projects" subproject on the redhat bugtracker and simply use this? Especially if you're recommending people use that very tracker as basically an upstream tracker anyway.

(Quite frankly, I think bugzilla is an abomination. It's UI is so bad, probably matching its choice of its programming language: perl. I am very sure there are better bug trackers than github, but I am also very sure that bugzilla is not it and a step backwards, not forward)

@ret2libc

This comment has been minimized.

Copy link
Contributor

@ret2libc ret2libc commented Jan 2, 2019

CVE-2018-16888 has been assigned to this flaw.

See https://bugzilla.redhat.com/show_bug.cgi?id=1662867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.