* let's revert e3cab998b48ab293a9962faf9779d70ca339c65d @ 2017-04-14 14:57 Dominick Grift 2017-04-14 15:33 ` Stephen Smalley 0 siblings, 1 reply; 15+ messages in thread From: Dominick Grift @ 2017-04-14 14:57 UTC (permalink / raw) To: selinux [-- Attachment #1: Type: text/plain, Size: 1295 bytes --] 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 -- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 14:57 let's revert e3cab998b48ab293a9962faf9779d70ca339c65d Dominick Grift @ 2017-04-14 15:33 ` Stephen Smalley 2017-04-14 15:45 ` Dominick Grift 2017-04-14 17:47 ` Daniel Walsh 0 siblings, 2 replies; 15+ messages in thread From: Stephen Smalley @ 2017-04-14 15:33 UTC (permalink / raw) To: Dominick Grift, selinux 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 15:33 ` Stephen Smalley @ 2017-04-14 15:45 ` Dominick Grift 2017-04-14 17:47 ` Daniel Walsh 1 sibling, 0 replies; 15+ messages in thread From: Dominick Grift @ 2017-04-14 15:45 UTC (permalink / raw) To: selinux [-- Attachment #1: Type: text/plain, Size: 2819 bytes --] On Fri, Apr 14, 2017 at 11:33:25AM -0400, 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? AFAIK its just getfilecon/setfilecon to ensure proper labeling of various files in /etc that it creates with random names. I understand that removing ProtectKernelTunables=yes atleast makes the functionality that I identified to have been broken before work. Whether something was overlooked. I do not know. > > 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. > These are questions that I cannot answer. I am not sure whether systemd mounts selinux r/o as part of ProtectKernelTunables, but I understand that it does. I agree that if it does that it should be do that in the first place. -- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 15:33 ` Stephen Smalley 2017-04-14 15:45 ` Dominick Grift @ 2017-04-14 17:47 ` Daniel Walsh 2017-04-14 17:56 ` Stephen Smalley 1 sibling, 1 reply; 15+ messages in thread From: Daniel Walsh @ 2017-04-14 17:47 UTC (permalink / raw) To: Stephen Smalley, Dominick Grift, selinux 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? 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 17:47 ` Daniel Walsh @ 2017-04-14 17:56 ` Stephen Smalley 2017-04-14 18:07 ` Dominick Grift 2017-04-14 18:49 ` Dominick Grift 0 siblings, 2 replies; 15+ messages in thread From: Stephen Smalley @ 2017-04-14 17:56 UTC (permalink / raw) To: dwalsh, Dominick Grift, selinux 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 17:56 ` Stephen Smalley @ 2017-04-14 18:07 ` Dominick Grift 2017-04-14 18:49 ` Dominick Grift 1 sibling, 0 replies; 15+ messages in thread From: Dominick Grift @ 2017-04-14 18:07 UTC (permalink / raw) To: selinux [-- Attachment #1: Type: text/plain, Size: 4513 bytes --] 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? Do you see anything else in there that might cause libselinux linked processes to think selinux is disabled? getfilecon/setfilecon does not work with ProtectKernelTunables=yes. There is no mention of it in the (debug) log. It just skips the whole getfilecon/setfilecon step > > > 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 -- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 17:56 ` Stephen Smalley 2017-04-14 18:07 ` Dominick Grift @ 2017-04-14 18:49 ` Dominick Grift 2017-04-14 19:43 ` Nicolas Iooss 1 sibling, 1 reply; 15+ messages in thread From: Dominick Grift @ 2017-04-14 18:49 UTC (permalink / raw) To: selinux [-- Attachment #1: Type: text/plain, Size: 4328 bytes --] 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 -- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 18:49 ` Dominick Grift @ 2017-04-14 19:43 ` Nicolas Iooss 2017-04-14 20:41 ` Stephen Smalley 0 siblings, 1 reply; 15+ messages in thread From: Nicolas Iooss @ 2017-04-14 19:43 UTC (permalink / raw) To: selinux, Stephen Smalley, Daniel J Walsh, Dominick Grift [-- Attachment #1: Type: text/plain, Size: 9555 bytes --] On Fri, Apr 14, 2017 at 8:49 PM, Dominick Grift <dac.override@gmail.com> 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 [-- Attachment #2: 0001-libselinux-detect-that-SELinux-is-enabled-when-sys-f.patch --] [-- Type: text/x-patch, Size: 2029 bytes --] From 92688d49f16a28875034c95827fbe0d20e221d7b Mon Sep 17 00:00:00 2001 From: Nicolas Iooss <nicolas.iooss@m4x.org> 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@gmail.com> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 19:43 ` Nicolas Iooss @ 2017-04-14 20:41 ` Stephen Smalley 2017-04-15 10:23 ` Daniel Walsh 0 siblings, 1 reply; 15+ messages in thread From: Stephen Smalley @ 2017-04-14 20:41 UTC (permalink / raw) To: Nicolas Iooss, selinux, Daniel J Walsh, Dominick Grift On Fri, 2017-04-14 at 21:43 +0200, Nicolas Iooss wrote: > On Fri, Apr 14, 2017 at 8:49 PM, Dominick Grift <dac.override@gmail.c > om> 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. Hmmm...so is systemd just recursively bind mounting everything under /sys as read-only? If so, why does it separately list /sys/kernel/debug and /sys/kernel/tracing in protect_kernel_tunables_table[]? > 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? I'll defer that one to Dan. > 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? Hmm...this is fragile. Suppose for instance that systemd were to start passing SELABEL_OPT_VALIDATE to selabel_open(). That would trigger failures because it wouldn't be able to write the context to /sys/fs/selinux/context to validate them. Or if it were using matchpathcon(), which writes the context to /sys/fs/selinux/context and reads back the canonicalized context for use (not sure why we stopped doing that in selabel_lookup; maybe that's a bug). > 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? With regard to the patch, Dan or others would have to assess the compatibility implications, since there are userspace components now relying on is_selinux_enabled() to return 0 if selinuxfs is read-only. With regard to use of the SELinux API, we've never guaranteed that a subset of the API will work if selinuxfs is not available or is read- only. Obviously parts of it are usable, but that seems fragile. I don't really think systemd ought to be remounting it read-only, but maybe that's just me. > Nicolas > > [1] ProtectKernelTunables=yes has actually been introduced in systemd > 232 with https://github.com/systemd/systemd/commit/0c28d51ac84973904e > 5f780b024adf8108e69fa1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-14 20:41 ` Stephen Smalley @ 2017-04-15 10:23 ` Daniel Walsh 2017-04-15 14:10 ` Nicolas Iooss 2017-04-17 13:34 ` Stephen Smalley 0 siblings, 2 replies; 15+ messages in thread From: Daniel Walsh @ 2017-04-15 10:23 UTC (permalink / raw) To: Stephen Smalley, Nicolas Iooss, selinux, Dominick Grift On 04/14/2017 04:41 PM, Stephen Smalley wrote: > On Fri, 2017-04-14 at 21:43 +0200, Nicolas Iooss wrote: >> On Fri, Apr 14, 2017 at 8:49 PM, Dominick Grift <dac.override@gmail.c >> om> 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. > Hmmm...so is systemd just recursively bind mounting everything under > /sys as read-only? If so, why does it separately list /sys/kernel/debug > and /sys/kernel/tracing in protect_kernel_tunables_table[]? > >> 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? > I'll defer that one to Dan. I believe that libselinux still reports that the system is running with SELinux, if the selinuxfs is not mounted inside of the container 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? > Hmm...this is fragile. Suppose for instance that systemd were to start > passing SELABEL_OPT_VALIDATE to selabel_open(). That would trigger > failures because it wouldn't be able to write the context to > /sys/fs/selinux/context to validate them. Or if it were using > matchpathcon(), which writes the context to /sys/fs/selinux/context and > reads back the canonicalized context for use (not sure why we stopped > doing that in selabel_lookup; maybe that's a bug). > >> 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? > With regard to the patch, Dan or others would have to assess the > compatibility implications, since there are userspace components now > relying on is_selinux_enabled() to return 0 if selinuxfs is read-only. > > With regard to use of the SELinux API, we've never guaranteed that a > subset of the API will work if selinuxfs is not available or is read- > only. Obviously parts of it are usable, but that seems fragile. I > don't really think systemd ought to be remounting it read-only, but > maybe that's just me. > >> Nicolas >> >> [1] ProtectKernelTunables=yes has actually been introduced in systemd >> 232 with https://github.com/systemd/systemd/commit/0c28d51ac84973904e >> 5f780b024adf8108e69fa1 > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-15 10:23 ` Daniel Walsh @ 2017-04-15 14:10 ` Nicolas Iooss 2017-04-17 13:34 ` Stephen Smalley 1 sibling, 0 replies; 15+ messages in thread From: Nicolas Iooss @ 2017-04-15 14:10 UTC (permalink / raw) To: Daniel J Walsh, Stephen Smalley, Dominick Grift; +Cc: selinux On Sat, Apr 15, 2017 at 12:23 PM, Daniel Walsh <dwalsh@redhat.com> wrote: > On 04/14/2017 04:41 PM, Stephen Smalley wrote: >> On Fri, 2017-04-14 at 21:43 +0200, Nicolas Iooss wrote: >>> On Fri, Apr 14, 2017 at 8:49 PM, Dominick Grift <dac.override@gmail.c >>> om> 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. >> Hmmm...so is systemd just recursively bind mounting everything under >> /sys as read-only? If so, why does it separately list /sys/kernel/debug >> and /sys/kernel/tracing in protect_kernel_tunables_table[]? systemd/src/core/namespace.c also contains the definition of make_read_only(), which calls bind_remount_recursive(mount_entry_path(m), true, blacklist). So the RO-remount is recursive. >> >>> 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? >> I'll defer that one to Dan. > I believe that libselinux still reports that the system is running with > SELinux, if the selinuxfs is not mounted > inside of the container at all. On my system, I get: # unshare -m # sestatus SELinux status: enabled SELinuxfs mount: /sys/fs/selinux SELinux root directory: /etc/selinux Loaded policy name: refpolicy-patched Current mode: permissive Mode from config file: permissive Policy MLS status: disabled Policy deny_unknown status: denied Max kernel policy version: 30 # mount -o remount,ro /sys/fs/selinux mount: selinuxfs mounted on /sys/fs/selinux. # sestatus SELinux status: disabled # umount /sys/fs/selinux umount: /sys/fs/selinux unmounted # sestatus SELinux status: disabled The last line seems to disagree with your belief. >>> 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? >> Hmm...this is fragile. Suppose for instance that systemd were to start >> passing SELABEL_OPT_VALIDATE to selabel_open(). That would trigger >> failures because it wouldn't be able to write the context to >> /sys/fs/selinux/context to validate them. Or if it were using >> matchpathcon(), which writes the context to /sys/fs/selinux/context and >> reads back the canonicalized context for use (not sure why we stopped >> doing that in selabel_lookup; maybe that's a bug). >> >>> 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? >> With regard to the patch, Dan or others would have to assess the >> compatibility implications, since there are userspace components now >> relying on is_selinux_enabled() to return 0 if selinuxfs is read-only. >> >> With regard to use of the SELinux API, we've never guaranteed that a >> subset of the API will work if selinuxfs is not available or is read- >> only. Obviously parts of it are usable, but that seems fragile. I >> don't really think systemd ought to be remounting it read-only, but >> maybe that's just me. All right. I was not aware of /sys/fs/selinux/context and, indeed, it makes sense to only support RW selinuxfs. I have opened a Pull Request for systemd on https://github.com/systemd/systemd/pull/5741 in order to keep /sys/fs/selinux writable. Thanks, Nicolas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-15 10:23 ` Daniel Walsh 2017-04-15 14:10 ` Nicolas Iooss @ 2017-04-17 13:34 ` Stephen Smalley 2017-04-17 14:40 ` Daniel Walsh 1 sibling, 1 reply; 15+ messages in thread From: Stephen Smalley @ 2017-04-17 13:34 UTC (permalink / raw) To: dwalsh, Nicolas Iooss, selinux, Dominick Grift On Sat, 2017-04-15 at 06:23 -0400, Daniel Walsh wrote: > I believe that libselinux still reports that the system is running > with > SELinux, if the selinuxfs is not mounted > inside of the container at all. Not after the commit referenced in the subject line; you removed the fallback code to check /proc/filesystems for selinuxfs from is_selinux_enabled(), so if selinuxfs is not mounted at all, it will return 0 (not enabled). On non-Android, you can also cause is_selinux_enabled() to return 0 by not providing an /etc/selinux/config file in your container's root directory (see commit c08c4eacab8d55598b9e5caaef8a871a7a476cab), i.e. as long as you do not install selinux-policy in your container root, then it will return disabled. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-17 13:34 ` Stephen Smalley @ 2017-04-17 14:40 ` Daniel Walsh 2017-04-17 14:49 ` Stephen Smalley 0 siblings, 1 reply; 15+ messages in thread From: Daniel Walsh @ 2017-04-17 14:40 UTC (permalink / raw) To: Stephen Smalley, Nicolas Iooss, selinux, Dominick Grift On 04/17/2017 09:34 AM, Stephen Smalley wrote: > On Sat, 2017-04-15 at 06:23 -0400, Daniel Walsh wrote: >> I believe that libselinux still reports that the system is running >> with >> SELinux, if the selinuxfs is not mounted >> inside of the container at all. > Not after the commit referenced in the subject line; you removed the > fallback code to check /proc/filesystems for selinuxfs from > is_selinux_enabled(), so if selinuxfs is not mounted at all, it will > return 0 (not enabled). On non-Android, you can also cause > is_selinux_enabled() to return 0 by not providing an > /etc/selinux/config file in your container's root directory (see commit > > c08c4eacab8d55598b9e5caaef8a871a7a476cab), i.e. as long as you do not > install selinux-policy in your container root, then it will return > disabled. > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. > > That seems to a chancy way of handling this. Since I can see it as pretty easy to accidently pull in selinux-policy package into a container and then the container gets /etc/selinux/config and stuff starts blowing up. Not sure why the availability of this file should indicate selinux is enabled. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-17 14:40 ` Daniel Walsh @ 2017-04-17 14:49 ` Stephen Smalley 2017-04-17 15:08 ` Daniel Walsh 0 siblings, 1 reply; 15+ messages in thread From: Stephen Smalley @ 2017-04-17 14:49 UTC (permalink / raw) To: dwalsh, Nicolas Iooss, selinux, Dominick Grift On Mon, 2017-04-17 at 10:40 -0400, Daniel Walsh wrote: > On 04/17/2017 09:34 AM, Stephen Smalley wrote: > > On Sat, 2017-04-15 at 06:23 -0400, Daniel Walsh wrote: > > > I believe that libselinux still reports that the system is > > > running > > > with > > > SELinux, if the selinuxfs is not mounted > > > inside of the container at all. > > > > Not after the commit referenced in the subject line; you removed > > the > > fallback code to check /proc/filesystems for selinuxfs from > > is_selinux_enabled(), so if selinuxfs is not mounted at all, it > > will > > return 0 (not enabled). On non-Android, you can also cause > > is_selinux_enabled() to return 0 by not providing an > > /etc/selinux/config file in your container's root directory (see > > commit > > > > c08c4eacab8d55598b9e5caaef8a871a7a476cab), i.e. as long as you do > > not > > install selinux-policy in your container root, then it will return > > disabled. > > That seems to a chancy way of handling this. Since I can see it as > pretty easy to accidently pull in selinux-policy package into a > container and then the container gets /etc/selinux/config and stuff > starts blowing up. Not sure why the availability of this file should > indicate selinux is enabled. The existence of /etc/selinux/config is necessary but not sufficient; is_selinux_enabled() only returns 1 if selinuxfs is mounted (read-write with the current logic) _and_ (on non-Android) if /etc/selinux/config exists. The /etc/selinux/config test was added to avoid a regression when we dropped the old no-policy-loaded test. In any event, not mounting selinuxfs within the container would suffice to cause is_selinux_enabled() to return 0. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: let's revert e3cab998b48ab293a9962faf9779d70ca339c65d 2017-04-17 14:49 ` Stephen Smalley @ 2017-04-17 15:08 ` Daniel Walsh 0 siblings, 0 replies; 15+ messages in thread From: Daniel Walsh @ 2017-04-17 15:08 UTC (permalink / raw) To: Stephen Smalley, Nicolas Iooss, selinux, Dominick Grift On 04/17/2017 10:49 AM, Stephen Smalley wrote: > On Mon, 2017-04-17 at 10:40 -0400, Daniel Walsh wrote: >> On 04/17/2017 09:34 AM, Stephen Smalley wrote: >>> On Sat, 2017-04-15 at 06:23 -0400, Daniel Walsh wrote: >>>> I believe that libselinux still reports that the system is >>>> running >>>> with >>>> SELinux, if the selinuxfs is not mounted >>>> inside of the container at all. >>> Not after the commit referenced in the subject line; you removed >>> the >>> fallback code to check /proc/filesystems for selinuxfs from >>> is_selinux_enabled(), so if selinuxfs is not mounted at all, it >>> will >>> return 0 (not enabled). On non-Android, you can also cause >>> is_selinux_enabled() to return 0 by not providing an >>> /etc/selinux/config file in your container's root directory (see >>> commit >>> >>> c08c4eacab8d55598b9e5caaef8a871a7a476cab), i.e. as long as you do >>> not >>> install selinux-policy in your container root, then it will return >>> disabled. >> That seems to a chancy way of handling this. Since I can see it as >> pretty easy to accidently pull in selinux-policy package into a >> container and then the container gets /etc/selinux/config and stuff >> starts blowing up. Not sure why the availability of this file should >> indicate selinux is enabled. > The existence of /etc/selinux/config is necessary but not sufficient; > is_selinux_enabled() only returns 1 if selinuxfs is mounted (read-write > with the current logic) _and_ (on non-Android) if /etc/selinux/config > exists. The /etc/selinux/config test was added to avoid a regression > when we dropped the old no-policy-loaded test. > > In any event, not mounting selinuxfs within the container would suffice > to cause is_selinux_enabled() to return 0. > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. If that is the case, then I have no problem removing the read/only check. We can make sure /sys/fs/selinux is not mounted into the container. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-17 15:08 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-14 14:57 let's revert e3cab998b48ab293a9962faf9779d70ca339c65d Dominick Grift 2017-04-14 15:33 ` Stephen Smalley 2017-04-14 15:45 ` Dominick Grift 2017-04-14 17:47 ` Daniel Walsh 2017-04-14 17:56 ` Stephen Smalley 2017-04-14 18:07 ` Dominick Grift 2017-04-14 18:49 ` Dominick Grift 2017-04-14 19:43 ` Nicolas Iooss 2017-04-14 20:41 ` Stephen Smalley 2017-04-15 10:23 ` Daniel Walsh 2017-04-15 14:10 ` Nicolas Iooss 2017-04-17 13:34 ` Stephen Smalley 2017-04-17 14:40 ` Daniel Walsh 2017-04-17 14:49 ` Stephen Smalley 2017-04-17 15:08 ` Daniel Walsh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.