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

[Stephen Smalley <sds@tycho.nsa.gov>]

On 1/10/20 9:43 AM, Christian Göttsche wrote:
> 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.
>>> ---

>>> 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

Not sure why though.  One difference is that sepol_polcap_getnum() is 
only used for capability names specified in policies, while 
security_is_policy_capability_enabled() will be used in application 
code.  It seems reasonable to require application writers to use the 
right case for the capability name?

> 
>>
>>> +
>>> +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)

No, we aren't crossing a trust boundary here and any sane application is 
going to hardcode the policy capability name, not take it from an 
untrusted source.