[PATCH] selinux: Allow file owner to set "security.sehash"

[PATCH] selinux: Allow file owner to set "security.sehash"

[Chirantan Ekbote]

Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a
particular filesystem in order to set a security xattr. However, this
restriction is relaxed for the security.selinux xattr: the file owner
or a process with CAP_FOWNER in its namespace may set this attribute.

Apply this relaxed restriction to the security.sehash xattr as well.
Since this xattr is mainly a performance optimization when labeling
files recursively it shouldn't have stricter requirements than setting
the selinux xattr in the first place.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 include/uapi/linux/xattr.h | 3 +++
 security/selinux/hooks.c   | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
index c1395b5bd432a..b700c8ffc3f1a 100644
--- a/include/uapi/linux/xattr.h
+++ b/include/uapi/linux/xattr.h
@@ -53,6 +53,9 @@
 #define XATTR_SELINUX_SUFFIX "selinux"
 #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
 
+#define XATTR_SEHASH_SUFFIX "sehash"
+#define XATTR_NAME_SEHASH XATTR_SECURITY_PREFIX XATTR_SEHASH_SUFFIX
+
 #define XATTR_SMACK_SUFFIX "SMACK64"
 #define XATTR_SMACK_IPIN "SMACK64IPIN"
 #define XATTR_SMACK_IPOUT "SMACK64IPOUT"
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4c037c2545c16..776df2ec85a82 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3121,9 +3121,10 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 	struct superblock_security_struct *sbsec;
 	struct common_audit_data ad;
 	u32 newsid, sid = current_sid();
+	const bool is_sehash = !strcmp(name, XATTR_NAME_SEHASH);
 	int rc = 0;
 
-	if (strcmp(name, XATTR_NAME_SELINUX)) {
+	if (strcmp(name, XATTR_NAME_SELINUX) && !is_sehash) {
 		rc = cap_inode_setxattr(dentry, name, value, size, flags);
 		if (rc)
 			return rc;
@@ -3143,6 +3144,10 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 	if (!inode_owner_or_capable(inode))
 		return -EPERM;
 
+	/* No more checks needed for security.sehash. */
+	if (is_sehash)
+		return 0;
+
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Stephen Smalley]

On Mon, Jun 1, 2020 at 3:29 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
>
> Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a
> particular filesystem in order to set a security xattr. However, this
> restriction is relaxed for the security.selinux xattr: the file owner
> or a process with CAP_FOWNER in its namespace may set this attribute.
>
> Apply this relaxed restriction to the security.sehash xattr as well.
> Since this xattr is mainly a performance optimization when labeling
> files recursively it shouldn't have stricter requirements than setting
> the selinux xattr in the first place.

First, setting either security.<non-selinux> or security.selinux has
an additional MAC check beyond the DAC/capability check; in the former
case there is the FILE__SETATTR check and in the latter there are the
FILE__RELABELFROM/TO checks.  We need to preserve some kind of SELinux
permission check here.

Second, security.sehash logic in userspace was introduced by Android
in its libselinux fork and then copied in upstream logic.  I'm not
sure Android wants to relax the current requirement for CAP_SYS_ADMIN
- I have copied them above.  A possible concern is that an
unprivileged process could disable the relabeling of a part of the
tree that it owns upon an upgrade, which could have unexpected
consequences.

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Chirantan Ekbote]

On Mon, Jun 1, 2020 at 9:42 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Jun 1, 2020 at 3:29 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> >
> > Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a
> > particular filesystem in order to set a security xattr. However, this
> > restriction is relaxed for the security.selinux xattr: the file owner
> > or a process with CAP_FOWNER in its namespace may set this attribute.
> >
> > Apply this relaxed restriction to the security.sehash xattr as well.
> > Since this xattr is mainly a performance optimization when labeling
> > files recursively it shouldn't have stricter requirements than setting
> > the selinux xattr in the first place.
>
> First, setting either security.<non-selinux> or security.selinux has
> an additional MAC check beyond the DAC/capability check; in the former
> case there is the FILE__SETATTR check and in the latter there are the
> FILE__RELABELFROM/TO checks.  We need to preserve some kind of SELinux
> permission check here.
>

So I understand correctly, what you're asking for is to change this section:

    if (is_sehash)
        return 0;

to this instead:

    if (is_sehash)
        return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);

Is that correct?

> Second, security.sehash logic in userspace was introduced by Android
> in its libselinux fork and then copied in upstream logic.  I'm not
> sure Android wants to relax the current requirement for CAP_SYS_ADMIN
> - I have copied them above.  A possible concern is that an
> unprivileged process could disable the relabeling of a part of the
> tree that it owns upon an upgrade, which could have unexpected
> consequences.

That's a good point.  Is this not an issue for the selinux xattr
because the selinux check could prevent a process from changing the
label of a file it owns?


The background for this patch is that I have a fuse server that runs
in a user namespace.  It runs as root in that namespace and keeps all
the file system caps so that it can set selinux xattrs.  However, it
cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent
namespace.  Looking at the code I thought that might have just been an
oversight but if it's intentional then do you have any suggestions for
how to make this work?  I'd rather not weaken the sandbox for this
process just so that it can set this one xattr.

Thanks,
Chirantan

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Stephen Smalley]

On Fri, Jun 5, 2020 at 2:21 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
>
> On Mon, Jun 1, 2020 at 9:42 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Mon, Jun 1, 2020 at 3:29 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> > >
> > > Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a
> > > particular filesystem in order to set a security xattr. However, this
> > > restriction is relaxed for the security.selinux xattr: the file owner
> > > or a process with CAP_FOWNER in its namespace may set this attribute.
> > >
> > > Apply this relaxed restriction to the security.sehash xattr as well.
> > > Since this xattr is mainly a performance optimization when labeling
> > > files recursively it shouldn't have stricter requirements than setting
> > > the selinux xattr in the first place.
> >
> > First, setting either security.<non-selinux> or security.selinux has
> > an additional MAC check beyond the DAC/capability check; in the former
> > case there is the FILE__SETATTR check and in the latter there are the
> > FILE__RELABELFROM/TO checks.  We need to preserve some kind of SELinux
> > permission check here.
> >
>
> So I understand correctly, what you're asking for is to change this section:
>
>     if (is_sehash)
>         return 0;
>
> to this instead:
>
>     if (is_sehash)
>         return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
>
> Is that correct?

I would suggest using FILE__RELABELFROM instead, under the raionale
that a process that is allowed to set security.sehash would also be
allowed to set security.selinux.  In contrast, a process might be
alowed to set another attribute (FILE__SETATTR) but not
security.selinux or security.sehash.

> > Second, security.sehash logic in userspace was introduced by Android
> > in its libselinux fork and then copied in upstream logic.  I'm not
> > sure Android wants to relax the current requirement for CAP_SYS_ADMIN
> > - I have copied them above.  A possible concern is that an
> > unprivileged process could disable the relabeling of a part of the
> > tree that it owns upon an upgrade, which could have unexpected
> > consequences.
>
> That's a good point.  Is this not an issue for the selinux xattr
> because the selinux check could prevent a process from changing the
> label of a file it owns?

Correct.

> The background for this patch is that I have a fuse server that runs
> in a user namespace.  It runs as root in that namespace and keeps all
> the file system caps so that it can set selinux xattrs.  However, it
> cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent
> namespace.  Looking at the code I thought that might have just been an
> oversight but if it's intentional then do you have any suggestions for
> how to make this work?  I'd rather not weaken the sandbox for this
> process just so that it can set this one xattr.

I'd be willing to move from requiring CAP_SYS_ADMIN to performing a
SELinux permission check (either FILE__RELABELFROM or a new one), but
I'd like the Android folks to chime in here.  Maybe you can ping them
through other channels since they haven't responded yet.

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Stephen Smalley]

On Fri, Jun 5, 2020 at 8:23 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jun 5, 2020 at 2:21 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> >
> > On Mon, Jun 1, 2020 at 9:42 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Mon, Jun 1, 2020 at 3:29 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> > > >
> > > > Normally a process needs CAP_SYS_ADMIN in the namespace that mounted a
> > > > particular filesystem in order to set a security xattr. However, this
> > > > restriction is relaxed for the security.selinux xattr: the file owner
> > > > or a process with CAP_FOWNER in its namespace may set this attribute.
> > > >
> > > > Apply this relaxed restriction to the security.sehash xattr as well.
> > > > Since this xattr is mainly a performance optimization when labeling
> > > > files recursively it shouldn't have stricter requirements than setting
> > > > the selinux xattr in the first place.
> > >
> > > First, setting either security.<non-selinux> or security.selinux has
> > > an additional MAC check beyond the DAC/capability check; in the former
> > > case there is the FILE__SETATTR check and in the latter there are the
> > > FILE__RELABELFROM/TO checks.  We need to preserve some kind of SELinux
> > > permission check here.
> > >
> >
> > So I understand correctly, what you're asking for is to change this section:
> >
> >     if (is_sehash)
> >         return 0;
> >
> > to this instead:
> >
> >     if (is_sehash)
> >         return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> >
> > Is that correct?
>
> I would suggest using FILE__RELABELFROM instead, under the raionale
> that a process that is allowed to set security.sehash would also be
> allowed to set security.selinux.  In contrast, a process might be
> alowed to set another attribute (FILE__SETATTR) but not
> security.selinux or security.sehash.
>
> > > Second, security.sehash logic in userspace was introduced by Android
> > > in its libselinux fork and then copied in upstream logic.  I'm not
> > > sure Android wants to relax the current requirement for CAP_SYS_ADMIN
> > > - I have copied them above.  A possible concern is that an
> > > unprivileged process could disable the relabeling of a part of the
> > > tree that it owns upon an upgrade, which could have unexpected
> > > consequences.
> >
> > That's a good point.  Is this not an issue for the selinux xattr
> > because the selinux check could prevent a process from changing the
> > label of a file it owns?
>
> Correct.
>
> > The background for this patch is that I have a fuse server that runs
> > in a user namespace.  It runs as root in that namespace and keeps all
> > the file system caps so that it can set selinux xattrs.  However, it
> > cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent
> > namespace.  Looking at the code I thought that might have just been an
> > oversight but if it's intentional then do you have any suggestions for
> > how to make this work?  I'd rather not weaken the sandbox for this
> > process just so that it can set this one xattr.
>
> I'd be willing to move from requiring CAP_SYS_ADMIN to performing a
> SELinux permission check (either FILE__RELABELFROM or a new one), but
> I'd like the Android folks to chime in here.  Maybe you can ping them
> through other channels since they haven't responded yet.

One thing to note however is that setting of security.sehash is just
an optimization to avoid unnecessarily walking the tree again.
Not sure if that matters for your use case.

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Chirantan Ekbote]

On Fri, Jun 5, 2020 at 9:23 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jun 5, 2020 at 2:21 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> >
>
> > The background for this patch is that I have a fuse server that runs
> > in a user namespace.  It runs as root in that namespace and keeps all
> > the file system caps so that it can set selinux xattrs.  However, it
> > cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent
> > namespace.  Looking at the code I thought that might have just been an
> > oversight but if it's intentional then do you have any suggestions for
> > how to make this work?  I'd rather not weaken the sandbox for this
> > process just so that it can set this one xattr.
>
> I'd be willing to move from requiring CAP_SYS_ADMIN to performing a
> SELinux permission check (either FILE__RELABELFROM or a new one), but
> I'd like the Android folks to chime in here.  Maybe you can ping them
> through other channels since they haven't responded yet.

I contacted them separately and they are not interested in relaxing
the requirements and also said that the kernel shouldn't have any
knowledge of the sehash xattr.  So I guess we can just drop this.

Thanks,
Chirantan

Re: [PATCH] selinux: Allow file owner to set "security.sehash"

[Stephen Smalley]

On Fri, Jun 12, 2020 at 12:00 AM Chirantan Ekbote
<chirantan@chromium.org> wrote:
>
> On Fri, Jun 5, 2020 at 9:23 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jun 5, 2020 at 2:21 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> > >
> >
> > > The background for this patch is that I have a fuse server that runs
> > > in a user namespace.  It runs as root in that namespace and keeps all
> > > the file system caps so that it can set selinux xattrs.  However, it
> > > cannot set the sehash xattr as that needs CAP_SYS_ADMIN in the parent
> > > namespace.  Looking at the code I thought that might have just been an
> > > oversight but if it's intentional then do you have any suggestions for
> > > how to make this work?  I'd rather not weaken the sandbox for this
> > > process just so that it can set this one xattr.
> >
> > I'd be willing to move from requiring CAP_SYS_ADMIN to performing a
> > SELinux permission check (either FILE__RELABELFROM or a new one), but
> > I'd like the Android folks to chime in here.  Maybe you can ping them
> > through other channels since they haven't responded yet.
>
> I contacted them separately and they are not interested in relaxing
> the requirements and also said that the kernel shouldn't have any
> knowledge of the sehash xattr.  So I guess we can just drop this.

Ok.  Setting of security.sehash is optional so you can always just
leave it disabled.  Only downside is it will then have to walk the
entire directory tree each time to check the labels.