[PATCH] selinux: allow labeling before policy is loaded

[PATCH] selinux: allow labeling before policy is loaded

[Jonathan Lebon]

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.
+		 */
+		return;
+	}
+
 	rc = security_context_to_sid_force(&selinux_state, value, size,
 					   &newsid);
 	if (rc) {
-- 
2.21.0

Re: [PATCH] selinux: allow labeling before policy is loaded

[Dominick Grift]

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

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
> 

-- 
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: [PATCH] selinux: allow labeling before policy is loaded

[Dominick Grift]

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

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
> 

-- 
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: [PATCH] selinux: allow labeling before policy is loaded

[Jonathan Lebon]

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

Re: [PATCH] selinux: allow labeling before policy is loaded

[Paul Moore]

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)
      ...

    ...
  }

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: allow labeling before policy is loaded

[Jonathan Lebon]

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.

Re: [PATCH] selinux: allow labeling before policy is loaded

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com