| Message ID | 20190819193032.848-1-jlebon@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: allow labeling before policy is loaded | expand |
On Mon, Aug 19, 2019 at 03:30:32PM -0400, Jonathan Lebon wrote: > Currently, the SELinux LSM prevents one from setting the > `security.selinux` xattr on an inode without a policy first being > loaded. However, this restriction is problematic: it makes it impossible > to have newly created files with the correct label before actually > loading the policy. > > This is relevant in distributions like Fedora, where the policy is > loaded by systemd shortly after pivoting out of the initrd. In such > instances, all files created prior to pivoting will be unlabeled. One > then has to relabel them after pivoting, an operation which inherently > races with other processes trying to access those same files. > > Going further, there are use cases for creating the entire root > filesystem on first boot from the initrd (e.g. Container Linux supports > this today[1], and we'd like to support it in Fedora CoreOS as well[2]). > One can imagine doing this in two ways: at the block device level (e.g. > laying down a disk image), or at the filesystem level. In the former, > labeling can simply be part of the image. But even in the latter > scenario, one still really wants to be able to set the right labels when > populating the new filesystem. Does `echo "/" > /run/systemd/relabel-extra.d/foo.relabel` not address this? https://github.com/fedora-selinux/selinux-policy/issues/270 If that does not do what it should do then this functionality should probably be removed? > > This patch enables this by changing behaviour in the following two ways: > 1. allow `setxattr` on mounts without `SBLABEL_MNT` (which is all of > them if no policy is loaded yet) > 2. don't try to set the in-core inode SID if we're not initialized; > instead leave it as `LABEL_INVALID` so that revalidation may be > attempted at a later time > > Note the first hunk of this patch is functionally the same as a > previously discussed one[3], though it was part of a larger series which > wasn't accepted. > > Co-developed-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Jonathan Lebon <jlebon@redhat.com> > > [1] https://coreos.com/os/docs/latest/root-filesystem-placement.html > [2] https://github.com/coreos/fedora-coreos-tracker/issues/94 > [3] https://www.spinics.net/lists/linux-initramfs/msg04593.html > --- > security/selinux/hooks.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 94de51628..faf93e9f8 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3143,7 +3143,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, > } > > sbsec = inode->i_sb->s_security; > - if (!(sbsec->flags & SBLABEL_MNT)) > + if (!(sbsec->flags & SBLABEL_MNT) && selinux_state.initialized) > return -EOPNOTSUPP; > > if (!inode_owner_or_capable(inode)) > @@ -3225,6 +3225,15 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, > return; > } > > + if (!selinux_state.initialized) { > + /* If we haven't even been initialized, then we can't validate > + * against a policy, so leave the label as invalid. It may > + * resolve to a valid label on the next revalidation try if > + * we've since initialized. > + */ > + return; > + } > + > rc = security_context_to_sid_force(&selinux_state, value, size, > &newsid); > if (rc) { > -- > 2.21.0 >
On Mon, Aug 19, 2019 at 03:30:32PM -0400, Jonathan Lebon wrote: > Currently, the SELinux LSM prevents one from setting the > `security.selinux` xattr on an inode without a policy first being > loaded. However, this restriction is problematic: it makes it impossible > to have newly created files with the correct label before actually > loading the policy. > > This is relevant in distributions like Fedora, where the policy is > loaded by systemd shortly after pivoting out of the initrd. In such > instances, all files created prior to pivoting will be unlabeled. One > then has to relabel them after pivoting, an operation which inherently > races with other processes trying to access those same files. > > Going further, there are use cases for creating the entire root > filesystem on first boot from the initrd (e.g. Container Linux supports > this today[1], and we'd like to support it in Fedora CoreOS as well[2]). > One can imagine doing this in two ways: at the block device level (e.g. > laying down a disk image), or at the filesystem level. In the former, > labeling can simply be part of the image. But even in the latter > scenario, one still really wants to be able to set the right labels when > populating the new filesystem. > > This patch enables this by changing behaviour in the following two ways: > 1. allow `setxattr` on mounts without `SBLABEL_MNT` (which is all of > them if no policy is loaded yet) > 2. don't try to set the in-core inode SID if we're not initialized; > instead leave it as `LABEL_INVALID` so that revalidation may be > attempted at a later time > > Note the first hunk of this patch is functionally the same as a > previously discussed one[3], though it was part of a larger series which > wasn't accepted. > > Co-developed-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Jonathan Lebon <jlebon@redhat.com> > > [1] https://coreos.com/os/docs/latest/root-filesystem-placement.html > [2] https://github.com/coreos/fedora-coreos-tracker/issues/94 > [3] https://www.spinics.net/lists/linux-initramfs/msg04593.html > --- > security/selinux/hooks.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 94de51628..faf93e9f8 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3143,7 +3143,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, > } > > sbsec = inode->i_sb->s_security; > - if (!(sbsec->flags & SBLABEL_MNT)) > + if (!(sbsec->flags & SBLABEL_MNT) && selinux_state.initialized) > return -EOPNOTSUPP; > > if (!inode_owner_or_capable(inode)) > @@ -3225,6 +3225,15 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, > return; > } > > + if (!selinux_state.initialized) { > + /* If we haven't even been initialized, then we can't validate > + * against a policy, so leave the label as invalid. It may > + * resolve to a valid label on the next revalidation try if > + * we've since initialized. > + */ If you cannot validate against a policy, then how do you know what labels to associate? > + return; > + } > + > rc = security_context_to_sid_force(&selinux_state, value, size, > &newsid); > if (rc) { > -- > 2.21.0 >
On Mon, Aug 19, 2019 at 3:49 PM Dominick Grift <dac.override@gmail.com> wrote: > > > Going further, there are use cases for creating the entire root > > filesystem on first boot from the initrd (e.g. Container Linux supports > > this today[1], and we'd like to support it in Fedora CoreOS as well[2]). > > One can imagine doing this in two ways: at the block device level (e.g. > > laying down a disk image), or at the filesystem level. In the former, > > labeling can simply be part of the image. But even in the latter > > scenario, one still really wants to be able to set the right labels when > > populating the new filesystem. > > Does `echo "/" > /run/systemd/relabel-extra.d/foo.relabel` not address this? > > https://github.com/fedora-selinux/selinux-policy/issues/270 If one has files on multiple partitions, using systemd for this would require keeping all the filesystems mounted across the pivot, something we're trying very hard to avoid because it makes the bootup process (and potentially mount hierarchy) different between the first and subsequent boots. See more discussions about this in: https://github.com/systemd/systemd/pull/11903 But also, asking systemd to relabel the whole system on boot isn't... ideal. In the filesystem provisioning case I mentioned, we would be extracting files from an OSTree repo, which already has labeling information. > > If that does not do what it should do then this functionality should probably be removed? I think the functionality on its own can be useful for a subset of use cases. E.g. dracut modules which need to write some files outside of /run (since that one already gets relabeled automatically)? On Mon, Aug 19, 2019 at 4:05 PM Dominick Grift <dac.override@gmail.com> wrote: > > > + if (!selinux_state.initialized) { > > + /* If we haven't even been initialized, then we can't validate > > + * against a policy, so leave the label as invalid. It may > > + * resolve to a valid label on the next revalidation try if > > + * we've since initialized. > > + */ > > If you cannot validate against a policy, then how do you know what labels to associate? If only relabeling a few files, we can use the libselinux APIs (`selabel_*`), fetching the file context policy from /sysroot. In the "full filesystem reprovision" scenario, labeling information is already part of the objects we're writing. One might then ask, "why not just load the policy in the initrd instead?". The answer is that this also comes with a host of issues, and isn't very widely used in practice. See discussions about that in: https://github.com/coreos/ignition/issues/635 Esp. this comment and onwards: https://github.com/coreos/ignition/issues/635#issuecomment-497730774
On Mon, Aug 19, 2019 at 3:31 PM Jonathan Lebon <jlebon@redhat.com> wrote: > > Currently, the SELinux LSM prevents one from setting the > `security.selinux` xattr on an inode without a policy first being > loaded. However, this restriction is problematic: it makes it impossible > to have newly created files with the correct label before actually > loading the policy. > > This is relevant in distributions like Fedora, where the policy is > loaded by systemd shortly after pivoting out of the initrd. In such > instances, all files created prior to pivoting will be unlabeled. One > then has to relabel them after pivoting, an operation which inherently > races with other processes trying to access those same files. > > Going further, there are use cases for creating the entire root > filesystem on first boot from the initrd (e.g. Container Linux supports > this today[1], and we'd like to support it in Fedora CoreOS as well[2]). > One can imagine doing this in two ways: at the block device level (e.g. > laying down a disk image), or at the filesystem level. In the former, > labeling can simply be part of the image. But even in the latter > scenario, one still really wants to be able to set the right labels when > populating the new filesystem. > > This patch enables this by changing behaviour in the following two ways: > 1. allow `setxattr` on mounts without `SBLABEL_MNT` (which is all of > them if no policy is loaded yet) > 2. don't try to set the in-core inode SID if we're not initialized; > instead leave it as `LABEL_INVALID` so that revalidation may be > attempted at a later time > > Note the first hunk of this patch is functionally the same as a > previously discussed one[3], though it was part of a larger series which > wasn't accepted. > > Co-developed-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Victor Kamensky <kamensky@cisco.com> > Signed-off-by: Jonathan Lebon <jlebon@redhat.com> > > [1] https://coreos.com/os/docs/latest/root-filesystem-placement.html > [2] https://github.com/coreos/fedora-coreos-tracker/issues/94 > [3] https://www.spinics.net/lists/linux-initramfs/msg04593.html > --- > security/selinux/hooks.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) ... > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 94de51628..faf93e9f8 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3143,7 +3143,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, > } > > sbsec = inode->i_sb->s_security; > - if (!(sbsec->flags & SBLABEL_MNT)) > + if (!(sbsec->flags & SBLABEL_MNT) && selinux_state.initialized) > return -EOPNOTSUPP; As I'm looking at this, I'm wondering why we don't just bail out early if the policy isn't loaded? The context lookup and permission checks later in the function are pretty much useless if the policy hasn't been loaded (they are just going to return defaults/allow), I think the only thing we would need to check would be inode_owner_or_capable(). int selinux_inode_setxattr(...) { if (strcmp(name, XATTR_NAME_SELINUX)) { ... } if (!selinux_state.initialized) return (inode_owner_or_capable(inode) ? 0 : -EPERM); if (sbsec & SBLABEL_MNT) ... if (!inode_owner_or_capable(inode) ... ... }
On Tue, Aug 27, 2019 at 8:56 PM Paul Moore <paul@paul-moore.com> wrote: > As I'm looking at this, I'm wondering why we don't just bail out early > if the policy isn't loaded? The context lookup and permission checks > later in the function are pretty much useless if the policy hasn't > been loaded (they are just going to return defaults/allow), I think > the only thing we would need to check would be > inode_owner_or_capable(). Yes, I think you're correct. Though in that case, would it make sense to just do the inode_owner_or_capable() check once upfront instead? int selinux_inode_setxattr(...) { if (strcmp(name, XATTR_NAME_SELINUX)) { ... } if (!inode_owner_or_capable(inode) ... if (!selinux_state.initialized) return 0; if (sbsec & SBLABEL_MNT) ... ... } Hmm, though I guess it does change the behaviour slightly even in the initialized case by returning EPERM first where before we might've returned EOPNOTSUPP (I've seen userspace code which subtly relied on the order in which the kernel checks for error conditions). I'm happy to be conservative and go with your approach if you prefer.
On Wed, Sep 11, 2019 at 5:28 PM Jonathan Lebon <jlebon@redhat.com> wrote: > On Tue, Aug 27, 2019 at 8:56 PM Paul Moore <paul@paul-moore.com> wrote: > > As I'm looking at this, I'm wondering why we don't just bail out early > > if the policy isn't loaded? The context lookup and permission checks > > later in the function are pretty much useless if the policy hasn't > > been loaded (they are just going to return defaults/allow), I think > > the only thing we would need to check would be > > inode_owner_or_capable(). > > Yes, I think you're correct. Though in that case, would it make sense > to just do the inode_owner_or_capable() check once upfront instead? > > int selinux_inode_setxattr(...) > { > > if (strcmp(name, XATTR_NAME_SELINUX)) { > ... > } > > if (!inode_owner_or_capable(inode) > ... > > if (!selinux_state.initialized) > return 0; > > if (sbsec & SBLABEL_MNT) > ... > > ... > } > > Hmm, though I guess it does change the behaviour slightly even in the > initialized case by returning EPERM first where before we might've > returned EOPNOTSUPP (I've seen userspace code which subtly relied on > the order in which the kernel checks for error conditions). I'm happy > to be conservative and go with your approach if you prefer. Exactly. I suggested the approach I did because I was trying to avoid changing the return behaviour; unless you can prove beyond a shadow of a doubt that changing the return values doesn't break anything (that's a pretty high bar), stick with the conservative approach.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 94de51628..faf93e9f8 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3143,7 +3143,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, } sbsec = inode->i_sb->s_security; - if (!(sbsec->flags & SBLABEL_MNT)) + if (!(sbsec->flags & SBLABEL_MNT) && selinux_state.initialized) return -EOPNOTSUPP; if (!inode_owner_or_capable(inode)) @@ -3225,6 +3225,15 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, return; } + if (!selinux_state.initialized) { + /* If we haven't even been initialized, then we can't validate + * against a policy, so leave the label as invalid. It may + * resolve to a valid label on the next revalidation try if + * we've since initialized. + */ + return; + } + rc = security_context_to_sid_force(&selinux_state, value, size, &newsid); if (rc) {