All of lore.kernel.org
 help / color / mirror / Atom feed
* 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.