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

[Dominick Grift <dac.override@gmail.com>]

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