Re: [PATCH] libselinux: selinux_set_mapping: fix handling of unknown classes/perms

[Stephen Smalley <sds@xxxxxxxxxxxxx>]

On 2/22/19 11:11 AM, Stephen Smalley wrote:
> On 2/22/19 9:41 AM, Stephen Smalley wrote:
> > The libselinux selinux_set_mapping() implementation was never updated
> > to handle unknown classes/permissions based on the policy handle_unknown
> > flag.  Update it and the internal mapping functions to gracefully
> > handle unknown classes/permissions.  Add a security_reject_unknown()
> > interface to expose the corresponding selinuxfs node and use it when
> > creating a mapping to decide whether to fail immediately or proceed.
> >
> > This enables dbus-daemon and XSELinux, which use selinux_set_mapping(),
> > to continue working with the dummy policy or other policies that lack
> > their userspace class/permission definitions as long as the policy
> > was built with -U allow.
> >
> > Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> > ---
> >   libselinux/include/selinux/selinux.h |  5 +-
> >   libselinux/src/compute_av.c          | 16 +++++--
> >   libselinux/src/mapping.c             | 71 ++++++++++++++++++++++------
> >   libselinux/src/mapping.h             |  4 +-
> >   libselinux/src/reject_unknown.c      | 40 ++++++++++++++++
> >   libselinux/src/selinux_internal.h    |  1 +
> >   6 files changed, 117 insertions(+), 20 deletions(-)
> >   create mode 100644 libselinux/src/reject_unknown.c
> >
> > diff --git a/libselinux/include/selinux/selinux.h 
> > b/libselinux/include/selinux/selinux.h
> > index 01201eee..a34d54fc 100644
> > --- a/libselinux/include/selinux/selinux.h
> > +++ b/libselinux/include/selinux/selinux.h
> > @@ -328,7 +328,10 @@ extern int security_getenforce(void);
> >   /* Set the enforce flag value. */
> >   extern int security_setenforce(int value);
> > -/* Get the behavior for undefined classes/permissions */
> > +/* Get the load-time behavior for undefined classes/permissions */
> > +extern int security_reject_unknown(void);
> > +
> > +/* Get the runtime behavior for undefined classes/permissions */
> >   extern int security_deny_unknown(void);
> >   /* Get the checkreqprot value */
> > diff --git a/libselinux/src/compute_av.c b/libselinux/src/compute_av.c
> > index 1d05e7b6..d5741637 100644
> > --- a/libselinux/src/compute_av.c
> > +++ b/libselinux/src/compute_av.c
> > @@ -20,6 +20,7 @@ int security_compute_av_flags_raw(const char * scon,
> >       char *buf;
> >       size_t len;
> >       int fd, ret;
> > +    security_class_t kclass;
> >       if (!selinux_mnt) {
> >           errno = ENOENT;
> > @@ -38,8 +39,9 @@ int security_compute_av_flags_raw(const char * scon,
> >           goto out;
> >       }
> > +    kclass = unmap_class(tclass);
> >       snprintf(buf, len, "%s %s %hu %x", scon, tcon,
> > -         unmap_class(tclass), unmap_perm(tclass, requested));
> > +         kclass, unmap_perm(tclass, requested));
> >       ret = write(fd, buf, strlen(buf));
> >       if (ret < 0)
> > @@ -60,9 +62,15 @@ int security_compute_av_flags_raw(const char * scon,
> >       } else if (ret < 6)
> >           avd->flags = 0;
> > -    /* If tclass invalid, kernel sets avd according to deny_unknown 
> > flag */
> > -    if (tclass != 0)
> > -        map_decision(tclass, avd);
> > +    /*
> > +     * If the tclass could not be mapped to a kernel class at all, the
> > +     * kernel will have already set avd according to the
> > +     * handle_unknown flag and we do not need to do anything further.
> > +     * Otherwise, we must map the permissions within the returned
> > +     * avd to the userspace permission values.
> > +     */
> > +    if (kclass != 0)
> > +        map_decision(tclass, avd, !security_deny_unknown());
>
> We probably want to cache security_deny_unknown() and refresh it upon a 
> policy reload.  If the callers were to use selinux_status_open() during 
> setup, they could get the current value via 
> selinux_status_deny_unknown() without requiring any further system 
> calls.  Or if using netlink selinux notifications, they could call 
> security_deny_unknown() again only when they receive a 
> SELNL_MSG_POLICYLOAD message.  Not clear how to best integrate it here 
> since security_compute_av*() can be called directly or via the AVC.

Actually, I should probably move the security_deny_unknown() call inside 
of map_decision().  Then we can omit it entirely if there is no mapping, 
i.e. caller is not using selinux_set_mapping().  That avoids any extra 
overhead on selinux_check_access() users.

> >       ret = 0;
> >         out2:
> > diff --git a/libselinux/src/mapping.c b/libselinux/src/mapping.c
> > index f205804b..892f887e 100644
> > --- a/libselinux/src/mapping.c
> > +++ b/libselinux/src/mapping.c
> > @@ -8,7 +8,9 @@
> >   #include <stdarg.h>
> >   #include <selinux/selinux.h>
> >   #include <selinux/avc.h>
> > +#include "callbacks.h"
> >   #include "mapping.h"
> > +#include "selinux_internal.h"
> >   /*
> >    * Class and permission mappings
> > @@ -33,6 +35,9 @@ selinux_set_mapping(struct security_class_mapping *map)
> >       size_t size = sizeof(struct selinux_mapping);
> >       security_class_t i, j;
> >       unsigned k;
> > +    bool print_unknown_handle = false;
> > +    bool reject = (security_reject_unknown() == 1);
> > +    bool deny = (security_deny_unknown() == 1);
> >       free(current_mapping);
> >       current_mapping = NULL;
> > @@ -62,8 +67,16 @@ selinux_set_mapping(struct security_class_mapping 
> > *map)
> >           struct selinux_mapping *p_out = current_mapping + j;
> >           p_out->value = string_to_security_class(p_in->name);
> > -        if (!p_out->value)
> > -            goto err2;
> > +        if (!p_out->value) {
> > +            selinux_log(SELINUX_INFO,
> > +                    "SELinux: Class %s not defined in policy.\n",
> > +                    p_in->name);
> > +            if (reject)
> > +                goto err2;
> > +            p_out->num_perms = 0;
> > +            print_unknown_handle = true;
> > +            continue;
> > +        }
> >           k = 0;
> >           while (p_in->perms[k]) {
> > @@ -74,13 +87,24 @@ selinux_set_mapping(struct security_class_mapping 
> > *map)
> >               }
> >               p_out->perms[k] = string_to_av_perm(p_out->value,
> >                                   p_in->perms[k]);
> > -            if (!p_out->perms[k])
> > -                goto err2;
> > +            if (!p_out->perms[k]) {
> > +                selinux_log(SELINUX_INFO,
> > +                        "SELinux:  Permission %s in class %s not 
> > defined in policy.\n",
> > +                        p_in->perms[k], p_in->name);
> > +                if (reject)
> > +                    goto err2;
> > +                print_unknown_handle = true;
> > +            }
> >               k++;
> >           }
> >           p_out->num_perms = k;
> >       }
> > +    if (print_unknown_handle)
> > +        selinux_log(SELINUX_INFO,
> > +                "SELinux: the above unknown classes and permissions 
> > will be %s\n",
> > +                deny ? "denied" : "allowed");
> > +
> >       /* Set the mapping size here so the above lookups are "raw" */
> >       current_mapping_size = i;
> >       return 0;
> > @@ -181,30 +205,49 @@ map_perm(security_class_t tclass, 
> > access_vector_t kperm)
> >   }
> >   void
> > -map_decision(security_class_t tclass, struct av_decision *avd)
> > +map_decision(security_class_t tclass, struct av_decision *avd,
> > +         bool allow_unknown)
> >   {
> >       if (tclass < current_mapping_size) {
> > -        unsigned i;
> > +        struct selinux_mapping *mapping = &current_mapping[tclass];
> > +        unsigned int i, n = mapping->num_perms;
> >           access_vector_t result;
> > -        for (i=0, result=0; i<current_mapping[tclass].num_perms; i++)
> > -            if (avd->allowed & current_mapping[tclass].perms[i])
> > +        for (i = 0, result = 0; i < n; i++) {
> > +            if (avd->allowed & mapping->perms[i])
> > +                result |= 1<<i;
> > +            if (allow_unknown && !mapping->perms[i])
> >                   result |= 1<<i;
> > +        }
> >           avd->allowed = result;
> > -        for (i=0, result=0; i<current_mapping[tclass].num_perms; i++)
> > -            if (avd->decided & current_mapping[tclass].perms[i])
> > +        for (i = 0, result = 0; i < n; i++) {
> > +            if (avd->decided & mapping->perms[i])
> > +                result |= 1<<i;
> > +            if (allow_unknown && !mapping->perms[i])
> >                   result |= 1<<i;
> > +        }
> >           avd->decided = result;
> > -        for (i=0, result=0; i<current_mapping[tclass].num_perms; i++)
> > -            if (avd->auditallow & current_mapping[tclass].perms[i])
> > +        for (i = 0, result = 0; i < n; i++)
> > +            if (avd->auditallow & mapping->perms[i])
> >                   result |= 1<<i;
> >           avd->auditallow = result;
> > -        for (i=0, result=0; i<current_mapping[tclass].num_perms; i++)
> > -            if (avd->auditdeny & current_mapping[tclass].perms[i])
> > +        for (i = 0, result = 0; i < n; i++) {
> > +            if (avd->auditdeny & mapping->perms[i])
> >                   result |= 1<<i;
> > +            if (!allow_unknown && !mapping->perms[i])
> > +                result |= 1<<i;
> > +        }
> > +
> > +        /*
> > +         * Make sure we audit denials for any permission check
> > +         * beyond the mapping->num_perms since this indicates
> > +         * a bug in the object manager.
> > +         */
> > +        for (; i < (sizeof(result)*8); i++)
> > +            result |= 1<<i;
> >           avd->auditdeny = result;
> >       }
> >   }
> > diff --git a/libselinux/src/mapping.h b/libselinux/src/mapping.h
> > index 22a4ccaf..12ebb89f 100644
> > --- a/libselinux/src/mapping.h
> > +++ b/libselinux/src/mapping.h
> > @@ -7,6 +7,7 @@
> >   #define _SELINUX_MAPPING_H_
> >   #include <selinux/selinux.h>
> > +#include <stdbool.h>
> >   /*
> >    * Get real, kernel values from mapped values
> > @@ -29,6 +30,7 @@ extern access_vector_t
> >   map_perm(security_class_t tclass, access_vector_t kperm);
> >   extern void
> > -map_decision(security_class_t tclass, struct av_decision *avd);
> > +map_decision(security_class_t tclass, struct av_decision *avd,
> > +         bool allow_unknown);
> >   #endif                /* _SELINUX_MAPPING_H_ */
> > diff --git a/libselinux/src/reject_unknown.c 
> > b/libselinux/src/reject_unknown.c
> > new file mode 100644
> > index 00000000..5c1d3605
> > --- /dev/null
> > +++ b/libselinux/src/reject_unknown.c
> > @@ -0,0 +1,40 @@
> > +#include <unistd.h>
> > +#include <sys/types.h>
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +#include "selinux_internal.h"
> > +#include "policy.h"
> > +#include <stdio.h>
> > +#include <limits.h>
> > +
> > +int security_reject_unknown(void)
> > +{
> > +    int fd, ret, reject_unknown = 0;
> > +    char path[PATH_MAX];
> > +    char buf[20];
> > +
> > +    if (!selinux_mnt) {
> > +        errno = ENOENT;
> > +        return -1;
> > +    }
> > +
> > +    snprintf(path, sizeof(path), "%s/reject_unknown", selinux_mnt);
> > +    fd = open(path, O_RDONLY | O_CLOEXEC);
> > +    if (fd < 0)
> > +        return -1;
> > +
> > +    memset(buf, 0, sizeof(buf));
> > +    ret = read(fd, buf, sizeof(buf) - 1);
> > +    close(fd);
> > +    if (ret < 0)
> > +        return -1;
> > +
> > +    if (sscanf(buf, "%d", &reject_unknown) != 1)
> > +        return -1;
> > +
> > +    return reject_unknown;
> > +}
> > +
> > +hidden_def(security_reject_unknown);
> > diff --git a/libselinux/src/selinux_internal.h 
> > b/libselinux/src/selinux_internal.h
> > index dfc421cc..70b5025d 100644
> > --- a/libselinux/src/selinux_internal.h
> > +++ b/libselinux/src/selinux_internal.h
> > @@ -59,6 +59,7 @@ hidden_proto(selinux_mkload_policy)
> >       hidden_proto(security_getenforce)
> >       hidden_proto(security_setenforce)
> >       hidden_proto(security_deny_unknown)
> > +    hidden_proto(security_reject_unknown)
> >       hidden_proto(security_get_checkreqprot)
> >       hidden_proto(selinux_boolean_sub)
> >       hidden_proto(selinux_current_policy_path)