[PATCH] selinux: allow reading labels before policy is loaded

[PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

This patch does for `getxattr` what 3e3e24b4204 did for `setxattr`: it
allows querying the current SELinux label on disk before the policy is
loaded.

One of the motivations described in that commit message also drives this
patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be
able to move the root filesystem for example from xfs to ext4, on first
boot, at initrd time.[1]

Because such an operation works at the filesystem level, we need to be
able to read the SELinux labels first from the original root, and apply
them to the files of the new root. Commit 3e3e24b4204 enabled the second
part of this process; this patch enables the first part.

[1] https://github.com/coreos/fedora-coreos-tracker/issues/94

Signed-off-by: Jonathan Lebon <jlebon@redhat.com>
---
 security/selinux/hooks.c | 55 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b7..3bbb9966697 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1317,8 +1317,11 @@ static int selinux_genfs_get_sid(struct dentry *dentry,
 	return rc;
 }
 
-static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
-				  u32 def_sid, u32 *sid)
+/* Retrieves the raw context from the fs xattr. Returns 0 on success. */
+static int get_inode_raw_xattr_context(struct inode *inode,
+				       struct dentry *dentry,
+				       char **out_context,
+				       u32 *out_len)
 {
 #define INITCONTEXTLEN 255
 	char *context;
@@ -1354,13 +1357,31 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
 		if (rc != -ENODATA) {
 			pr_warn("SELinux: %s:  getxattr returned %d for dev=%s ino=%ld\n",
 				__func__, -rc, inode->i_sb->s_id, inode->i_ino);
-			return rc;
 		}
+		return rc;
+	}
+
+	*out_len = rc;
+	*out_context = context;
+	return 0;
+}
+
+static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
+				  u32 def_sid, u32 *sid)
+{
+	char *context;
+	u32 size;
+	int rc;
+
+	rc = get_inode_raw_xattr_context(inode, dentry, &context, &size);
+	if (rc < 0) {
+		if (rc != -ENODATA)
+			return rc;
 		*sid = def_sid;
 		return 0;
 	}
 
-	rc = security_context_to_sid_default(&selinux_state, context, rc, sid,
+	rc = security_context_to_sid_default(&selinux_state, context, size, sid,
 					     def_sid, GFP_NOFS);
 	if (rc) {
 		char *dev = inode->i_sb->s_id;
@@ -3333,10 +3354,34 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
 	int error;
 	char *context = NULL;
 	struct inode_security_struct *isec;
+	struct superblock_security_struct *sbsec;
 
 	if (strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
 
+	isec = inode_security(inode);
+	sbsec = inode->i_sb->s_security;
+
+	/* Just return the raw context if the policy isn't even loaded since we
+	 * have no way to validate it anyway. This is symmetrical with allowing
+	 * setxattr without a policy. */
+	if (!selinux_state.initialized) {
+		/* See similar code in inode_doinit_with_dentry; for xattrs,
+		 * some filesystems really want a connected inode. If we don't
+		 * find one, just let fallback in case it corresponds to one of
+		 * the default sids. */
+		struct dentry *dentry = d_find_alias(inode);
+		if (!dentry)
+			dentry = d_find_any_alias(inode);
+
+		if (dentry) {
+			error = get_inode_raw_xattr_context(inode, dentry,
+							    &context, &size);
+			dput(dentry);
+			goto out;
+		}
+	}
+
 	/*
 	 * If the caller has CAP_MAC_ADMIN, then get the raw context
 	 * value even if it is not defined by current policy; otherwise,
@@ -3346,7 +3391,6 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
 	 * and lack of permission just means that we fall back to the
 	 * in-core context value, not a denial.
 	 */
-	isec = inode_security(inode);
 	if (has_cap_mac_admin(false))
 		error = security_sid_to_context_force(&selinux_state,
 						      isec->sid, &context,
@@ -3354,6 +3398,7 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
 	else
 		error = security_sid_to_context(&selinux_state, isec->sid,
 						&context, &size);
+out:
 	if (error)
 		return error;
 	error = size;
-- 
2.25.4

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Ondrej Mosnacek]

On Sat, May 23, 2020 at 9:56 PM Jonathan Lebon <jlebon@redhat.com> wrote:
> This patch does for `getxattr` what 3e3e24b4204 did for `setxattr`: it
> allows querying the current SELinux label on disk before the policy is
> loaded.
>
> One of the motivations described in that commit message also drives this
> patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be
> able to move the root filesystem for example from xfs to ext4, on first
> boot, at initrd time.[1]
>
> Because such an operation works at the filesystem level, we need to be
> able to read the SELinux labels first from the original root, and apply
> them to the files of the new root. Commit 3e3e24b4204 enabled the second
> part of this process; this patch enables the first part.
>
> [1] https://github.com/coreos/fedora-coreos-tracker/issues/94
>
> Signed-off-by: Jonathan Lebon <jlebon@redhat.com>
> ---
>  security/selinux/hooks.c | 55 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b7..3bbb9966697 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
<snip>
> @@ -3333,10 +3354,34 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
>         int error;
>         char *context = NULL;
>         struct inode_security_struct *isec;
> +       struct superblock_security_struct *sbsec;
>
>         if (strcmp(name, XATTR_SELINUX_SUFFIX))
>                 return -EOPNOTSUPP;

I might be missing something, but couldn't you achieve the same by
simply adding something like this in here:

if (!selinux_initialized(&selinux_state))
        return -EOPNOTSUPP;

(Or by adding it to the condition above.)

Then you should hit this condition here and be all set:
https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337

>
> +       isec = inode_security(inode);
> +       sbsec = inode->i_sb->s_security;
> +
> +       /* Just return the raw context if the policy isn't even loaded since we
> +        * have no way to validate it anyway. This is symmetrical with allowing
> +        * setxattr without a policy. */
> +       if (!selinux_state.initialized) {
> +               /* See similar code in inode_doinit_with_dentry; for xattrs,
> +                * some filesystems really want a connected inode. If we don't
> +                * find one, just let fallback in case it corresponds to one of
> +                * the default sids. */
> +               struct dentry *dentry = d_find_alias(inode);
> +               if (!dentry)
> +                       dentry = d_find_any_alias(inode);
> +
> +               if (dentry) {
> +                       error = get_inode_raw_xattr_context(inode, dentry,
> +                                                           &context, &size);
> +                       dput(dentry);
> +                       goto out;
> +               }
> +       }
> +
>         /*
>          * If the caller has CAP_MAC_ADMIN, then get the raw context
>          * value even if it is not defined by current policy; otherwise,
> @@ -3346,7 +3391,6 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
>          * and lack of permission just means that we fall back to the
>          * in-core context value, not a denial.
>          */
> -       isec = inode_security(inode);
>         if (has_cap_mac_admin(false))
>                 error = security_sid_to_context_force(&selinux_state,
>                                                       isec->sid, &context,
> @@ -3354,6 +3398,7 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
>         else
>                 error = security_sid_to_context(&selinux_state, isec->sid,
>                                                 &context, &size);
> +out:
>         if (error)
>                 return error;
>         error = size;
> --
> 2.25.4
>

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel,
Red Hat, Inc.

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I might be missing something, but couldn't you achieve the same by
> simply adding something like this in here:
>
> if (!selinux_initialized(&selinux_state))
>         return -EOPNOTSUPP;
>
> (Or by adding it to the condition above.)
>
> Then you should hit this condition here and be all set:
> https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337

Hi Ondrej,

Yes, that looks promising. Two questions with that approach:

1. Is there a concern here with transiently returning -EOPNOTSUPP even
if the SELinux LSM does technically support the inode_getsecurity
hook? I'm thinking of potential corner-cases down the road where
somehow this knowledge is cached.

2. The selinux_inode_getsecurity hook today does somewhat handle the
uninitialized case. It ends up here:

https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322.

Specifically, it has support for initial SIDs. The patch I wrote
purposely tries to allow falling back to that logic. Is there a
concern with short-circuiting this logic even if the inode SID somehow
isn't SECINITSID_UNLABELED?

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Ondrej Mosnacek]

On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@redhat.com> wrote:
> On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > I might be missing something, but couldn't you achieve the same by
> > simply adding something like this in here:
> >
> > if (!selinux_initialized(&selinux_state))
> >         return -EOPNOTSUPP;
> >
> > (Or by adding it to the condition above.)
> >
> > Then you should hit this condition here and be all set:
> > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337
>
> Hi Ondrej,
>
> Yes, that looks promising. Two questions with that approach:
>
> 1. Is there a concern here with transiently returning -EOPNOTSUPP even
> if the SELinux LSM does technically support the inode_getsecurity
> hook? I'm thinking of potential corner-cases down the road where
> somehow this knowledge is cached.

I would hope not. I don't think it's likely this would be cached,
since it would require a guarantee from all LSMs that they won't flip
from -EOPNOTSUPP to something else... That would be error-prone IMHO.

>
> 2. The selinux_inode_getsecurity hook today does somewhat handle the
> uninitialized case. It ends up here:
>
> https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322.
>
> Specifically, it has support for initial SIDs. The patch I wrote
> purposely tries to allow falling back to that logic. Is there a
> concern with short-circuiting this logic even if the inode SID somehow
> isn't SECINITSID_UNLABELED?

Oh, right, so that's what I missed :) I'll have to defer to Stephen on
whether this is a concern... Obviously we lose the previous behavior
of returning the initial SID strings via getxattr(), but I'm not sure
if that's significant. Since those strings obviously aren't real
contexts, it seems they only serve an informational purpose.

Anyway, I looked at the original patch again and it generally looks
sane. I don't like the fact that we need to call back to
__vfs_getxattr() in yet another place, but it makes sense since
security_inode_getsecurity() is basically overriding it. So I leave it
on Stephen or Paul to decide which is better.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel,
Red Hat, Inc.

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Stephen Smalley]

On Wed, May 27, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@redhat.com> wrote:
> > On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > I might be missing something, but couldn't you achieve the same by
> > > simply adding something like this in here:
> > >
> > > if (!selinux_initialized(&selinux_state))
> > >         return -EOPNOTSUPP;
> > >
> > > (Or by adding it to the condition above.)
> > >
> > > Then you should hit this condition here and be all set:
> > > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337
> >
> > Hi Ondrej,
> >
> > Yes, that looks promising. Two questions with that approach:
> >
> > 1. Is there a concern here with transiently returning -EOPNOTSUPP even
> > if the SELinux LSM does technically support the inode_getsecurity
> > hook? I'm thinking of potential corner-cases down the road where
> > somehow this knowledge is cached.
>
> I would hope not. I don't think it's likely this would be cached,
> since it would require a guarantee from all LSMs that they won't flip
> from -EOPNOTSUPP to something else... That would be error-prone IMHO.
>
> >
> > 2. The selinux_inode_getsecurity hook today does somewhat handle the
> > uninitialized case. It ends up here:
> >
> > https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322.
> >
> > Specifically, it has support for initial SIDs. The patch I wrote
> > purposely tries to allow falling back to that logic. Is there a
> > concern with short-circuiting this logic even if the inode SID somehow
> > isn't SECINITSID_UNLABELED?
>
> Oh, right, so that's what I missed :) I'll have to defer to Stephen on
> whether this is a concern... Obviously we lose the previous behavior
> of returning the initial SID strings via getxattr(), but I'm not sure
> if that's significant. Since those strings obviously aren't real
> contexts, it seems they only serve an informational purpose.
>
> Anyway, I looked at the original patch again and it generally looks
> sane. I don't like the fact that we need to call back to
> __vfs_getxattr() in yet another place, but it makes sense since
> security_inode_getsecurity() is basically overriding it. So I leave it
> on Stephen or Paul to decide which is better.

I think Ondrej's suggested approach is better.  I don't think it is a concern.

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I might be missing something, but couldn't you achieve the same by
> simply adding something like this in here:
>
> if (!selinux_initialized(&selinux_state))
>         return -EOPNOTSUPP;
>
> (Or by adding it to the condition above.)
>
> Then you should hit this condition here and be all set:
> https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337

OK, I tried this now and it does work well at least for our use case.
It's clearly a much simpler patch as well, which is nice. So assuming
my questions from the previous email don't raise any concerns, I'm
happy to switch to that instead.

One thing worth mentioning is that both patches change the behaviour
of `getxattr` for inodes on SE_SBGENFS superblocks. For example,
before, `getxattr("/proc", "security.selinux")` would return
`unlabeled_t`. Now it gets EOPNOTSUPP instead.

Anyway, I think this is fine overall and consistent with the behaviour
change proposed, but just wanted to flag it.

[PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

This patch does for `getxattr` what commit 3e3e24b42043 ("selinux: allow
labeling before policy is loaded") did for `setxattr`; it allows
querying the current SELinux label on disk before the policy is loaded.

One of the motivations described in that commit message also drives this
patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be
able to move the root filesystem for example, from xfs to ext4 on RAID,
on first boot, at initrd time.[1]

Because such an operation works at the filesystem level, we need to be
able to read the SELinux labels first from the original root, and apply
them to the files of the new root. The previous commit enabled the
second part of this process; this commit enables the first part.

[1] https://github.com/coreos/fedora-coreos-tracker/issues/94

Signed-off-by: Jonathan Lebon <jlebon@redhat.com>
---
 security/selinux/hooks.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0b4e32161b7..a2caf6e2313 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3334,7 +3334,11 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
 	char *context = NULL;
 	struct inode_security_struct *isec;
 
-	if (strcmp(name, XATTR_SELINUX_SUFFIX))
+	/* If we're not initialized yet, then we can't validate contexts, so
+	 * just let vfs_getxattr fall back to using the on-disk xattr.
+	 */
+	if (!selinux_initialized(&selinux_state) ||
+	    strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
 
 	/*
-- 
2.25.4

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

Apologies, this should have had the subject line:

> [PATCH v3] selinux: allow reading labels before policy is loaded

I missed passing `-v 3` to `git format-patch`.

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Stephen Smalley]

On Wed, May 27, 2020 at 6:10 PM Jonathan Lebon <jlebon@redhat.com> wrote:
>
> This patch does for `getxattr` what commit 3e3e24b42043 ("selinux: allow
> labeling before policy is loaded") did for `setxattr`; it allows
> querying the current SELinux label on disk before the policy is loaded.
>
> One of the motivations described in that commit message also drives this
> patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be
> able to move the root filesystem for example, from xfs to ext4 on RAID,
> on first boot, at initrd time.[1]
>
> Because such an operation works at the filesystem level, we need to be
> able to read the SELinux labels first from the original root, and apply
> them to the files of the new root. The previous commit enabled the
> second part of this process; this commit enables the first part.
>
> [1] https://github.com/coreos/fedora-coreos-tracker/issues/94
>
> Signed-off-by: Jonathan Lebon <jlebon@redhat.com>

You might want to fix the comment style below, but otherwise,

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
>  security/selinux/hooks.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0b4e32161b7..a2caf6e2313 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3334,7 +3334,11 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void
>         char *context = NULL;
>         struct inode_security_struct *isec;
>
> -       if (strcmp(name, XATTR_SELINUX_SUFFIX))
> +       /* If we're not initialized yet, then we can't validate contexts, so
> +        * just let vfs_getxattr fall back to using the on-disk xattr.
> +        */

coding-style says that multi-line comment style is to use a separate
line for the opening /* unless in net/


> +       if (!selinux_initialized(&selinux_state) ||
> +           strcmp(name, XATTR_SELINUX_SUFFIX))
>                 return -EOPNOTSUPP;
>
>         /*
> --
> 2.25.4
>

Re: [PATCH] selinux: allow reading labels before policy is loaded

[Jonathan Lebon]

On Thu, May 28, 2020 at 9:42 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> You might want to fix the comment style below, but otherwise,
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Fixed in v4!

Thank you and Ondrej for the reviews.