Re: [RFC PATCH 2/3] libselinux: add security_is_policy_capabilty_enabled()

["Christian Göttsche" <cgzones@googlemail.com>]

Am Fr., 10. Jan. 2020 um 15:29 Uhr schrieb Stephen Smalley <sds@tycho.nsa.gov>:
>
> On 1/10/20 9:15 AM, Christian Göttsche wrote:
> > Allow userspace (e.g. object managers like systemd) to obtain the state of a policy capability via a library call.
> > ---
> >   libselinux/include/selinux/selinux.h          |  3 +
> >   .../security_is_policy_capability_enabled.3   | 27 ++++++++
> >   libselinux/src/polcap.c                       | 64 +++++++++++++++++++
> >   libselinux/src/selinux_internal.h             |  1 +
> >   libselinux/src/selinuxswig_python_exception.i |  9 +++
> >   5 files changed, 104 insertions(+)
> >   create mode 100644 libselinux/man/man3/security_is_policy_capability_enabled.3
> >   create mode 100644 libselinux/src/polcap.c
> >
> > diff --git a/libselinux/include/selinux/selinux.h b/libselinux/include/selinux/selinux.h
> > index fe46e681..b46f152d 100644
> > --- a/libselinux/include/selinux/selinux.h
> > +++ b/libselinux/include/selinux/selinux.h
> > @@ -354,6 +354,9 @@ extern int security_disable(void);
> >   /* Get the policy version number. */
> >   extern int security_policyvers(void);
> >
> > +/* Get the state of a policy capability. */
> > +extern int security_is_policy_capability_enabled(const char *name);
>
> Not sure if this should be security_ or selinux_.  Historically, we
> originally used security_ as the prefix for security server interfaces
> (e.g. security_compute_av), avc_ as the prefix for AVC interfaces, and
> no prefix at all for various other interfaces (e.g. getcon, getfilecon).
>   Then people pointed out the potential for name collisions (even more
> so in a multi-LSM world) and we started using selinux_ prefixes (but not
> consistently).  I'm ok either way but thought I'd mention it.
>

I'll think about it..

> > +
> >   /* Get the boolean names */
> >   extern int security_get_boolean_names(char ***names, int *len);
> >
> > diff --git a/libselinux/man/man3/security_is_policy_capability_enabled.3 b/libselinux/man/man3/security_is_policy_capability_enabled.3
> > new file mode 100644
> > index 00000000..18c53b67
> > --- /dev/null
> > +++ b/libselinux/man/man3/security_is_policy_capability_enabled.3
> > @@ -0,0 +1,27 @@
> > +.TH "security_is_policy_capability_enabled" "3" "9 January 2020" "cgzones@googlemail.com" "SELinux API documentation"
> > +.SH "NAME"
> > +security_is_policy_capability_enabled \- get the state of a SELinux policy
> > +capability
> > +.
> > +.SH "SYNOPSIS"
> > +.B #include <selinux/selinux.h>
> > +.sp
> > +.BI "int security_is_policy_capability_enabled(const char *" name ");"
> > +.
> > +.SH "DESCRIPTION"
> > +.BR security_is_policy_capability_enabled ()
> > +returns 1 if the SELinux policy capability with the given name is enabled,
> > +0 if it is disabled, and \-1 on error.
> > +.SH "NOTES"
> > +The parameter
> > +.IR name
> > +is case insensitive.
>
> Why support case-insensitivity?  It complicates the implementation and
> seems unnecessary.
>

sepol_polcap_getnum() does it:
https://github.com/SELinuxProject/selinux/blob/5bbe32a7e585dcd403739ea55a2fb25cbd184383/libsepol/src/polcaps.c#L25

>
> > +
> > +If the the current kernel does not support the given policy capability \-1 is returned and
> > +.BR errno
> > +is set to
> > +.BR ENOTSUP
> > +\&.
> > +.
> > +.SH "SEE ALSO"
> > +.BR selinux "(8)"
> > diff --git a/libselinux/src/polcap.c b/libselinux/src/polcap.c
> > new file mode 100644
> > index 00000000..801231cf
> > --- /dev/null
> > +++ b/libselinux/src/polcap.c
> > @@ -0,0 +1,64 @@
> > +#include <dirent.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include "policy.h"
> > +#include "selinux_internal.h"
> > +
> > +int security_is_policy_capability_enabled(const char *name)
> > +{
> > +     int fd, enabled;
> > +     ssize_t ret;
> > +     char path[PATH_MAX];
> > +     char buf[20];
> > +     DIR *polcapdir;
> > +     struct dirent *dentry;
> > +
> > +     if (!selinux_mnt) {
> > +             errno = ENOENT;
> > +             return -1;
> > +     }
> > +
> > +     snprintf(path, sizeof path, "%s/policy_capabilities", selinux_mnt);
> > +     polcapdir = opendir(path);
> > +     if (!polcapdir)
> > +             return -1;
> > +
> > +     while ((dentry = readdir(polcapdir)) != NULL) {
> > +             if (strcmp(dentry->d_name, ".") == 0 || strcmp(dentry->d_name, "..") == 0)
> > +                     continue;
> > +
> > +             if (strcasecmp(name, dentry->d_name) != 0)
> > +                     continue;
> > +
> > +             snprintf(path, sizeof path, "%s/policy_capabilities/%s", selinux_mnt, dentry->d_name);
> > +             fd = open(path, O_RDONLY | O_CLOEXEC);
> > +             if (fd < 0)
> > +                 goto err;
>
> If you weren't trying to support case-insensitivity, you could just
> directly open() the capability file and be done with it.
>

Would we need to check for directory traversals in that case?
char *tainted_userdata = "../../../../etc/passwd"
security_is_policy_capability_enabled(tainted_userdata)

>
> > +
> > +             memset(buf, 0, sizeof buf);
> > +             ret = read(fd, buf, sizeof buf - 1);
> > +             close(fd);
> > +             if (ret < 0)
> > +                     goto err;
> > +
> > +             if (sscanf(buf, "%d", &enabled) != 1)
> > +                     goto err;
> > +
> > +             closedir(polcapdir);
> > +             return !!enabled;
> > +     }
> > +
> > +     if (errno == 0)
> > +             errno = ENOTSUP;
> > +err:
> > +     closedir(polcapdir);
> > +     return -1;
> > +}
> > +
> > +hidden_def(security_is_policy_capability_enabled)
>
> The hidden_proto/hidden_def declarations are for libselinux functions
> that are called by libselinux itself, to provide an internal symbol for
> it and thereby avoid the cost and confusion of supporting libselinux
> using some other .so's definition of it.  So unless you put a call to it
> somewhere inside libselinux, you don't need this.
>

Ok, I'll drop it.

> > diff --git a/libselinux/src/selinux_internal.h b/libselinux/src/selinux_internal.h
> > index 8b4bed2f..7ca1c329 100644
> > --- a/libselinux/src/selinux_internal.h
> > +++ b/libselinux/src/selinux_internal.h
> > @@ -9,6 +9,7 @@ hidden_proto(selinux_mkload_policy)
> >       hidden_proto(security_disable)
> >       hidden_proto(security_policyvers)
> >       hidden_proto(security_load_policy)
> > +    hidden_proto(security_is_policy_capability_enabled)
>
> Ditto
>
> >       hidden_proto(security_get_boolean_active)
> >       hidden_proto(security_get_boolean_names)
> >       hidden_proto(security_set_boolean)
> > diff --git a/libselinux/src/selinuxswig_python_exception.i b/libselinux/src/selinuxswig_python_exception.i
> > index cf658259..bd107295 100644
> > --- a/libselinux/src/selinuxswig_python_exception.i
> > +++ b/libselinux/src/selinuxswig_python_exception.i
> > @@ -665,6 +665,15 @@
> >   }
> >
> >
> > +%exception security_is_policy_capability_enabled {
> > +  $action
> > +  if (result < 0) {
> > +     PyErr_SetFromErrno(PyExc_OSError);
> > +     SWIG_fail;
> > +  }
> > +}
> > +
> > +
> >   %exception security_get_boolean_names {
> >     $action
> >     if (result < 0) {
> >
>