| Message ID | 1493239388.1829342.957407608.3C297B30@webmail.messagingengine.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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!
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.
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.
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. > >
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.
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.
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
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