Created attachment 547970 [details] diagnostic info (errors, AVCs, policies) for syslog-ng init script selinux-enabled profiles have checkpath (/lib64/rc/bin/checkpath) as tmpfiles_exec_t The following commit introduces read() instead of lstat in multiple places and breaks selinux for multiple init scripts (e.g. syslog-ng): https://github.com/OpenRC/openrc/commit/1771bc2a83fe65bfe6ec3e93ea7632609e697a38#diff-eef8b936498d0198be4c7fd761836d3a specifically, this first open()s fails because read() is not permitted, so checkpath assumes it doesn't exist and tries to create it (and fail) I have not tagged this SELinux as I am not sure where the bug is: - syslog-ng should ignore if checkpath doesn't work? The daemon will probably fail on its own if that's the case - hardened-refpolicy should allow tmpfiles_t read access to everything? Seems like a bad idea - OpenRC checkpath should avoid using read to see if a file is present, perhaps there is another way to fix https://github.com/OpenRC/openrc/issues/195 ?
related bug https://github.com/OpenRC/openrc/issues/201
That commit does not introduce and read() calls whatsoever. It does call open() with O_RDONLY, which is probably what triggers selinux. This could probably be avoided by using O_PATH in place of O_RDONLY. However, I would also blame the selinux policy for being overly conservative here. The policy should be tailored to meet the requirements of the tools to which it is applied.
i mixed up: read() -> open() read -> read permission From the perspective of a SELinux user: I respectfully disagree, SELinux should aim to be conservative as that is the reason people use it at the cost of extra overhead (i.e. because it was conservative, selinux-enabled profiles didn't suffer from the 2 previous github security issues)
(In reply to dkjii from comment #3) If the policy breaks the tool, the policy should be adjusted.
Using O_PATH would indeed fix this issue. In general, I agree with floppym that the policy should be tailored to the tool, not the other way round, but in this case, it'd be great if this could be fixed in OpenRC -- Using O_PATH shouldn't incur any disadvantages (other than breaking compatibility with non-Linux systems, which can be mitigated by only using the flag if it's defined), and it'd allow us to keep our policy more restrictive.
(In reply to Luis Ressel from comment #5) > Using O_PATH would indeed fix this issue. > > In general, I agree with floppym that the policy should be tailored to the > tool, not the other way round, but in this case, it'd be great if this could > be fixed in OpenRC -- Using O_PATH shouldn't incur any disadvantages (other > than breaking compatibility with non-Linux systems, which can be mitigated > by only using the flag if it's defined), and it'd allow us to keep our > policy more restrictive. My concern is that using O_PATH would require me to define _GNU_SOURCE, which OpenRC doesn't currently define, and I'm not sure what other portability issues that would lead to in the future.
Unfortunately, fchmod does not work on file descriptors created with O_PATH. openat(AT_FDCWD, "/tmp/foo", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3 fchmod(3, 0755) = -1 EBADF (Bad file descriptor) systemd calls chmod("/proc/self/fd/N", ...) to work around this.
(In reply to William Hubbs from comment #6) > My concern is that using O_PATH would require me to define _GNU_SOURCE, > which OpenRC doesn't currently define, and I'm not sure what other > portability issues that would lead to in the future. Good point. (In reply to Mike Gilbert from comment #7) > systemd calls chmod("/proc/self/fd/N", ...) to work around this. That's an annoying limitation. I'm all for avoiding ugly hacks like this. Thanks for your investigation! I'll take care of the SELinux policy fix.
We've decided not to add any new permissions to our policy to address this bug. checkpath has never been permitted to create paths in /var/lib. The only thing which the OpenRC commit mentioned above has changed is that denials are now logged on every start of the affected init scripts, instead of just when checkpath tries to create any missing files/dirs. Users have two options: 1) Enable the tmpfiles_manage_all_non_security boolean to grant checkpath access on /var/lib/ data (and lots of other stuff too, see "seinfo -xanon_security_file_type for an exhaustive list"). 2) Manually make sure the paths being checked by the init scripts exist, and ignore the denials, or remove the checkpath calls from your init scripts altogether. (If you can provide the SELinux team with a list of types for which the denials occur, such as syslogd_var_lib_t, we'll add dontaudit rules to our policy.) Personally, I don't think it makes much sense to call checkpath for static locations (i.e. those which will survive a reboot) anyway. If a daemon requires that a certain directory be present, you might just as well create it in the corresponding ebuild. @OpenRC, feel free to close this bug.
I support the selinux's team decision, but if that is the case, maybe the bug should be reassigned instead of closed The previous behavior was that checkpath would fail if the permissions were not met or if the file was inexistant Now checkpath will fail in all case, even if the path exists and has the right permissions, which will prevent (i assume) a lot of daemon from starting on selinux-enabled profiles The initial bug was that syslog-ng was not starting with the newest stable changes, openrc-0.38 should at least be masked or marked unstable in selinux-enabled profiles I am not sure how to proceed for checkpath. My guess is that checkpath should always be optional in init scripts, since most daemon will fail if they cannot write their tmpfiles, so modifying the failing init scripts might be the better solution? (e.g. always add a checkpath || einfo "checkpath failed"?)
Oh wow, it seems I missed something important. I hadn't realized this was preventing services from starting altogether. Sorry! Making the affected checkpath calls non-fatal seems like a good way forward.
On the other hand, this is a rather serious breakage, and it might take a while to fix all affected init scripts. We could handle this on the SELinux end by enabling the "tmpfiles_manage_all_non_security" boolean by default, hence allowing checkpath to work correctly in all cases. Users who don't like granting tmpfiles_t those additional permissions could still disable this boolean on their systems, but then it'd be their personal responsibility to avoid this problem by removing the affected checkpath calls from their local init scripts.
(In reply to Luis Ressel from comment #12) > We could handle this on the SELinux end by enabling the > "tmpfiles_manage_all_non_security" boolean by default, hence allowing > checkpath to work correctly in all cases. perfinion agreed with my approach. There will be a new selinux policy release soon to address this issue.
I don't really expect this to be merged given the negative feedback thus far, but I did manage to get checkpath working with O_PATH. https://github.com/OpenRC/openrc/pull/244
commit 2af0cedd5952d7da71681b7a636dff3540e4295d Author: Mike Gilbert <floppym@gentoo.org> AuthorDate: Sat Dec 1 23:36:27 2018 -0500 Commit: Mike Frysinger <vapier@gmail.com> CommitDate: Sat Dec 1 21:43:18 2018 -0800 checkpath: use O_PATH when available This avoids opening directories/files with read permission, which is sometimes rejected by selinux policy. Bug: https://bugs.gentoo.org/667122
(In reply to Luis Ressel from comment #9) > We've decided not to add any new permissions to our policy to address this > bug. > > checkpath has never been permitted to create paths in /var/lib. The only > thing which the OpenRC commit mentioned above has changed is that denials > are now logged on every start of the affected init scripts, instead of just > when checkpath tries to create any missing files/dirs. checkpath has always created missing files, dirs and fifos. It also adjusts ownership and permissions. > Users have two options: > 1) Enable the tmpfiles_manage_all_non_security boolean to grant checkpath > access on /var/lib/ data (and lots of other stuff too, see "seinfo > -xanon_security_file_type for an exhaustive list"). > > 2) Manually make sure the paths being checked by the init scripts exist, and > ignore the denials, or remove the checkpath calls from your init scripts > altogether. (If you can provide the SELinux team with a list of types for > which the denials occur, such as syslogd_var_lib_t, we'll add dontaudit > rules to our policy.) > > Personally, I don't think it makes much sense to call checkpath for static > locations (i.e. those which will survive a reboot) anyway. If a daemon > requires that a certain directory be present, you might just as well create > it in the corresponding ebuild. I disagree strongly with this, because that means you would have to re-emerge the package if some static file, directory or fifo is missing that you need re-created. > > @OpenRC, feel free to close this bug.
I officially give up on this bug report. William's hard-line stance on _GNU_SOURCE is simply unreasonable, and I'm done arguing about it.
commit ff4af908a58eedf9a165946f109f06add23fff9c Author: William Hubbs <w.d.hubbs@gmail.com> AuthorDate: Sun Dec 2 16:08:42 2018 -0600 Commit: William Hubbs <w.d.hubbs@gmail.com> CommitDate: Sun Dec 2 16:08:42 2018 -0600 Revert "checkpath: use O_PATH when available" This reverts commit 2af0cedd5952d7da71681b7a636dff3540e4295d. After speaking with Luis Ressel on the Gentoo selinux team, I am reverting this commit for the following reasons: - Luis told me that he feels this is not the solution we need to address the concern with checkpath; I will be working with him on another solution. - There are concerns about the way the path variable was handled and the assert() call. The path variable should be dynamically allocated using xasprintf instead of defining a length at compile time. This would eliminate the need for the assert() call. - It introduces the definition of _GNU_SOURCE which makes it easier to introduce portability concerns in the future (see #262).
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/hardened-refpolicy.git/commit/?id=892c088f75d2df27a501850dae2ef05c8759a591 commit 892c088f75d2df27a501850dae2ef05c8759a591 Author: Luis Ressel <aranea@aixah.de> AuthorDate: 2018-10-03 17:10:39 +0000 Commit: Jason Zaman <jason@perfinion.com> CommitDate: 2018-11-18 10:59:17 +0000 Enable the tmpfiles_manage_all_non_security boolean by default This sucks, not only because I don't like granting tmpfiles_t this access, but also since it's one more unneccessary difference between gentoo and refpolicy. Nevertheless, it's the most reasonable fix I can think of. Bug: https://bugs.gentoo.org/667122 Signed-off-by: Jason Zaman <jason@perfinion.com> policy/modules/system/tmpfiles.te | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
i think it's been resolved by: https://github.com/OpenRC/openrc/commit/b6fef599bf8493480664b766040fa9b0d4b1e335 plus, it's a bit moot as services should migrate to tmpfiles.d and not use checkpath at all in the first place.
