diff mbox series

[RFC,2/3] selinux: never allow relabeling on context mounts

Message ID 20181213141739.8534-3-omosnace@redhat.com (mailing list archive)
State Superseded
Headers show
Series Fix SELinux context mount with the cgroup filesystem | expand

Commit Message

Ondrej Mosnacek Dec. 13, 2018, 2:17 p.m. UTC
In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
files/directories, so we should never set the SBLABEL_MNT flag in this
case. The 'special handling' in selinux_is_sblabel_mnt() is only
intended for SECURITY_FS_USE_GENFS.

While there, make the logic in selinux_is_sblabel_mnt() more explicit
and add a BUILD_BUG_ON() to make sure that introducing a new
SECURITY_FS_USE_* forces a review of the logic.

Note that checkpatch.pl produces some false positives here, likely
having problems recognizing the monstrous return statement...

Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 41 ++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Stephen Smalley Dec. 13, 2018, 4:18 p.m. UTC | #1
On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
> files/directories, so we should never set the SBLABEL_MNT flag in this
> case. The 'special handling' in selinux_is_sblabel_mnt() is only
> intended for SECURITY_FS_USE_GENFS.
> 
> While there, make the logic in selinux_is_sblabel_mnt() more explicit
> and add a BUILD_BUG_ON() to make sure that introducing a new
> SECURITY_FS_USE_* forces a review of the logic.
> 
> Note that checkpatch.pl produces some false positives here, likely
> having problems recognizing the monstrous return statement...
> 
> Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>   security/selinux/hooks.c | 41 ++++++++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7ce012d9ec51..d6d29ec54eab 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
>   {
>   	struct superblock_security_struct *sbsec = sb->s_security;
>   
> -	return sbsec->behavior == SECURITY_FS_USE_XATTR ||
> -		sbsec->behavior == SECURITY_FS_USE_TRANS ||
> -		sbsec->behavior == SECURITY_FS_USE_TASK ||
> -		sbsec->behavior == SECURITY_FS_USE_NATIVE ||
> +	/*
> +	 * IMPORTANT: Double-check logic in this function when adding a new
> +	 * SECURITY_FS_USE_* definition!
> +	 */
> +	BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
> +
> +	switch (sbsec->behavior) {
> +	case SECURITY_FS_USE_XATTR:
> +	case SECURITY_FS_USE_TRANS:
> +	case SECURITY_FS_USE_TASK:
> +	case SECURITY_FS_USE_NATIVE:
> +		return 1;
> +
> +	case SECURITY_FS_USE_GENFS:
>   		/* Special handling. Genfs but also in-core setxattr handler */
> -		!strcmp(sb->s_type->name, "sysfs") ||
> -		!strcmp(sb->s_type->name, "pstore") ||
> -		!strcmp(sb->s_type->name, "debugfs") ||
> -		!strcmp(sb->s_type->name, "tracefs") ||
> -		!strcmp(sb->s_type->name, "rootfs") ||
> -		(selinux_policycap_cgroupseclabel() &&
> -		 (!strcmp(sb->s_type->name, "cgroup") ||
> -		  !strcmp(sb->s_type->name, "cgroup2")));
> +		return	!strcmp(sb->s_type->name, "sysfs") ||
> +			!strcmp(sb->s_type->name, "pstore") ||
> +			!strcmp(sb->s_type->name, "debugfs") ||
> +			!strcmp(sb->s_type->name, "tracefs") ||
> +			!strcmp(sb->s_type->name, "rootfs") ||
> +			(selinux_policycap_cgroupseclabel() &&
> +			 (!strcmp(sb->s_type->name, "cgroup") ||
> +			  !strcmp(sb->s_type->name, "cgroup2")));
> +
> +	/* Never allow relabeling on context mounts */
> +	case SECURITY_FS_USE_MNTPOINT:
> +	case SECURITY_FS_USE_NONE:
> +	default:
> +		return 0;
> +	}
>   }

This looks sane to me. Note that 
https://github.com/SELinuxProject/selinux-kernel/issues/2 calls for 
replacing the use of hardcoded lists of filesystem types above with 
something more general.  Maybe you could at least abstract that part 
into its own function.

>   
>   static int sb_finish_set_opts(struct super_block *sb)
>
Ondrej Mosnacek Dec. 18, 2018, 3:38 p.m. UTC | #2
On Thu, Dec 13, 2018 at 5:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> > In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
> > files/directories, so we should never set the SBLABEL_MNT flag in this
> > case. The 'special handling' in selinux_is_sblabel_mnt() is only
> > intended for SECURITY_FS_USE_GENFS.
> >
> > While there, make the logic in selinux_is_sblabel_mnt() more explicit
> > and add a BUILD_BUG_ON() to make sure that introducing a new
> > SECURITY_FS_USE_* forces a review of the logic.
> >
> > Note that checkpatch.pl produces some false positives here, likely
> > having problems recognizing the monstrous return statement...
> >
> > Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >   security/selinux/hooks.c | 41 ++++++++++++++++++++++++++++------------
> >   1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 7ce012d9ec51..d6d29ec54eab 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
> >   {
> >       struct superblock_security_struct *sbsec = sb->s_security;
> >
> > -     return sbsec->behavior == SECURITY_FS_USE_XATTR ||
> > -             sbsec->behavior == SECURITY_FS_USE_TRANS ||
> > -             sbsec->behavior == SECURITY_FS_USE_TASK ||
> > -             sbsec->behavior == SECURITY_FS_USE_NATIVE ||
> > +     /*
> > +      * IMPORTANT: Double-check logic in this function when adding a new
> > +      * SECURITY_FS_USE_* definition!
> > +      */
> > +     BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
> > +
> > +     switch (sbsec->behavior) {
> > +     case SECURITY_FS_USE_XATTR:
> > +     case SECURITY_FS_USE_TRANS:
> > +     case SECURITY_FS_USE_TASK:
> > +     case SECURITY_FS_USE_NATIVE:
> > +             return 1;
> > +
> > +     case SECURITY_FS_USE_GENFS:
> >               /* Special handling. Genfs but also in-core setxattr handler */
> > -             !strcmp(sb->s_type->name, "sysfs") ||
> > -             !strcmp(sb->s_type->name, "pstore") ||
> > -             !strcmp(sb->s_type->name, "debugfs") ||
> > -             !strcmp(sb->s_type->name, "tracefs") ||
> > -             !strcmp(sb->s_type->name, "rootfs") ||
> > -             (selinux_policycap_cgroupseclabel() &&
> > -              (!strcmp(sb->s_type->name, "cgroup") ||
> > -               !strcmp(sb->s_type->name, "cgroup2")));
> > +             return  !strcmp(sb->s_type->name, "sysfs") ||
> > +                     !strcmp(sb->s_type->name, "pstore") ||
> > +                     !strcmp(sb->s_type->name, "debugfs") ||
> > +                     !strcmp(sb->s_type->name, "tracefs") ||
> > +                     !strcmp(sb->s_type->name, "rootfs") ||
> > +                     (selinux_policycap_cgroupseclabel() &&
> > +                      (!strcmp(sb->s_type->name, "cgroup") ||
> > +                       !strcmp(sb->s_type->name, "cgroup2")));
> > +
> > +     /* Never allow relabeling on context mounts */
> > +     case SECURITY_FS_USE_MNTPOINT:
> > +     case SECURITY_FS_USE_NONE:
> > +     default:
> > +             return 0;
> > +     }
> >   }
>
> This looks sane to me. Note that
> https://github.com/SELinuxProject/selinux-kernel/issues/2 calls for
> replacing the use of hardcoded lists of filesystem types above with
> something more general.  Maybe you could at least abstract that part
> into its own function.

Agreed, that's a good idea. It should also allow checkpatch.pl to
correctly parse the control flow and not throw an error.

>
> >
> >   static int sb_finish_set_opts(struct super_block *sb)
> >
>

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7ce012d9ec51..d6d29ec54eab 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -501,19 +501,36 @@  static int selinux_is_sblabel_mnt(struct super_block *sb)
 {
 	struct superblock_security_struct *sbsec = sb->s_security;
 
-	return sbsec->behavior == SECURITY_FS_USE_XATTR ||
-		sbsec->behavior == SECURITY_FS_USE_TRANS ||
-		sbsec->behavior == SECURITY_FS_USE_TASK ||
-		sbsec->behavior == SECURITY_FS_USE_NATIVE ||
+	/*
+	 * IMPORTANT: Double-check logic in this function when adding a new
+	 * SECURITY_FS_USE_* definition!
+	 */
+	BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
+
+	switch (sbsec->behavior) {
+	case SECURITY_FS_USE_XATTR:
+	case SECURITY_FS_USE_TRANS:
+	case SECURITY_FS_USE_TASK:
+	case SECURITY_FS_USE_NATIVE:
+		return 1;
+
+	case SECURITY_FS_USE_GENFS:
 		/* Special handling. Genfs but also in-core setxattr handler */
-		!strcmp(sb->s_type->name, "sysfs") ||
-		!strcmp(sb->s_type->name, "pstore") ||
-		!strcmp(sb->s_type->name, "debugfs") ||
-		!strcmp(sb->s_type->name, "tracefs") ||
-		!strcmp(sb->s_type->name, "rootfs") ||
-		(selinux_policycap_cgroupseclabel() &&
-		 (!strcmp(sb->s_type->name, "cgroup") ||
-		  !strcmp(sb->s_type->name, "cgroup2")));
+		return	!strcmp(sb->s_type->name, "sysfs") ||
+			!strcmp(sb->s_type->name, "pstore") ||
+			!strcmp(sb->s_type->name, "debugfs") ||
+			!strcmp(sb->s_type->name, "tracefs") ||
+			!strcmp(sb->s_type->name, "rootfs") ||
+			(selinux_policycap_cgroupseclabel() &&
+			 (!strcmp(sb->s_type->name, "cgroup") ||
+			  !strcmp(sb->s_type->name, "cgroup2")));
+
+	/* Never allow relabeling on context mounts */
+	case SECURITY_FS_USE_MNTPOINT:
+	case SECURITY_FS_USE_NONE:
+	default:
+		return 0;
+	}
 }
 
 static int sb_finish_set_opts(struct super_block *sb)