Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types to separate classes

[Stephen Smalley]

On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> teach SELinux about anonymous inodes") to use a single class for all
> anon inodes and let the policy distinguish between them using named
> transitions turned out to have a rather unfortunate drawback.
>
> For example, suppose we have two types of anon inodes, "A" and "B", and
> we want to allow a set of domains (represented by an attribute "attr_x")
> certain set of permissions on anon inodes of type "A" that were created
> by the same domain, but at the same time disallow this set to access
> anon inodes of type "B" entirely. Since all inodes share the same class
> and we want to distinguish both the inode types and the domains that
> created them, we have no choice than to create separate types for the
> cartesian product of (domains that belong to attr_x) x ("A", "B") and
> add all the necessary allow and transition rules for each domain
> individually.
>
> This makes it very impractical to write sane policies for anon inodes in
> the future, as more anon inode types are added. Therefore, this patch
> implements an alternative approach that assigns a separate class to each
> type of anon inode. This allows the example above to be implemented
> without any transition rules and with just a single allow rule:
>
> allow attr_x self:A { ... };
>
> In order to not break possible existing users of the already merged
> original approach, this patch also adds a new policy capability
> "extended_anon_inode_class" that needs to be set by the policy to enable
> the new behavior.
>
> I decided to keep the named transition mechanism in the new variant,
> since there might eventually be some extra information in the anon inode
> name that could be used in transitions.
>
> One minor annoyance is that the kernel still expects the policy to
> provide both classes (anon_inode and userfaultfd) regardless of the
> capability setting and if one of them is not defined in the policy, the
> kernel will print a warning when loading the policy. However, it doesn't
> seem worth to work around that in the kernel, as the policy can provide
> just the definition of the unused class(es) (and permissions) to avoid
> this warning. Keeping the legacy anon_inode class with some fallback
> rules may also be desirable to keep the policy compatible with kernels
> that only support anon_inode.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>

NAK.  We do not want to introduce a new security class for every user
of anon inodes - that isn't what security classes are for.
For things like kvm device inodes, those should ultimately use the
inherited context from the related inode (the /dev/kvm inode itself).
That was the original intent of supporting the related inode.

> ---
>  security/selinux/hooks.c                   | 27 +++++++++++++++++++++-
>  security/selinux/include/classmap.h        |  2 ++
>  security/selinux/include/policycap.h       |  1 +
>  security/selinux/include/policycap_names.h |  3 ++-
>  security/selinux/include/security.h        |  7 ++++++
>  5 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index dc57ba21d8ff..20a8d7d17936 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3079,7 +3079,32 @@ static int selinux_inode_init_security_anon(struct inode *inode,
>                 isec->sclass = context_isec->sclass;
>                 isec->sid = context_isec->sid;
>         } else {
> -               isec->sclass = SECCLASS_ANON_INODE;
> +               /*
> +                * If the check below fails:
> +                *  1. Add the corresponding security class to
> +                *     security/selinux/include/classmap.h
> +                *  2. Map the new LSM_ANON_INODE_* value to the class in
> +                *     the switch statement below.
> +                *  3. Update the RHS of the comparison in the BUILD_BUG_ON().
> +                *  4. CC selinux@xxxxxxxxxxxxxxx and
> +                *     linux-security-module@xxxxxxxxxxxxxxx when submitting
> +                *     the patch or in case of any questions.
> +                */
> +               BUILD_BUG_ON(LSM_ANON_INODE_MAX > LSM_ANON_INODE_USERFAULTFD);
> +
> +               if (selinux_policycap_extended_anon_inode()) {
> +                       switch (type) {
> +                       case LSM_ANON_INODE_USERFAULTFD:
> +                               isec->sclass = SECCLASS_USERFAULTFD;
> +                               break;
> +                       default:
> +                               pr_err("SELinux:  got invalid anon inode type: %d",
> +                                      (int)type);
> +                               return -EINVAL;
> +                       }
> +               } else {
> +                       isec->sclass = SECCLASS_ANON_INODE;
> +               }
>                 rc = security_transition_sid(
>                         &selinux_state, tsec->sid, tsec->sid,
>                         isec->sclass, name, &isec->sid);
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index ba2e01a6955c..e4308cad6407 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -251,6 +251,8 @@ struct security_class_mapping secclass_map[] = {
>           { "integrity", "confidentiality", NULL } },
>         { "anon_inode",
>           { COMMON_FILE_PERMS, NULL } },
> +       { "userfaultfd",
> +         { COMMON_FILE_PERMS, NULL } },
>         { NULL }
>    };
>
> diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
> index 2ec038efbb03..969804bd6dab 100644
> --- a/security/selinux/include/policycap.h
> +++ b/security/selinux/include/policycap.h
> @@ -11,6 +11,7 @@ enum {
>         POLICYDB_CAPABILITY_CGROUPSECLABEL,
>         POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
>         POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
> +       POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS,
>         __POLICYDB_CAPABILITY_MAX
>  };
>  #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
> index b89289f092c9..78651990425e 100644
> --- a/security/selinux/include/policycap_names.h
> +++ b/security/selinux/include/policycap_names.h
> @@ -12,7 +12,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
>         "always_check_network",
>         "cgroup_seclabel",
>         "nnp_nosuid_transition",
> -       "genfs_seclabel_symlinks"
> +       "genfs_seclabel_symlinks",
> +       "extended_anon_inode_class",
>  };
>
>  #endif /* _SELINUX_POLICYCAP_NAMES_H_ */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 7130c9648ad1..4fb75101aca4 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -219,6 +219,13 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
>         return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS]);
>  }
>
> +static inline bool selinux_policycap_extended_anon_inode(void)
> +{
> +       struct selinux_state *state = &selinux_state;
> +
> +       return READ_ONCE(state->policycap[POLICYDB_CAPABILITY_EXTENDED_ANON_INODE_CLASS]);
> +}
> +
>  int security_mls_enabled(struct selinux_state *state);
>  int security_load_policy(struct selinux_state *state,
>                         void *data, size_t len,
> --
> 2.30.2
>