[RFC PATCH] selinux: add note to avoid permissions with _perms suffix

[RFC PATCH] selinux: add note to avoid permissions with _perms suffix

[Christian Göttsche]

The suffix "_perms" is used in Referency Policy style policies for
permission macros, bundling several single raw permissions.

Add a note to not confuse policy writers/readers.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/include/classmap.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 986f3ac14282..b06ea7b23760 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -2,6 +2,11 @@
 #include <linux/capability.h>
 #include <linux/socket.h>
 
+/*
+ * Note: The name for a permission should not end with the suffix "_perms",
+ *       to prevent confusion with Refpolicy style permission macros.
+ */
+
 #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
     "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
 
-- 
2.26.2

Re: [RFC PATCH] selinux: add note to avoid permissions with _perms suffix

[Stephen Smalley]

On Thu, May 14, 2020 at 12:09 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The suffix "_perms" is used in Referency Policy style policies for
> permission macros, bundling several single raw permissions.
>
> Add a note to not confuse policy writers/readers.

I don't really see a valid justification and I wouldn't recommend
doing this via a comment alone if it were justified.
The kernel shouldn't be tied to refpolicy since refpolicy is merely
one SELinux policy configuration albeit widely used as the base for
most Linux distros (but not Android). If we were going to enforce a
naming convention on the classes/permissions, we should do it via a
build-time check using the existing scripts/selinux/genheaders program
that generates the symbols from classmap.h that are used by the kernel
code.  And this particular case seems highly improbable - who would
name a permission with a "_perms" (plural) suffix especially since no
other kernel permission has been so named to date.  The comment is
also a bit confusing since it occurs immediately before a macro that
ends in _PERMS is defined, but that macro presents no problem since it
is purely kernel-internal.  Absent some motivating example of where we
have broken refpolicy in the past, I can't see why we need this.

>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/include/classmap.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 986f3ac14282..b06ea7b23760 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -2,6 +2,11 @@
>  #include <linux/capability.h>
>  #include <linux/socket.h>
>
> +/*
> + * Note: The name for a permission should not end with the suffix "_perms",
> + *       to prevent confusion with Refpolicy style permission macros.
> + */
> +
>  #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
>      "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
>
> --
> 2.26.2
>

Re: [RFC PATCH] selinux: add note to avoid permissions with _perms suffix

[Paul Moore]

On Thu, May 14, 2020 at 12:36 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Thu, May 14, 2020 at 12:09 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > The suffix "_perms" is used in Referency Policy style policies for
> > permission macros, bundling several single raw permissions.
> >
> > Add a note to not confuse policy writers/readers.
>
> I don't really see a valid justification and I wouldn't recommend
> doing this via a comment alone if it were justified.
> The kernel shouldn't be tied to refpolicy since refpolicy is merely
> one SELinux policy configuration albeit widely used as the base for
> most Linux distros (but not Android). If we were going to enforce a
> naming convention on the classes/permissions, we should do it via a
> build-time check using the existing scripts/selinux/genheaders program
> that generates the symbols from classmap.h that are used by the kernel
> code.  And this particular case seems highly improbable - who would
> name a permission with a "_perms" (plural) suffix especially since no
> other kernel permission has been so named to date.  The comment is
> also a bit confusing since it occurs immediately before a macro that
> ends in _PERMS is defined, but that macro presents no problem since it
> is purely kernel-internal.  Absent some motivating example of where we
> have broken refpolicy in the past, I can't see why we need this.

Not that Stephen's points really need a "+1", but yes, +1.

-- 
paul moore
www.paul-moore.com