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

Make MAINPID= and PIDFile= handling more restrictive (and other stuff) #7816

Merged
merged 14 commits into from Jan 15, 2018

Conversation

4 participants
@poettering
Copy link
Member

poettering commented Jan 5, 2018

Mostly stuff to make MAINPID= sd_notify() and PIDFile= handling a bit stricter. Plus other minor stuff, some in preparation and some not.

Fixes: #6632.

int r;

assert(p);

r = sd_bus_message_read(message, "u", &v);
r = sd_bus_message_read(message, "t", &v);

This comment has been minimized.

@yuwata

yuwata Jan 5, 2018

Member

Oh, thank you...

@systemd systemd deleted a comment from Keensyst Jan 5, 2018

@@ -557,7 +557,6 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l

r = get_glinksettings(fd, &ifr, &u);
if (r < 0) {

This comment has been minimized.

@yuwata

yuwata Jan 7, 2018

Member

commit title: ehtool -> ethtool

This comment has been minimized.

@poettering

poettering Jan 8, 2018

Member

commit title: ehtool -> ethtool

pushed a new version fixing that

@poettering poettering force-pushed the poettering:chase-pid branch from 8d5879d to 627a77e Jan 8, 2018

@poettering poettering added this to the v237 milestone Jan 10, 2018

poettering added some commits Jan 4, 2018

fs-util: add new CHASE_SAFE flag to chase_symlinks()
When the flag is specified we won't transition to a privilege-owned
file or directory from an unprivileged-owned one. This is useful when
privileged code wants to load data from a file unprivileged users have
write access to, and validates the ownership, but want's to make sure
that no symlink games are played to read a root-owned system file
belonging to a different context.
fs-util: add new chase_symlinks() flag CHASE_OPEN
The new flag returns the O_PATH fd of the final component, which may be
converted into a proper fd by open()ing it again through the
/proc/self/fd/xyz path.

Together with O_SAFE this provides us with a somewhat safe way to open()
files in directories potentially owned by unprivileged code, where we
want to refuse operation if any symlink tricks are played pointing to
privileged files.
manager: swap order in which we ellipsize/escape sd_notify() messages…
… for debugging

If we have to chose between truncated escape sequences and strings
exploded to 4 times the desried length by fully escaping, prefer the
latter.

It's for debug only, hence doesn't really matter much.
dbus-util: properly parse timeout values
This makes transient TimeoutStopSec= properties work. After all they are
64bit entitites, not 32bit ones.
core: be stricter when handling PID files and MAINPID sd_notify() mes…
…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: #6632
sd-dameon: also sent ucred when our UID differs from EUID
Let's be explicit, and always send the messages from our UID and never
our EUID. Previously this behaviour was conditionalized only on whether
the PID was specified, which made this non-obvious.
notify: add new --uid= command
The new --uid= switch allows selecting the UID from which the
notificaiton messages shall originate.

This is primarily useful for testing purposes, but might have other
uses.
ethtool-util: don't pass fds as pointers if we don't have to
Passing them as pointers is just weird, hence don't do it
cocci: there's not ENOTSUP, there's only EOPNOTSUPP
On Linux the former is a compat alias to the latter, and that's really
weird, as inside the kernel the two are distinct. Which means we really
should stay away from it.

@poettering poettering force-pushed the poettering:chase-pid branch from 627a77e to 6b44a12 Jan 11, 2018

@keszybz keszybz merged commit e0b6d3c into systemd:master Jan 15, 2018

3 checks passed

Fedora Rawhide CI x86_64 rpm build [succeeded]
Details
Fedora Rawhide Compose x86_64
Details
semaphoreci The build passed on Semaphore.
Details
@vincentbernat

This comment has been minimized.

Copy link

vincentbernat commented Feb 28, 2018

Isn't there a race condition in the checks? Suppose I am forking a new main process (with a new configuration). The old one stays around for cleanup purpose (waiting for old connections to terminate). It signals the new main process with sd_notify(). Unfortunately, the new main process dies early. systemd says the PID is not a valid one and MAINPID stays unchanged. The service won't be correctly restarted until the old main process terminates its cleanup.

If there was a way for systemd to acknowledge the MAINPID change somehow, this would fix the problem (old main process would pause new main process until the acknowledgment).

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Feb 28, 2018

We probably should add proper dbus APIs for the various things you can do with sd_notify(), and that would be naturally synchronous, and could return error. However, I wonder if it would be as attractive to people, given that D-Bus isn't universally loved still...

In the scenario you describe things wouldn't be too bad, after all things would eventually recover. But of course that could be delayed arbitrarily long...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment