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

ListenStream doesn't honor SELinux policy changes until restart #9997

Closed
outergod opened this issue Sep 3, 2018 · 9 comments
Closed

ListenStream doesn't honor SELinux policy changes until restart #9997

outergod opened this issue Sep 3, 2018 · 9 comments

Comments

@outergod
Copy link

outergod commented Sep 3, 2018

systemd version the issue has been seen with

systemd-238-9.git0e0aa59.fc28.x86_64

Used distribution

Fedora 28 Workstation

Expected behaviour you didn't see

After modifying SELinux file context policies through either semanage fcontext -a or installing a new module with semodule -i, attempting to start a socket unit would attempt to create parent directories as well as the socket itself defined in the ListenStream directive using the matching file context.

Unexpected behaviour you saw

SELinux denial until after a system reboot, after which starting the socket correctly picks up the new context.

Steps to reproduce the problem

getenforce # -> Enforcing
semanage fcontext -a -t var_run_t '/test(/.*)?'

cat >> /usr/lib/systemd/system/foo.socket <<'EOF'
[Unit]
Description=Foo
Before=multi-user.target

[Socket]
ListenStream=/test/socket

[Install]
WantedBy=sockets.target
EOF

cat >> /usr/lib/systemd/system/foo.service <<'EOF'
[Unit]
Description=Foo

[Service]
ExecStart=/usr/bin/true
EOF

systemctl daemon-reload
systemctl start foo.socket # fails, SELinux denial; strace -p1 -f -s 256 shows that /test and the socket itself are attempted to be created as default_t

reboot

systemctl start foo.socket # works; strace -p1 -f -s 256 shows that /test and the socket itself are attempted to be created as var_run_t

Context: Creating sockets outside /var/run is required to support the new, distribution-independent Nix multi-user installer which uses its own hierarchy under /nix but requires integration with the host's own init system.

@sourcejedi
Copy link
Contributor

sourcejedi commented Sep 3, 2018

I haven't looked at the details, but I wonder if systemctl daemon-reexec works for you?

@outergod
Copy link
Author

outergod commented Sep 3, 2018

@sourcejedi I can confirm that systemctl daemon-reexec appears to pick up the change without rebooting. Thanks.

Is this by design or documented anywhere? This seems to be an OK workaround but shouldn't there be a less overkill way of forcing systemd to re-read policies?

@sourcejedi
Copy link
Contributor

sourcejedi commented Sep 3, 2018

I don't know myself. All I can say is that this difference is not mentioned in the most obvious place :).

@outergod
Copy link
Author

outergod commented Sep 4, 2018

Please disregard the automatic reference above, I accidentally pasted the wrong link in a PR and GitHub doesn't seem to honor the fact I fixed it in the comment.

@poettering
Copy link
Member

poettering commented Sep 11, 2018

My selinux-fu is limited. We don't reload the selinux database/policy ever during runtime. "systemctl daemon-reexec" does that however as a side-effect (simply because it reexeces PID 1, i.e. flushes out all memory and starts fresh and thus has to reload it)

I am not sure we should reload the database/policy automatically, when it changes, i.e. i don#t know what the intended mode of operation of selinux is there: should consumers of the db/policy automatically flush the old one out after stat-mtime or inotify change detection or should this remain an explicit step? If the latter then there's nothing to do I think from our side, except maybe we could flush out the selinux db/policy during "systemctl daemon-reload" too. But for that we'd need some input from the selinux folks. In fact, for selinux support we usually rely on patches from the selinux community.

@mgrepl @rhatdan any input on this?

if we should flush out the db/policy during daemon-reload, what's the right series of libselinux calls to make for that? any chance you can prep a patch if this is desirable?

@rhatdan
Copy link
Contributor

rhatdan commented Sep 11, 2018

SELinux policy update should only be done during the semodule -i phase. Never done to daemon-reload.
If you modify policy and the service process is not running with the correct label as specified by the udpated policy, then you would need to restart the process or service. Then the transition rules would apply and the process would be launched with the correct label.

systemctl restart service

Is what is needed. I don't see where systemctl daemon-reload would come into play at all.

@sourcejedi
Copy link
Contributor

sourcejedi commented Sep 12, 2018

@rhatdan The example problem is a denial of file operations performed by PID 1. Specifically, mkdir() followed by bind() of a unix socket file. The newly installed policy is not about changing the label of PID 1.

Instead, the example changes the "file context mapping definitions" (as in semanage fcontext). The problem is that PID1 loads the file contexts at boot-time, systemctl daemon-reload picks up the new foo.socket unit file, but it does not pick up the new file contexts. So systemctl start foo.socket uses the old file contexts, which is wrong (and also immediately denied).

We're asking about when and how we need to reload the "file context mapping definitions".

(Edit: It sounds like SELinux-aware packages need to trigger such a reload at install time, around the same time they trigger systemd to load their new unit files. Maybe it hasn't been needed in most cases... I guess it wouldn't have been needed here if the socket directory was instead created by running systemd-tmpfiles with an appropriate config.)

@rhatdan
Copy link
Contributor

rhatdan commented Sep 13, 2018

The file context should probably be checked on each access to the socket. Since it could change at any time.

chcon -t foobar_socket_t /run/foobar.sock

@poettering
Copy link
Member

I guess this should work now that a9dfac2 is merged.

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

No branches or pull requests

4 participants