On Fri, Apr 14, 2017 at 8:49 PM, Dominick Grift <dac.override@xxxxxxxxx> wrote:
> On Fri, Apr 14, 2017 at 01:56:30PM -0400, Stephen Smalley wrote:
>> On Fri, 2017-04-14 at 13:47 -0400, Daniel Walsh wrote:
>> > On 04/14/2017 11:33 AM, Stephen Smalley wrote:
>> > > On Fri, 2017-04-14 at 16:57 +0200, Dominick Grift wrote:
>> > > > Bear with me please, because i might not fully grasp the issue (i
>> > > > received help with diagnosing this issue):
>> > > >
>> > > > This commit causes issues (and is, i think, a lousy hack):
>> > > > e3cab998b48ab293a9962faf9779d70ca339c65d
>> > > >
>> > > > The commit causes entities to "think" that SELinux is disabled
>> > > > after
>> > > > "mount -o remount,ro /sys/fs/selinux
>> > > >
>> > > > It is "neat" to be able to make processes "think" that selinux is
>> > > > disabled on a selinux enabled system but not if it break anything
>> > > >
>> > > > The above results in the following:
>> > > >
>> > > > Systemd services that have ProtectKernelTunables=yes set in their
>> > > > respective service units, think that SELinux is disabled.
>> > > >
>> > > > However we have found that some of these services actually rely
>> > > > on
>> > > > SELinux to ensure proper labeling.
>> > > >
>> > > > So we have the option to make people aware that if you set
>> > > > ProtectKernelTunables=yes that then the process cannot be
>> > > > SELinux-
>> > > > aware properly, or we can just get rid of the commit above and
>> > > > just
>> > > > accept that process know that SELinux is enabled.
>> > > >
>> > > > Actual bug that caused me to look into this: systemd-localed
>> > > > selinux
>> > > > awareness is broken due it having ProtectKernelTunables=yes in
>> > > > its
>> > > > service unit
>> > >
>> > > If selinuxfs is mounted read-only, then they can't use most of the
>> > > selinuxfs interfaces, including even the ability to validate or
>> > > canonicalize security contexts. That will break most SELinux-aware
>> > > services if we tell them that SELinux is enabled. Are you sure
>> > > systemd-localed would actually work if you told it SELinux was
>> > > enabled
>> > > when selinuxfs was mounted read-only? What SELinux interfaces is
>> > > it
>> > > using?
>> > >
>> > > The other question is whether ProtectKernelTunables ought to be
>> > > mounting selinuxfs read-only. SELinux already controls the ability
>> > > to
>> > > use its interfaces, including limiting even root, so it is unclear
>> > > what
>> > > benefit we derive from having systemd add a further restriction on
>> > > top.
>> > >
>> >
>> > Why is selinuxfs mounted readonly in this case?
>>
>> I don't actually see this in upstream systemd unless I am just missing
>> it.
>>
>> systemd/src/core/namespace.c:
>> /* ProtectKernelTunables= option and the related filesystem APIs */
>> static const MountEntry protect_kernel_tunables_table[] = {
>> { "/proc/sys", READONLY, false },
>> { "/proc/sysrq-trigger", READONLY, true },
>> { "/proc/latency_stats", READONLY, true },
>> { "/proc/mtrr", READONLY, true },
>> { "/proc/apm", READONLY, true }, /* Obsolete
>> API, there's no point in permitting access to this, ever */
>> { "/proc/acpi", READONLY, true },
>> { "/proc/timer_stats", READONLY, true },
>> { "/proc/asound", READONLY, true },
>> { "/proc/bus", READONLY, true },
>> { "/proc/fs", READONLY, true },
>> { "/proc/irq", READONLY, true },
>> { "/sys", READONLY, false },
>> { "/sys/kernel/debug", READONLY, true },
>> { "/sys/kernel/tracing", READONLY, true },
>> { "/sys/fs/cgroup", READWRITE, false }, /* READONLY is
>> set by ProtectControlGroups= option */
>> };
>>
>> No mention of selinuxfs at all. Maybe it is a Fedora patch?
>>
>> > The reason we want this is so that processes inside of containers do
>> > not
>> > attempt to do SELinux stuff.
>> >
>> > http://danwalsh.livejournal.com/73099.html
>
> Before one dismisses my concern (8 minute proof):
>
> https://www.youtube.com/watch?v=YqiM1MlOG0w
Hello,
I see this on Arch Linux as well, where there is no
distribution-specific patch which is applied to systemd (the only
patches which are applied are backported commits). A simple way to see
that the selinuxfs is mounted read-only is the following command:
"localectl && findmnt --task $(pgrep systemd-localed)". It will
display the mountpoints of systemd-localed.service, which (with
systemd 232 [1]) contains:
├─/sys sys sysfs
ro,nosuid,nodev,noexec,relatime,seclabel
│ ├─/sys/firmware/efi/efivars efivarfs efivarfs
ro,nosuid,nodev,noexec,relatime
│ ├─/sys/kernel/security securityfs securityfs
ro,nosuid,nodev,noexec,relatime
│ ├─/sys/fs/selinux selinuxfs selinuxfs
ro,relatime
│ ├─/sys/fs/cgroup tmpfs tmpfs
ro,nosuid,nodev,noexec,seclabel,mode=755
│ │ ├─/sys/fs/cgroup/systemd cgroup cgroup
ro,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=
│ │ ├─/sys/fs/cgroup/net_cls cgroup cgroup
ro,nosuid,nodev,noexec,relatime,net_cls
│ │ ├─/sys/fs/cgroup/perf_event cgroup cgroup
ro,nosuid,nodev,noexec,relatime,perf_event
│ │ ├─/sys/fs/cgroup/pids cgroup cgroup
ro,nosuid,nodev,noexec,relatime,pids
│ │ ├─/sys/fs/cgroup/blkio cgroup cgroup
ro,nosuid,nodev,noexec,relatime,blkio
│ │ ├─/sys/fs/cgroup/freezer cgroup cgroup
ro,nosuid,nodev,noexec,relatime,freezer
│ │ ├─/sys/fs/cgroup/cpu,cpuacct cgroup cgroup
ro,nosuid,nodev,noexec,relatime,cpu,cpuacct
│ │ ├─/sys/fs/cgroup/cpuset cgroup cgroup
ro,nosuid,nodev,noexec,relatime,cpuset
│ │ ├─/sys/fs/cgroup/devices cgroup cgroup
ro,nosuid,nodev,noexec,relatime,devices
│ │ └─/sys/fs/cgroup/memory cgroup cgroup
ro,nosuid,nodev,noexec,relatime,memory
│ ├─/sys/fs/pstore pstore pstore
ro,nosuid,nodev,noexec,relatime,seclabel
│ ├─/sys/kernel/debug debugfs debugfs
ro,relatime,seclabel
│ ├─/sys/kernel/config configfs configfs
ro,relatime
│ └─/sys/fs/fuse/connections fusectl fusectl
ro,relatime
/sys/fs/selinux is mounted read-only. Moreover when I run "strace -f
-p 1 -e mount" while starting systemd-localed.service, I get:
3401 mount(NULL, "/sys/fs/cgroup/perf_event", NULL,
MS_RDONLY|MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_REMOUNT|MS_BIND, NULL) = 0
3401 mount(NULL, "/sys/fs/cgroup/blkio", NULL,
MS_RDONLY|MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_REMOUNT|MS_BIND, NULL) = 0
3401 mount(NULL, "/sys/fs/cgroup/pids", NULL,
MS_RDONLY|MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_REMOUNT|MS_BIND, NULL) = 0
3401 mount(NULL, "/sys/fs/selinux", NULL,
MS_RDONLY|MS_REMOUNT|MS_BIND, NULL) = 0
3401 mount(NULL, "/sys/fs/cgroup", NULL,
MS_RDONLY|MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_REMOUNT|MS_BIND, NULL) = 0
3401 mount(NULL, "/sys/kernel/debug", NULL,
MS_RDONLY|MS_REMOUNT|MS_BIND, NULL) = 0
...
So /sys/fs/selinux *is* remounted read-only by systemd. When I remove
"ProtectKernelTunables=yes" from the unit file, /sys/fs/selinux is not
remounted and is kept RW in the namespace of the service.
About containers, in http://danwalsh.livejournal.com/73099.html there
is: "In containers we don't mount these file systems by default or we
mount it read/only causing libselinux to report that it is disabled.".
Why does /sys/fs/selinux need to be mounted read-only instead of not
been mounted at all?
About systemd-localed, its use of namespaces makes it "look like" a
container, but it needs to be SELinux-aware in order to use
/proc/thread-self/attr/fscreate. The use-case is to atomically create
files like /etc/vconsole.conf with the right context. In order to do
so, the service:
* loads the file context database,
* requests the expected context of /etc/vconsole.conf (selabel_lookup_raw),
* configures the fscreate context (setfscreatecon_raw)
* creates a temporary file with this context named for example
"/etc/.#vconsole.confiYiPml",
* writes data to it and closes it,
* and finally renames it to /etc/vconsole.conf (with the rename syscall)
I am not aware of a way of making /etc/vconsole.conf have the right
file context in the end without making the program use libselinux's
API (named type_transition does not support patterns suitable for
temporary files). Did I miss something?
Anyway, there is a bug in vanilla code (it is not specific to Fedora)
and it is not clear whether it is a bug in libselinux code or in
systemd's one. Is it's libselinux, I have prepared a patch for it
(attached). Otherwise, what does systemd did wrong in its use of the
SELinux API?
Nicolas
[1] ProtectKernelTunables=yes has actually been introduced in systemd
232 with https://github.com/systemd/systemd/commit/0c28d51ac84973904e5f780b024adf8108e69fa1
From 92688d49f16a28875034c95827fbe0d20e221d7b Mon Sep 17 00:00:00 2001
From: Nicolas Iooss <nicolas.iooss@xxxxxxx>
Date: Fri, 14 Apr 2017 21:27:42 +0200
Subject: [PATCH 1/1] libselinux: detect that SELinux is enabled when
/sys/fs/selinux is mounted read-only
systemd service units can use "ProtectKernelTunables=yes" in order to
mount some file systems read-only (the documentation is available at
https://www.freedesktop.org/software/systemd/man/systemd.exec.html).
This makes /sys/fs/selinux read-only too.
Services using such a configuration option sees SELinux as disabled
because of the behavior described in commit e3cab998b48a ("libselinux
mountpoint changing patch."):
NOTE: We added the check for RO, to allow tools like mock to be
able to tell a chroot that SELinux is disabled while enforcing it
outside the chroot.
However this changes the behavior of some systemd services in unexpected
ways. For example systemd-localed uses libselinux in order to create
/etc/locale.conf, /etc/vconsole.conf... with the right file context
while using a temporary file (using setfscreatecon_raw() with the label
of /etc/vconsole.conf). With ProtectKernelTunables=yes, systemd-localed
sees SELinux as being disabled (because /sys/fs/selinux is mounted
read-only) and creates files in /etc with a wrong label.
Fix this issue by making verify_selinuxmnt() use read-only mount-points
too.
Reported-by: Dominick Grift <dac.override@xxxxxxxxx>
Signed-off-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
---
libselinux/src/init.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/libselinux/src/init.c b/libselinux/src/init.c
index 814c7e6a9964..4fca2b9c6ecd 100644
--- a/libselinux/src/init.c
+++ b/libselinux/src/init.c
@@ -42,9 +42,7 @@ static int verify_selinuxmnt(const char *mnt)
struct statvfs vfsbuf;
rc = statvfs(mnt, &vfsbuf);
if (rc == 0) {
- if (!(vfsbuf.f_flag & ST_RDONLY)) {
- set_selinuxmnt(mnt);
- }
+ set_selinuxmnt(mnt);
return 0;
}
}
--
2.12.0
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
