is_selinux_enabled() always returns 0 after selinux_set_policy_root()

is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Colin Walters]

I've been reworking some bits of ostree/rpm-ostree's SELinux support
recently in https://github.com/ostreedev/ostree/pull/797 - basically
using setfscreatecon() more.

However, I ran into an interesting thing I think is a bug - if others
agree I can write a patch.   Right now, as far as I can see, after
a process calls selinux_set_policy_root(), every further call to
is_selinux_enabled() will return FALSE (0).

In current git master of libselinux, we initialize via a library
constructor:
https://github.com/SELinuxProject/selinux/blob/89ce96cac6ce5eeed78cb39c58514cd68494d7aa/libselinux/src/init.c#L151

Now, selinux_set_policy_root() explicitly undoes this initialization:
https://github.com/SELinuxProject/selinux/blob/89ce96cac6ce5eeed78cb39c58514cd68494d7aa/libselinux/src/selinux_config.c#L285

Why does it do that?  It looks like this call existed since the function
was created in
https://github.com/SELinuxProject/selinux/commit/7fe6036ca5e3624d6e3a0294b909d93b145eac31

And I can't see any other place where we will reinitialize the variable;
There's no API to reinitialize `selinux_mnt`, it's only set once at library
init time.

In other words...

diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
index d8e140c..292728f 100644
--- a/libselinux/src/selinux_config.c
+++ b/libselinux/src/selinux_config.c
@@ -282,7 +282,6 @@ int selinux_set_policy_root(const char *path)
 	}
 	policy_type++;
 
-	fini_selinuxmnt();
 	fini_selinux_policyroot();
 
 	selinux_policyroot = strdup(path);

Right?

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Stephen Smalley]

On Wed, 2017-04-26 at 16:10 -0400, Colin Walters wrote:
> I've been reworking some bits of ostree/rpm-ostree's SELinux support
> recently in https://github.com/ostreedev/ostree/pull/797 - basically
> using setfscreatecon() more.
> 
> However, I ran into an interesting thing I think is a bug - if others
> agree I can write a patch.   Right now, as far as I can see, after
> a process calls selinux_set_policy_root(), every further call to
> is_selinux_enabled() will return FALSE (0).
> 
> In current git master of libselinux, we initialize via a library
> constructor:
> https://github.com/SELinuxProject/selinux/blob/89ce96cac6ce5eeed78cb3
> 9c58514cd68494d7aa/libselinux/src/init.c#L151
> 
> Now, selinux_set_policy_root() explicitly undoes this initialization:
> https://github.com/SELinuxProject/selinux/blob/89ce96cac6ce5eeed78cb3
> 9c58514cd68494d7aa/libselinux/src/selinux_config.c#L285
> 
> Why does it do that?  It looks like this call existed since the
> function
> was created in
> https://github.com/SELinuxProject/selinux/commit/7fe6036ca5e3624d6e3a
> 0294b909d93b145eac31
> 
> And I can't see any other place where we will reinitialize the
> variable;
> There's no API to reinitialize `selinux_mnt`, it's only set once at
> library
> init time.
> 
> In other words...
> 
> diff --git a/libselinux/src/selinux_config.c
> b/libselinux/src/selinux_config.c
> index d8e140c..292728f 100644
> --- a/libselinux/src/selinux_config.c
> +++ b/libselinux/src/selinux_config.c
> @@ -282,7 +282,6 @@ int selinux_set_policy_root(const char *path)
>  	}
>  	policy_type++;
>  
> -	fini_selinuxmnt();
>  	fini_selinux_policyroot();
>  
>  	selinux_policyroot = strdup(path);
> 
> Right?

Your analysis and proposed fix sound correct to me.  I blame Dan ;)

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Colin Walters]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
>
> Your analysis and proposed fix sound correct to me.  I blame Dan ;)

Thanks.  I tested the patch and confirmed it fixed ostree as it stands today,
but I'm going to change ostree to cache the result of `is_selinux_enabled()`
itself to work around this, since for our use cases it should never really
change dynamically.

Here's a git-format-patch version attached:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-config-Don-t-finalize-mount-state-in-selinux_set_pol.patch --]
[-- Type: text/x-patch; name="0001-config-Don-t-finalize-mount-state-in-selinux_set_pol.patch", Size: 1045 bytes --]

From 9268336b3e3a8994e495e7a997c9978453f7b155 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Wed, 26 Apr 2017 16:26:21 -0400
Subject: [PATCH] config: Don't finalize mount state in
 selinux_set_policy_root()

This breaks every further call to e.g. `is_selinux_enabled()` after a policy
root has been set.  This tripped up some code landed in libostree:
https://github.com/ostreedev/ostree/pull/797
Since in some cases we initialize a policy twice in process, and we'd
call `is_selinux_enabled()` each time.

More info in: http://marc.info/?l=selinux&m=149323809332417&w=2
---
 libselinux/src/selinux_config.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c
index d8e140c..292728f 100644
--- a/libselinux/src/selinux_config.c
+++ b/libselinux/src/selinux_config.c
@@ -282,7 +282,6 @@ int selinux_set_policy_root(const char *path)
 	}
 	policy_type++;
 
-	fini_selinuxmnt();
 	fini_selinux_policyroot();
 
 	selinux_policyroot = strdup(path);
-- 
2.9.3

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Colin Walters]

On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
> On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> >
> > Your analysis and proposed fix sound correct to me.  I blame Dan ;)
> 
> Thanks.  I tested the patch and confirmed it fixed ostree as it stands today,
> but I'm going to change ostree to cache the result of `is_selinux_enabled()`
> itself to work around this, since for our use cases it should never really
> change dynamically.

Although as I was working on the workaround, which I just put up as:
https://github.com/ostreedev/ostree/pull/815

I was thinking about this a bit more and I realized (maybe) why
Dan added that call.  

Right now (ignoring #ifdef ANDROID):
int is_selinux_enabled()
{
return (selinux_mnt && has_selinux_config);
}

And conceptually "has_selinux_config" derives from the policy root.
But in practice it doesn't today - that variable is also only initialized
in the constructor.   Should it?  I'm not sure.

The way libostree uses the policy root is basically for the regexp labeling
database.   We're using `is_selinux_enabled()` to determine whether
or not we should call `setfscreatecon()`. 

Eh.  My inclination is not think too much more about this.  The patch
is unlikely to break anything, it does fix a bug, and I'm not aware of a
case where someone would be using e.g. a host system with SELinux
fully disabled to do anything related to ostree, so we don't
need to care about trying to disentangle those cases.  Hopefully!

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Stephen Smalley]

On Wed, 2017-04-26 at 16:43 -0400, Colin Walters wrote:
> On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> > 
> > Your analysis and proposed fix sound correct to me.  I blame Dan ;)
> 
> Thanks.  I tested the patch and confirmed it fixed ostree as it
> stands today,
> but I'm going to change ostree to cache the result of
> `is_selinux_enabled()`
> itself to work around this, since for our use cases it should never
> really
> change dynamically.
> 
> Here's a git-format-patch version attached:

Thanks, applied.

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Stephen Smalley]

On Wed, 2017-04-26 at 17:08 -0400, Colin Walters wrote:
> 
> On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
> > On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> > > 
> > > Your analysis and proposed fix sound correct to me.  I blame Dan
> > > ;)
> > 
> > Thanks.  I tested the patch and confirmed it fixed ostree as it
> > stands today,
> > but I'm going to change ostree to cache the result of
> > `is_selinux_enabled()`
> > itself to work around this, since for our use cases it should never
> > really
> > change dynamically.
> 
> Although as I was working on the workaround, which I just put up as:
> https://github.com/ostreedev/ostree/pull/815
> 
> I was thinking about this a bit more and I realized (maybe) why
> Dan added that call.  
> 
> Right now (ignoring #ifdef ANDROID):
> int is_selinux_enabled()
> {
> return (selinux_mnt && has_selinux_config);
> }
> 
> And conceptually "has_selinux_config" derives from the policy root.
> But in practice it doesn't today - that variable is also only
> initialized
> in the constructor.   Should it?  I'm not sure.
> 
> The way libostree uses the policy root is basically for the regexp
> labeling
> database.   We're using `is_selinux_enabled()` to determine whether
> or not we should call `setfscreatecon()`. 
> 
> Eh.  My inclination is not think too much more about this.  The patch
> is unlikely to break anything, it does fix a bug, and I'm not aware
> of a
> case where someone would be using e.g. a host system with SELinux
> fully disabled to do anything related to ostree, so we don't
> need to care about trying to disentangle those cases.  Hopefully!

1. The has_selinux_config test was added long after the introduction of
 selinux_set_policy_root(), and only to avoid a regression due to
removal of the test to see if policy is loaded.  See commits
c08c4eacab8d55598b9e5caaef8a871a7a476cab and
685f4aeeadc0b60f3770404d4f149610d656e3c8.

2. The test for has_selinux_config wouldn't actually be affected by
selinux_set_policy_root() even if we were to re-evaluate it. 
selinux_set_policy_root() sets the prefix for the policy files (e.g.
from /etc/selinux/targeted to /etc/selinux/mls), it does not affect the
path for /etc/selinux/config.  We don't presently provide any way to
redirect libselinux to a different config file.

3. Originally, is_selinux_enabled() returned 1 if selinuxfs was
registered (in /proc/filesystems), even if not mounted. This was
dropped by commit e3cab998b48ab293a9962faf9779d70ca339c65d, which
changed the test to only return 1 if selinuxfs was mounted and only if
it is mounted rw, so that a ro mount could be used in mock and other
tools to make SELinux appear disabled.  Recent discussions on the list
(subject: "let's revert e3cab998b48ab293a9962faf9779d70ca339c65d") have
 proposed removing the ro check and only considering it disabled if
selinuxfs is not mounted at all because systemd is remounting
everything under /sys as ro for ProtectKernelTunables=yes and this is
leading services to conclude that SELinux is disabled and then failing
to label files correctly.  However, my impression is that systemd will
alter its behavior and I don't think we can in the near term change
libselinux in this regard as it will break various chroot and container
implementations.

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 4161 bytes --]

On Thu, Apr 27, 2017 at 09:04:11AM -0400, Stephen Smalley wrote:
> On Wed, 2017-04-26 at 17:08 -0400, Colin Walters wrote:
> > 
> > On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
> > > On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> > > > 
> > > > Your analysis and proposed fix sound correct to me.  I blame Dan
> > > > ;)
> > > 
> > > Thanks.  I tested the patch and confirmed it fixed ostree as it
> > > stands today,
> > > but I'm going to change ostree to cache the result of
> > > `is_selinux_enabled()`
> > > itself to work around this, since for our use cases it should never
> > > really
> > > change dynamically.
> > 
> > Although as I was working on the workaround, which I just put up as:
> > https://github.com/ostreedev/ostree/pull/815
> > 
> > I was thinking about this a bit more and I realized (maybe) why
> > Dan added that call.  
> > 
> > Right now (ignoring #ifdef ANDROID):
> > int is_selinux_enabled()
> > {
> > return (selinux_mnt && has_selinux_config);
> > }
> > 
> > And conceptually "has_selinux_config" derives from the policy root.
> > But in practice it doesn't today - that variable is also only
> > initialized
> > in the constructor.   Should it?  I'm not sure.
> > 
> > The way libostree uses the policy root is basically for the regexp
> > labeling
> > database.   We're using `is_selinux_enabled()` to determine whether
> > or not we should call `setfscreatecon()`. 
> > 
> > Eh.  My inclination is not think too much more about this.  The patch
> > is unlikely to break anything, it does fix a bug, and I'm not aware
> > of a
> > case where someone would be using e.g. a host system with SELinux
> > fully disabled to do anything related to ostree, so we don't
> > need to care about trying to disentangle those cases.  Hopefully!
> 
> 1. The has_selinux_config test was added long after the introduction of
>  selinux_set_policy_root(), and only to avoid a regression due to
> removal of the test to see if policy is loaded.  See commits
> c08c4eacab8d55598b9e5caaef8a871a7a476cab and
> 685f4aeeadc0b60f3770404d4f149610d656e3c8.
> 
> 2. The test for has_selinux_config wouldn't actually be affected by
> selinux_set_policy_root() even if we were to re-evaluate it. 
> selinux_set_policy_root() sets the prefix for the policy files (e.g.
> from /etc/selinux/targeted to /etc/selinux/mls), it does not affect the
> path for /etc/selinux/config.  We don't presently provide any way to
> redirect libselinux to a different config file.
> 
> 3. Originally, is_selinux_enabled() returned 1 if selinuxfs was
> registered (in /proc/filesystems), even if not mounted. This was
> dropped by commit e3cab998b48ab293a9962faf9779d70ca339c65d, which
> changed the test to only return 1 if selinuxfs was mounted and only if
> it is mounted rw, so that a ro mount could be used in mock and other
> tools to make SELinux appear disabled.  Recent discussions on the list
> (subject: "let's revert e3cab998b48ab293a9962faf9779d70ca339c65d") have
>  proposed removing the ro check and only considering it disabled if
> selinuxfs is not mounted at all because systemd is remounting
> everything under /sys as ro for ProtectKernelTunables=yes and this is
> leading services to conclude that SELinux is disabled and then failing
> to label files correctly.  However, my impression is that systemd will
> alter its behavior and I don't think we can in the near term change
> libselinux in this regard as it will break various chroot and container
> implementations.

I hope you're not being too optimistic. The way i see it is that reverting the patch will give these entities a reason to remove the mounting of selinuxfs in containers. If you keep it in for "compatibility" then i fear that it will never get fixed because there is not sense of urgency.

Also the discussion on github WRT to ProtectKernelTurnables is seemingly over but no action has been decided upon.
 
> 
> 

-- 
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 --]

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Stephen Smalley]

On Thu, 2017-04-27 at 18:53 +0200, Dominick Grift wrote:
> On Thu, Apr 27, 2017 at 09:04:11AM -0400, Stephen Smalley wrote:
> > On Wed, 2017-04-26 at 17:08 -0400, Colin Walters wrote:
> > > 
> > > On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
> > > > On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> > > > > 
> > > > > Your analysis and proposed fix sound correct to me.  I blame
> > > > > Dan
> > > > > ;)
> > > > 
> > > > Thanks.  I tested the patch and confirmed it fixed ostree as it
> > > > stands today,
> > > > but I'm going to change ostree to cache the result of
> > > > `is_selinux_enabled()`
> > > > itself to work around this, since for our use cases it should
> > > > never
> > > > really
> > > > change dynamically.
> > > 
> > > Although as I was working on the workaround, which I just put up
> > > as:
> > > https://github.com/ostreedev/ostree/pull/815
> > > 
> > > I was thinking about this a bit more and I realized (maybe) why
> > > Dan added that call.  
> > > 
> > > Right now (ignoring #ifdef ANDROID):
> > > int is_selinux_enabled()
> > > {
> > > return (selinux_mnt && has_selinux_config);
> > > }
> > > 
> > > And conceptually "has_selinux_config" derives from the policy
> > > root.
> > > But in practice it doesn't today - that variable is also only
> > > initialized
> > > in the constructor.   Should it?  I'm not sure.
> > > 
> > > The way libostree uses the policy root is basically for the
> > > regexp
> > > labeling
> > > database.   We're using `is_selinux_enabled()` to determine
> > > whether
> > > or not we should call `setfscreatecon()`. 
> > > 
> > > Eh.  My inclination is not think too much more about this.  The
> > > patch
> > > is unlikely to break anything, it does fix a bug, and I'm not
> > > aware
> > > of a
> > > case where someone would be using e.g. a host system with SELinux
> > > fully disabled to do anything related to ostree, so we don't
> > > need to care about trying to disentangle those cases.  Hopefully!
> > 
> > 1. The has_selinux_config test was added long after the
> > introduction of
> >  selinux_set_policy_root(), and only to avoid a regression due to
> > removal of the test to see if policy is loaded.  See commits
> > c08c4eacab8d55598b9e5caaef8a871a7a476cab and
> > 685f4aeeadc0b60f3770404d4f149610d656e3c8.
> > 
> > 2. The test for has_selinux_config wouldn't actually be affected by
> > selinux_set_policy_root() even if we were to re-evaluate it. 
> > selinux_set_policy_root() sets the prefix for the policy files
> > (e.g.
> > from /etc/selinux/targeted to /etc/selinux/mls), it does not affect
> > the
> > path for /etc/selinux/config.  We don't presently provide any way
> > to
> > redirect libselinux to a different config file.
> > 
> > 3. Originally, is_selinux_enabled() returned 1 if selinuxfs was
> > registered (in /proc/filesystems), even if not mounted. This was
> > dropped by commit e3cab998b48ab293a9962faf9779d70ca339c65d, which
> > changed the test to only return 1 if selinuxfs was mounted and only
> > if
> > it is mounted rw, so that a ro mount could be used in mock and
> > other
> > tools to make SELinux appear disabled.  Recent discussions on the
> > list
> > (subject: "let's revert e3cab998b48ab293a9962faf9779d70ca339c65d")
> > have
> >  proposed removing the ro check and only considering it disabled if
> > selinuxfs is not mounted at all because systemd is remounting
> > everything under /sys as ro for ProtectKernelTunables=yes and this
> > is
> > leading services to conclude that SELinux is disabled and then
> > failing
> > to label files correctly.  However, my impression is that systemd
> > will
> > alter its behavior and I don't think we can in the near term change
> > libselinux in this regard as it will break various chroot and
> > container
> > implementations.
> 
> I hope you're not being too optimistic. The way i see it is that
> reverting the patch will give these entities a reason to remove the
> mounting of selinuxfs in containers. If you keep it in for
> "compatibility" then i fear that it will never get fixed because
> there is not sense of urgency.
> 
> Also the discussion on github WRT to ProtectKernelTurnables is
> seemingly over but no action has been decided upon.

Yes, I don't know how to help move that forward.  I don't think
applying Nicolas' patch for libselinux helps in that regard; even if we
applied it today, it doesn't help motivate systemd to change its
handling of PKT or NNP.  If anything, it might cause them to delay
fixing PKT because it would "solve" the short-term problem associated
with running systemd-localed with a ro selinuxfs mount, whereas we
really need them to mount selinuxfs rw to support full use of the
SELinux API by services.

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Daniel Walsh]

On 04/27/2017 04:37 PM, Stephen Smalley wrote:
> On Thu, 2017-04-27 at 18:53 +0200, Dominick Grift wrote:
>> On Thu, Apr 27, 2017 at 09:04:11AM -0400, Stephen Smalley wrote:
>>> On Wed, 2017-04-26 at 17:08 -0400, Colin Walters wrote:
>>>> On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
>>>>> On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
>>>>>> Your analysis and proposed fix sound correct to me.  I blame
>>>>>> Dan
>>>>>> ;)
>>>>> Thanks.  I tested the patch and confirmed it fixed ostree as it
>>>>> stands today,
>>>>> but I'm going to change ostree to cache the result of
>>>>> `is_selinux_enabled()`
>>>>> itself to work around this, since for our use cases it should
>>>>> never
>>>>> really
>>>>> change dynamically.
>>>> Although as I was working on the workaround, which I just put up
>>>> as:
>>>> https://github.com/ostreedev/ostree/pull/815
>>>>
>>>> I was thinking about this a bit more and I realized (maybe) why
>>>> Dan added that call.
>>>>
>>>> Right now (ignoring #ifdef ANDROID):
>>>> int is_selinux_enabled()
>>>> {
>>>> return (selinux_mnt && has_selinux_config);
>>>> }
>>>>
>>>> And conceptually "has_selinux_config" derives from the policy
>>>> root.
>>>> But in practice it doesn't today - that variable is also only
>>>> initialized
>>>> in the constructor.   Should it?  I'm not sure.
>>>>
>>>> The way libostree uses the policy root is basically for the
>>>> regexp
>>>> labeling
>>>> database.   We're using `is_selinux_enabled()` to determine
>>>> whether
>>>> or not we should call `setfscreatecon()`.
>>>>
>>>> Eh.  My inclination is not think too much more about this.  The
>>>> patch
>>>> is unlikely to break anything, it does fix a bug, and I'm not
>>>> aware
>>>> of a
>>>> case where someone would be using e.g. a host system with SELinux
>>>> fully disabled to do anything related to ostree, so we don't
>>>> need to care about trying to disentangle those cases.  Hopefully!
>>> 1. The has_selinux_config test was added long after the
>>> introduction of
>>>   selinux_set_policy_root(), and only to avoid a regression due to
>>> removal of the test to see if policy is loaded.  See commits
>>> c08c4eacab8d55598b9e5caaef8a871a7a476cab and
>>> 685f4aeeadc0b60f3770404d4f149610d656e3c8.
>>>
>>> 2. The test for has_selinux_config wouldn't actually be affected by
>>> selinux_set_policy_root() even if we were to re-evaluate it.
>>> selinux_set_policy_root() sets the prefix for the policy files
>>> (e.g.
>>> from /etc/selinux/targeted to /etc/selinux/mls), it does not affect
>>> the
>>> path for /etc/selinux/config.  We don't presently provide any way
>>> to
>>> redirect libselinux to a different config file.
>>>
>>> 3. Originally, is_selinux_enabled() returned 1 if selinuxfs was
>>> registered (in /proc/filesystems), even if not mounted. This was
>>> dropped by commit e3cab998b48ab293a9962faf9779d70ca339c65d, which
>>> changed the test to only return 1 if selinuxfs was mounted and only
>>> if
>>> it is mounted rw, so that a ro mount could be used in mock and
>>> other
>>> tools to make SELinux appear disabled.  Recent discussions on the
>>> list
>>> (subject: "let's revert e3cab998b48ab293a9962faf9779d70ca339c65d")
>>> have
>>>   proposed removing the ro check and only considering it disabled if
>>> selinuxfs is not mounted at all because systemd is remounting
>>> everything under /sys as ro for ProtectKernelTunables=yes and this
>>> is
>>> leading services to conclude that SELinux is disabled and then
>>> failing
>>> to label files correctly.  However, my impression is that systemd
>>> will
>>> alter its behavior and I don't think we can in the near term change
>>> libselinux in this regard as it will break various chroot and
>>> container
>>> implementations.
>> I hope you're not being too optimistic. The way i see it is that
>> reverting the patch will give these entities a reason to remove the
>> mounting of selinuxfs in containers. If you keep it in for
>> "compatibility" then i fear that it will never get fixed because
>> there is not sense of urgency.
>>
>> Also the discussion on github WRT to ProtectKernelTurnables is
>> seemingly over but no action has been decided upon.
> Yes, I don't know how to help move that forward.  I don't think
> applying Nicolas' patch for libselinux helps in that regard; even if we
> applied it today, it doesn't help motivate systemd to change its
> handling of PKT or NNP.  If anything, it might cause them to delay
> fixing PKT because it would "solve" the short-term problem associated
> with running systemd-localed with a ro selinuxfs mount, whereas we
> really need them to mount selinuxfs rw to support full use of the
> SELinux API by services.
>
>
I thought we agreed to remove the R/O check from libselinux.  I am fine 
as long as libselinux reports SELinux is disabled when selinuxfs is not 
mounted in the container.  We can move to ensure that selinuxfs is not 
mounted by container runtimes.  Currently I am not aware of a container 
runtime that mounts selinuxfs read/only to take advantage of this 
feature.  selinuxfs is not mounted by default in any container runtimes 
that I know of. Removing the libselinux READ/ONLY restriction should not 
break these work loads.

Re: is_selinux_enabled() always returns 0 after selinux_set_policy_root()

[Dominick Grift]

[-- Attachment #1: Type: text/plain, Size: 6466 bytes --]

On Sun, Apr 30, 2017 at 06:51:17AM -0400, Daniel Walsh wrote:
> On 04/27/2017 04:37 PM, Stephen Smalley wrote:
> > On Thu, 2017-04-27 at 18:53 +0200, Dominick Grift wrote:
> > > On Thu, Apr 27, 2017 at 09:04:11AM -0400, Stephen Smalley wrote:
> > > > On Wed, 2017-04-26 at 17:08 -0400, Colin Walters wrote:
> > > > > On Wed, Apr 26, 2017, at 04:43 PM, Colin Walters wrote:
> > > > > > On Wed, Apr 26, 2017, at 04:24 PM, Stephen Smalley wrote:
> > > > > > > Your analysis and proposed fix sound correct to me.  I blame
> > > > > > > Dan
> > > > > > > ;)
> > > > > > Thanks.  I tested the patch and confirmed it fixed ostree as it
> > > > > > stands today,
> > > > > > but I'm going to change ostree to cache the result of
> > > > > > `is_selinux_enabled()`
> > > > > > itself to work around this, since for our use cases it should
> > > > > > never
> > > > > > really
> > > > > > change dynamically.
> > > > > Although as I was working on the workaround, which I just put up
> > > > > as:
> > > > > https://github.com/ostreedev/ostree/pull/815
> > > > > 
> > > > > I was thinking about this a bit more and I realized (maybe) why
> > > > > Dan added that call.
> > > > > 
> > > > > Right now (ignoring #ifdef ANDROID):
> > > > > int is_selinux_enabled()
> > > > > {
> > > > > return (selinux_mnt && has_selinux_config);
> > > > > }
> > > > > 
> > > > > And conceptually "has_selinux_config" derives from the policy
> > > > > root.
> > > > > But in practice it doesn't today - that variable is also only
> > > > > initialized
> > > > > in the constructor.   Should it?  I'm not sure.
> > > > > 
> > > > > The way libostree uses the policy root is basically for the
> > > > > regexp
> > > > > labeling
> > > > > database.   We're using `is_selinux_enabled()` to determine
> > > > > whether
> > > > > or not we should call `setfscreatecon()`.
> > > > > 
> > > > > Eh.  My inclination is not think too much more about this.  The
> > > > > patch
> > > > > is unlikely to break anything, it does fix a bug, and I'm not
> > > > > aware
> > > > > of a
> > > > > case where someone would be using e.g. a host system with SELinux
> > > > > fully disabled to do anything related to ostree, so we don't
> > > > > need to care about trying to disentangle those cases.  Hopefully!
> > > > 1. The has_selinux_config test was added long after the
> > > > introduction of
> > > >   selinux_set_policy_root(), and only to avoid a regression due to
> > > > removal of the test to see if policy is loaded.  See commits
> > > > c08c4eacab8d55598b9e5caaef8a871a7a476cab and
> > > > 685f4aeeadc0b60f3770404d4f149610d656e3c8.
> > > > 
> > > > 2. The test for has_selinux_config wouldn't actually be affected by
> > > > selinux_set_policy_root() even if we were to re-evaluate it.
> > > > selinux_set_policy_root() sets the prefix for the policy files
> > > > (e.g.
> > > > from /etc/selinux/targeted to /etc/selinux/mls), it does not affect
> > > > the
> > > > path for /etc/selinux/config.  We don't presently provide any way
> > > > to
> > > > redirect libselinux to a different config file.
> > > > 
> > > > 3. Originally, is_selinux_enabled() returned 1 if selinuxfs was
> > > > registered (in /proc/filesystems), even if not mounted. This was
> > > > dropped by commit e3cab998b48ab293a9962faf9779d70ca339c65d, which
> > > > changed the test to only return 1 if selinuxfs was mounted and only
> > > > if
> > > > it is mounted rw, so that a ro mount could be used in mock and
> > > > other
> > > > tools to make SELinux appear disabled.  Recent discussions on the
> > > > list
> > > > (subject: "let's revert e3cab998b48ab293a9962faf9779d70ca339c65d")
> > > > have
> > > >   proposed removing the ro check and only considering it disabled if
> > > > selinuxfs is not mounted at all because systemd is remounting
> > > > everything under /sys as ro for ProtectKernelTunables=yes and this
> > > > is
> > > > leading services to conclude that SELinux is disabled and then
> > > > failing
> > > > to label files correctly.  However, my impression is that systemd
> > > > will
> > > > alter its behavior and I don't think we can in the near term change
> > > > libselinux in this regard as it will break various chroot and
> > > > container
> > > > implementations.
> > > I hope you're not being too optimistic. The way i see it is that
> > > reverting the patch will give these entities a reason to remove the
> > > mounting of selinuxfs in containers. If you keep it in for
> > > "compatibility" then i fear that it will never get fixed because
> > > there is not sense of urgency.
> > > 
> > > Also the discussion on github WRT to ProtectKernelTurnables is
> > > seemingly over but no action has been decided upon.
> > Yes, I don't know how to help move that forward.  I don't think
> > applying Nicolas' patch for libselinux helps in that regard; even if we
> > applied it today, it doesn't help motivate systemd to change its
> > handling of PKT or NNP.  If anything, it might cause them to delay
> > fixing PKT because it would "solve" the short-term problem associated
> > with running systemd-localed with a ro selinuxfs mount, whereas we
> > really need them to mount selinuxfs rw to support full use of the
> > SELinux API by services.
> > 
> > 
> I thought we agreed to remove the R/O check from libselinux.  I am fine as
> long as libselinux reports SELinux is disabled when selinuxfs is not mounted
> in the container.  We can move to ensure that selinuxfs is not mounted by
> container runtimes.  Currently I am not aware of a container runtime that
> mounts selinuxfs read/only to take advantage of this feature.  selinuxfs is
> not mounted by default in any container runtimes that I know of. Removing
> the libselinux READ/ONLY restriction should not break these work loads.
> 

I might not be reading this code the right way but:

https://github.com/systemd/systemd/blob/master/src/nspawn/nspawn-mount.c#L563

looks like its bind mounting selinuxfs?

Anyhow, Could you please have a look at the below issue and give your perspective on whether NNP should be treated as mutually exclusive with SELinux in systemd?

https://github.com/systemd/systemd/pull/5741#issuecomment-294931845

-- 
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 --]