| Message ID | 53471B6A.1060700@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10 April 2014 23:30, Carlos O'Donell <carlos@redhat.com> wrote: > The SELinux team has indicated to me that glibc's SELinux checks > in nscd are not being carried out as they would expect the API > to be used today. They would like to move away from static header > defines for class and permissions and instead use dynamic checks > at runtime that provide an answer which is dependent on the runtime > status of SELinux i.e. more dynamic. > > The following patch is a minimal change that moves us forward in > this direction. > > It does the following: > > * Stop checking for SELinux headers that define NSCD__SHMEMHOST. > Check only for the presence or absence of the library. > > * Don't encode the specific SELinux permission constants into a > table at build time, and instead use the symbolic name for the > permission as expected. > > * Lookup the "What do we do if we don't know this permission?" > policy and use that if we find SELinux's policy is older than > the glibc policy e.g. we make a request for a permission that > SELinux doesn't know about. > > * Lastly, translate the class and permission and then make > the permission check. This is done every time we lookup > a permission, and this is the expected way to use the API. > SELinux will optimize this for us, and we expect the network > latencies to hide these extra library calls. > > Tested on x86, x86-64, and via Fedora Rawhide since November 2013. > > As an added benefit we remove two #ifdef's :-) > > If nobody objects I'll check this in after a couple days. > > Cheers, > Carlos. > > 2014-04-10 Carlos O'Donell <carlos@redhat.com> > > * configure.ac: Remove SELinux header check. > * configure: Regenerate. > * nscd/selinux.c (perms): Array of const char* to permission names. > (nscd_request_avc_has_perm): Call security_deny_unknown to find > default policy. Call string_to_security_class and string_to_av_perm to > translate strings. Enforce default policy and call avs_has_perm with > results of translated strings. > > diff --git a/configure b/configure > index abefcdb..8b0b222 100755 > --- a/configure > +++ b/configure > @@ -6938,38 +6938,9 @@ else > have_selinux=no > fi > > - # See if we have the SELinux header with the NSCD permissions in it. > - if test x$have_selinux = xyes ; then > - { $as_echo "$as_me:${as_lineno-$LINENO}: checking for NSCD Flask permissions in selinux/av_permissions.h" >&5 > -$as_echo_n "checking for NSCD Flask permissions in selinux/av_permissions.h... " >&6; } > - cat confdefs.h - <<_ACEOF >conftest.$ac_ext > -/* end confdefs.h. */ > -#include <selinux/av_permissions.h> > -int > -main () > -{ > -#ifdef NSCD__SHMEMHOST > - return 0; > - #else > - #error NSCD__SHMEMHOST not defined > - #endif > - ; > - return 0; > -} > -_ACEOF > -if ac_fn_c_try_compile "$LINENO"; then : > - have_selinux=yes > -else > - have_selinux=no > -fi > -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext > - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $have_selinux" >&5 > -$as_echo "$have_selinux" >&6; } > - fi > - > if test x$with_selinux = xyes ; then > if test x$have_selinux = xno ; then > - as_fn_error $? "SELinux explicitly required, but sufficiently recent SELinux library not found" "$LINENO" 5 > + as_fn_error $? "SELinux explicitly required, but SELinux library not found" "$LINENO" 5 > fi > fi > fi > diff --git a/configure.ac b/configure.ac > index 6291872..97a9591 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -1945,22 +1945,9 @@ else > # See if we have the SELinux library > AC_CHECK_LIB(selinux, is_selinux_enabled, > have_selinux=yes, have_selinux=no) > - # See if we have the SELinux header with the NSCD permissions in it. > - if test x$have_selinux = xyes ; then > - AC_MSG_CHECKING([for NSCD Flask permissions in selinux/av_permissions.h]) > - AC_TRY_COMPILE([#include <selinux/av_permissions.h>], > - [#ifdef NSCD__SHMEMHOST > - return 0; > - #else > - #error NSCD__SHMEMHOST not defined > - #endif], > - have_selinux=yes, have_selinux=no) > - AC_MSG_RESULT($have_selinux) > - fi > - > if test x$with_selinux = xyes ; then > if test x$have_selinux = xno ; then > - AC_MSG_ERROR([SELinux explicitly required, but sufficiently recent SELinux library not found]) > + AC_MSG_ERROR([SELinux explicitly required, but SELinux library not found]) > fi > fi > fi > diff --git a/nscd/selinux.c b/nscd/selinux.c > index 46b0ea9..2f143a5 100644 > --- a/nscd/selinux.c > +++ b/nscd/selinux.c Do we still need to include av_permissions.h after this change? > @@ -44,35 +44,31 @@ > /* Global variable to tell if the kernel has SELinux support. */ > int selinux_enabled; > > -/* Define mappings of access vector permissions to request types. */ > -static const access_vector_t perms[LASTREQ] = > +/* Define mappings of request type to AVC permission name. */ > +static const char *perms[LASTREQ] = > { > - [GETPWBYNAME] = NSCD__GETPWD, > - [GETPWBYUID] = NSCD__GETPWD, > - [GETGRBYNAME] = NSCD__GETGRP, > - [GETGRBYGID] = NSCD__GETGRP, > - [GETHOSTBYNAME] = NSCD__GETHOST, > - [GETHOSTBYNAMEv6] = NSCD__GETHOST, > - [GETHOSTBYADDR] = NSCD__GETHOST, > - [GETHOSTBYADDRv6] = NSCD__GETHOST, > - [GETSTAT] = NSCD__GETSTAT, > - [SHUTDOWN] = NSCD__ADMIN, > - [INVALIDATE] = NSCD__ADMIN, > - [GETFDPW] = NSCD__SHMEMPWD, > - [GETFDGR] = NSCD__SHMEMGRP, > - [GETFDHST] = NSCD__SHMEMHOST, > - [GETAI] = NSCD__GETHOST, > - [INITGROUPS] = NSCD__GETGRP, > -#ifdef NSCD__GETSERV > - [GETSERVBYNAME] = NSCD__GETSERV, > - [GETSERVBYPORT] = NSCD__GETSERV, > - [GETFDSERV] = NSCD__SHMEMSERV, > -#endif > -#ifdef NSCD__GETNETGRP > - [GETNETGRENT] = NSCD__GETNETGRP, > - [INNETGR] = NSCD__GETNETGRP, > - [GETFDNETGR] = NSCD__SHMEMNETGRP, > -#endif > + [GETPWBYNAME] = "getpwd", > + [GETPWBYUID] = "getpwd", > + [GETGRBYNAME] = "getgrp", > + [GETGRBYGID] = "getgrp", > + [GETHOSTBYNAME] = "gethost", > + [GETHOSTBYNAMEv6] = "gethost", > + [GETHOSTBYADDR] = "gethost", > + [GETHOSTBYADDRv6] = "gethost", > + [SHUTDOWN] = "admin", > + [GETSTAT] = "getstat", > + [INVALIDATE] = "admin", > + [GETFDPW] = "shmempwd", > + [GETFDGR] = "shmemgrp", > + [GETFDHST] = "shmemhost", > + [GETAI] = "gethost", > + [INITGROUPS] = "getgrp", > + [GETSERVBYNAME] = "getserv", > + [GETSERVBYPORT] = "getserv", > + [GETFDSERV] = "shmemserv", > + [GETNETGRENT] = "getnetgrp", > + [INNETGR] = "getnetgrp", > + [GETFDNETGR] = "shmemnetgrp", > }; > > /* Store an entry ref to speed AVC decisions. */ > @@ -344,7 +340,16 @@ nscd_avc_init (void) > > > /* Check the permission from the caller (via getpeercon) to nscd. > - Returns 0 if access is allowed, 1 if denied, and -1 on error. */ > + Returns 0 if access is allowed, 1 if denied, and -1 on error. > + > + The SELinux policy, enablement, and permission bits are all dynamic and the > + caching done by glibc is not entirely correct. This nscd support should be > + rewritten to use selinux_check_permission. A rewrite is risky though and > + requires some refactoring. Currently we use symbolic mappings instead of > + compile time constants (which selinux upstream says are going away), and we Other references seem to use "SELinux". > + use security_deny_unknown to determine what to do if selinux-policy doesn't > + have a definition for the the permission or object class we are looking > + up. */ > int > nscd_request_avc_has_perm (int fd, request_type req) > { > @@ -354,6 +359,33 @@ nscd_request_avc_has_perm (int fd, request_type req) > security_id_t ssid = NULL; > security_id_t tsid = NULL; > int rc = -1; > + security_class_t sc_nscd = 0; > + access_vector_t perm = 0; > + int avc_deny_unknown = 0; These initializations are not strictly required. > + > + /* Check if SELinux denys or allows unknown object classes > + and permissions. It is 0 if they are allowed, 1 if they > + are not allowed and -1 on error. */ > + if ((avc_deny_unknown = security_deny_unknown ()) == -1) > + dbg_log (_("Error querying policy for undefined object classes " > + "or permissions.")); > + > + /* Get the security class for nscd. If this fails we will likely be > + unable to do anything unless avc_deny_unknown is 0. */ > + if ((sc_nscd = string_to_security_class ("nscd")) == 0 I don't know if there is anything in the GNU coding style on this but it I think it looks neater to move the assignment out of the conditional like "perm" below. > + && avc_deny_unknown == 1) > + dbg_log (_("Error getting security class for nscd.")); > + > + /* Convert permission to AVC bits. */ > + perm = string_to_av_perm (sc_nscd, perms[req]); > + if (perm == 0 && avc_deny_unknown == 1) > + dbg_log (_("Error translating permission name " > + "\"%s\" to access vector bit."), perms[req]); > + > + /* If the nscd security class was not found or perms were not > + found and AVC does not deny unknown values then allow it. */ > + if ((sc_nscd == 0 || perm == 0) && avc_deny_unknown == 0) > + return 0; > > if (getpeercon (fd, &scon) < 0) > { > @@ -372,15 +404,7 @@ nscd_request_avc_has_perm (int fd, request_type req) > goto out; > } > > -#ifndef NSCD__GETSERV > - if (perms[req] == 0) > - { > - dbg_log (_("compile-time support for database policy missing")); > - goto out; > - } > -#endif > - > - rc = avc_has_perm (ssid, tsid, SECCLASS_NSCD, perms[req], &aeref, NULL) < 0; > + rc = avc_has_perm (ssid, tsid, sc_nscd, perm, &aeref, NULL) < 0; This treats all errors as access denied, but the code already does that so not a problem (indeed the only caller only checks for success). > > out: > if (scon) > --- > > Cheers, > Carlos.
On 04/11/2014 05:13 AM, Will Newton wrote: > On 10 April 2014 23:30, Carlos O'Donell <carlos@redhat.com> wrote: >> The SELinux team has indicated to me that glibc's SELinux checks >> in nscd are not being carried out as they would expect the API >> to be used today. They would like to move away from static header >> defines for class and permissions and instead use dynamic checks >> at runtime that provide an answer which is dependent on the runtime >> status of SELinux i.e. more dynamic. >> >> The following patch is a minimal change that moves us forward in >> this direction. >> >> It does the following: >> >> * Stop checking for SELinux headers that define NSCD__SHMEMHOST. >> Check only for the presence or absence of the library. >> >> * Don't encode the specific SELinux permission constants into a >> table at build time, and instead use the symbolic name for the >> permission as expected. >> >> * Lookup the "What do we do if we don't know this permission?" >> policy and use that if we find SELinux's policy is older than >> the glibc policy e.g. we make a request for a permission that >> SELinux doesn't know about. >> >> * Lastly, translate the class and permission and then make >> the permission check. This is done every time we lookup >> a permission, and this is the expected way to use the API. >> SELinux will optimize this for us, and we expect the network >> latencies to hide these extra library calls. >> >> Tested on x86, x86-64, and via Fedora Rawhide since November 2013. >> >> As an added benefit we remove two #ifdef's :-) >> >> If nobody objects I'll check this in after a couple days. >> >> Cheers, >> Carlos. >> >> 2014-04-10 Carlos O'Donell <carlos@redhat.com> >> >> * configure.ac: Remove SELinux header check. >> * configure: Regenerate. >> * nscd/selinux.c (perms): Array of const char* to permission names. >> (nscd_request_avc_has_perm): Call security_deny_unknown to find >> default policy. Call string_to_security_class and string_to_av_perm to >> translate strings. Enforce default policy and call avs_has_perm with >> results of translated strings. >> >> diff --git a/configure b/configure >> index abefcdb..8b0b222 100755 >> --- a/configure >> +++ b/configure >> @@ -6938,38 +6938,9 @@ else >> have_selinux=no >> fi >> >> - # See if we have the SELinux header with the NSCD permissions in it. >> - if test x$have_selinux = xyes ; then >> - { $as_echo "$as_me:${as_lineno-$LINENO}: checking for NSCD Flask permissions in selinux/av_permissions.h" >&5 >> -$as_echo_n "checking for NSCD Flask permissions in selinux/av_permissions.h... " >&6; } >> - cat confdefs.h - <<_ACEOF >conftest.$ac_ext >> -/* end confdefs.h. */ >> -#include <selinux/av_permissions.h> >> -int >> -main () >> -{ >> -#ifdef NSCD__SHMEMHOST >> - return 0; >> - #else >> - #error NSCD__SHMEMHOST not defined >> - #endif >> - ; >> - return 0; >> -} >> -_ACEOF >> -if ac_fn_c_try_compile "$LINENO"; then : >> - have_selinux=yes >> -else >> - have_selinux=no >> -fi >> -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext >> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $have_selinux" >&5 >> -$as_echo "$have_selinux" >&6; } >> - fi >> - >> if test x$with_selinux = xyes ; then >> if test x$have_selinux = xno ; then >> - as_fn_error $? "SELinux explicitly required, but sufficiently recent SELinux library not found" "$LINENO" 5 >> + as_fn_error $? "SELinux explicitly required, but SELinux library not found" "$LINENO" 5 >> fi >> fi >> fi >> diff --git a/configure.ac b/configure.ac >> index 6291872..97a9591 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -1945,22 +1945,9 @@ else >> # See if we have the SELinux library >> AC_CHECK_LIB(selinux, is_selinux_enabled, >> have_selinux=yes, have_selinux=no) >> - # See if we have the SELinux header with the NSCD permissions in it. >> - if test x$have_selinux = xyes ; then >> - AC_MSG_CHECKING([for NSCD Flask permissions in selinux/av_permissions.h]) >> - AC_TRY_COMPILE([#include <selinux/av_permissions.h>], >> - [#ifdef NSCD__SHMEMHOST >> - return 0; >> - #else >> - #error NSCD__SHMEMHOST not defined >> - #endif], >> - have_selinux=yes, have_selinux=no) >> - AC_MSG_RESULT($have_selinux) >> - fi >> - >> if test x$with_selinux = xyes ; then >> if test x$have_selinux = xno ; then >> - AC_MSG_ERROR([SELinux explicitly required, but sufficiently recent SELinux library not found]) >> + AC_MSG_ERROR([SELinux explicitly required, but SELinux library not found]) >> fi >> fi >> fi >> diff --git a/nscd/selinux.c b/nscd/selinux.c >> index 46b0ea9..2f143a5 100644 >> --- a/nscd/selinux.c >> +++ b/nscd/selinux.c > > Do we still need to include av_permissions.h after this change? Fixed. Good catch. We do not. >> @@ -44,35 +44,31 @@ >> /* Global variable to tell if the kernel has SELinux support. */ >> int selinux_enabled; >> >> -/* Define mappings of access vector permissions to request types. */ >> -static const access_vector_t perms[LASTREQ] = >> +/* Define mappings of request type to AVC permission name. */ >> +static const char *perms[LASTREQ] = >> { >> - [GETPWBYNAME] = NSCD__GETPWD, >> - [GETPWBYUID] = NSCD__GETPWD, >> - [GETGRBYNAME] = NSCD__GETGRP, >> - [GETGRBYGID] = NSCD__GETGRP, >> - [GETHOSTBYNAME] = NSCD__GETHOST, >> - [GETHOSTBYNAMEv6] = NSCD__GETHOST, >> - [GETHOSTBYADDR] = NSCD__GETHOST, >> - [GETHOSTBYADDRv6] = NSCD__GETHOST, >> - [GETSTAT] = NSCD__GETSTAT, >> - [SHUTDOWN] = NSCD__ADMIN, >> - [INVALIDATE] = NSCD__ADMIN, >> - [GETFDPW] = NSCD__SHMEMPWD, >> - [GETFDGR] = NSCD__SHMEMGRP, >> - [GETFDHST] = NSCD__SHMEMHOST, >> - [GETAI] = NSCD__GETHOST, >> - [INITGROUPS] = NSCD__GETGRP, >> -#ifdef NSCD__GETSERV >> - [GETSERVBYNAME] = NSCD__GETSERV, >> - [GETSERVBYPORT] = NSCD__GETSERV, >> - [GETFDSERV] = NSCD__SHMEMSERV, >> -#endif >> -#ifdef NSCD__GETNETGRP >> - [GETNETGRENT] = NSCD__GETNETGRP, >> - [INNETGR] = NSCD__GETNETGRP, >> - [GETFDNETGR] = NSCD__SHMEMNETGRP, >> -#endif >> + [GETPWBYNAME] = "getpwd", >> + [GETPWBYUID] = "getpwd", >> + [GETGRBYNAME] = "getgrp", >> + [GETGRBYGID] = "getgrp", >> + [GETHOSTBYNAME] = "gethost", >> + [GETHOSTBYNAMEv6] = "gethost", >> + [GETHOSTBYADDR] = "gethost", >> + [GETHOSTBYADDRv6] = "gethost", >> + [SHUTDOWN] = "admin", >> + [GETSTAT] = "getstat", >> + [INVALIDATE] = "admin", >> + [GETFDPW] = "shmempwd", >> + [GETFDGR] = "shmemgrp", >> + [GETFDHST] = "shmemhost", >> + [GETAI] = "gethost", >> + [INITGROUPS] = "getgrp", >> + [GETSERVBYNAME] = "getserv", >> + [GETSERVBYPORT] = "getserv", >> + [GETFDSERV] = "shmemserv", >> + [GETNETGRENT] = "getnetgrp", >> + [INNETGR] = "getnetgrp", >> + [GETFDNETGR] = "shmemnetgrp", >> }; >> >> /* Store an entry ref to speed AVC decisions. */ >> @@ -344,7 +340,16 @@ nscd_avc_init (void) >> >> >> /* Check the permission from the caller (via getpeercon) to nscd. >> - Returns 0 if access is allowed, 1 if denied, and -1 on error. */ >> + Returns 0 if access is allowed, 1 if denied, and -1 on error. >> + >> + The SELinux policy, enablement, and permission bits are all dynamic and the >> + caching done by glibc is not entirely correct. This nscd support should be >> + rewritten to use selinux_check_permission. A rewrite is risky though and >> + requires some refactoring. Currently we use symbolic mappings instead of >> + compile time constants (which selinux upstream says are going away), and we > > Other references seem to use "SELinux". Fixed. It should be SELinux. >> + use security_deny_unknown to determine what to do if selinux-policy doesn't Fixed. It should be selinux-policy* (there are several policy packages). >> + have a definition for the the permission or object class we are looking >> + up. */ >> int >> nscd_request_avc_has_perm (int fd, request_type req) >> { >> @@ -354,6 +359,33 @@ nscd_request_avc_has_perm (int fd, request_type req) >> security_id_t ssid = NULL; >> security_id_t tsid = NULL; >> int rc = -1; >> + security_class_t sc_nscd = 0; >> + access_vector_t perm = 0; >> + int avc_deny_unknown = 0; > > These initializations are not strictly required. Removed. You're right in that they are all assigned before use. >> + >> + /* Check if SELinux denys or allows unknown object classes >> + and permissions. It is 0 if they are allowed, 1 if they >> + are not allowed and -1 on error. */ >> + if ((avc_deny_unknown = security_deny_unknown ()) == -1) >> + dbg_log (_("Error querying policy for undefined object classes " >> + "or permissions.")); >> + >> + /* Get the security class for nscd. If this fails we will likely be >> + unable to do anything unless avc_deny_unknown is 0. */ >> + if ((sc_nscd = string_to_security_class ("nscd")) == 0 > > I don't know if there is anything in the GNU coding style on this but > it I think it looks neater to move the assignment out of the > conditional like "perm" below. Fixed. I agree it's easier to read. Nothing explicitly in GNU, but peer review like this helps keep things readable. Thanks again for the review! >> + && avc_deny_unknown == 1) >> + dbg_log (_("Error getting security class for nscd.")); >> + >> + /* Convert permission to AVC bits. */ >> + perm = string_to_av_perm (sc_nscd, perms[req]); >> + if (perm == 0 && avc_deny_unknown == 1) >> + dbg_log (_("Error translating permission name " >> + "\"%s\" to access vector bit."), perms[req]); >> + >> + /* If the nscd security class was not found or perms were not >> + found and AVC does not deny unknown values then allow it. */ >> + if ((sc_nscd == 0 || perm == 0) && avc_deny_unknown == 0) >> + return 0; >> >> if (getpeercon (fd, &scon) < 0) >> { >> @@ -372,15 +404,7 @@ nscd_request_avc_has_perm (int fd, request_type req) >> goto out; >> } >> >> -#ifndef NSCD__GETSERV >> - if (perms[req] == 0) >> - { >> - dbg_log (_("compile-time support for database policy missing")); >> - goto out; >> - } >> -#endif >> - >> - rc = avc_has_perm (ssid, tsid, SECCLASS_NSCD, perms[req], &aeref, NULL) < 0; >> + rc = avc_has_perm (ssid, tsid, sc_nscd, perm, &aeref, NULL) < 0; > > This treats all errors as access denied, but the code already does > that so not a problem (indeed the only caller only checks for > success). Fixed (added a comment to explain why we do this). /* The SELinux API for avc_has_perm conflates access denied and error into the return code -1, while nscd_request_avs_has_perm has distinct error (-1) and denied (1) return codes. We map the avc_has_perm access denied or error into an access denied at the nscd interface level (we do accurately report error for the getpeercon, getcon, and avc_context_to_sid interfaces used above). */ The problem is that nscd_request_avc_has_perm has a tristate return, with 0 being allowed, 1 being denied, and -1 being error. Unfortunately the avc_has_perm SELinux interface doesn't distinguish between error and denied returning -1 for both. The `< 0' does the mapping for us from `-1' denied or error from avc_has_perm to `1' from nscd_request_avc_has_perm. This is sufficiently complicated that I've added a comment there to explain why we do this. Cheers, Carlos.
On 04/11/2014 07:02 PM, Carlos O'Donell wrote: > On 04/11/2014 05:13 AM, Will Newton wrote: >> On 10 April 2014 23:30, Carlos O'Donell <carlos@redhat.com> wrote: >>> The SELinux team has indicated to me that glibc's SELinux checks >>> in nscd are not being carried out as they would expect the API >>> to be used today. They would like to move away from static header >>> defines for class and permissions and instead use dynamic checks >>> at runtime that provide an answer which is dependent on the runtime >>> status of SELinux i.e. more dynamic. >>> >>> The following patch is a minimal change that moves us forward in >>> this direction. >>> >>> It does the following: >>> >>> * Stop checking for SELinux headers that define NSCD__SHMEMHOST. >>> Check only for the presence or absence of the library. >>> >>> * Don't encode the specific SELinux permission constants into a >>> table at build time, and instead use the symbolic name for the >>> permission as expected. >>> >>> * Lookup the "What do we do if we don't know this permission?" >>> policy and use that if we find SELinux's policy is older than >>> the glibc policy e.g. we make a request for a permission that >>> SELinux doesn't know about. >>> >>> * Lastly, translate the class and permission and then make >>> the permission check. This is done every time we lookup >>> a permission, and this is the expected way to use the API. >>> SELinux will optimize this for us, and we expect the network >>> latencies to hide these extra library calls. >>> >>> Tested on x86, x86-64, and via Fedora Rawhide since November 2013. >>> >>> As an added benefit we remove two #ifdef's :-) >>> >>> If nobody objects I'll check this in after a couple days. >>> >>> Cheers, >>> Carlos. >>> >>> 2014-04-10 Carlos O'Donell <carlos@redhat.com> >>> >>> * configure.ac: Remove SELinux header check. >>> * configure: Regenerate. >>> * nscd/selinux.c (perms): Array of const char* to permission names. >>> (nscd_request_avc_has_perm): Call security_deny_unknown to find >>> default policy. Call string_to_security_class and string_to_av_perm to >>> translate strings. Enforce default policy and call avs_has_perm with >>> results of translated strings. >>> >>> diff --git a/configure b/configure >>> index abefcdb..8b0b222 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -6938,38 +6938,9 @@ else >>> have_selinux=no >>> fi >>> >>> - # See if we have the SELinux header with the NSCD permissions in it. >>> - if test x$have_selinux = xyes ; then >>> - { $as_echo "$as_me:${as_lineno-$LINENO}: checking for NSCD Flask permissions in selinux/av_permissions.h" >&5 >>> -$as_echo_n "checking for NSCD Flask permissions in selinux/av_permissions.h... " >&6; } >>> - cat confdefs.h - <<_ACEOF >conftest.$ac_ext >>> -/* end confdefs.h. */ >>> -#include <selinux/av_permissions.h> >>> -int >>> -main () >>> -{ >>> -#ifdef NSCD__SHMEMHOST >>> - return 0; >>> - #else >>> - #error NSCD__SHMEMHOST not defined >>> - #endif >>> - ; >>> - return 0; >>> -} >>> -_ACEOF >>> -if ac_fn_c_try_compile "$LINENO"; then : >>> - have_selinux=yes >>> -else >>> - have_selinux=no >>> -fi >>> -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext >>> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $have_selinux" >&5 >>> -$as_echo "$have_selinux" >&6; } >>> - fi >>> - >>> if test x$with_selinux = xyes ; then >>> if test x$have_selinux = xno ; then >>> - as_fn_error $? "SELinux explicitly required, but sufficiently recent SELinux library not found" "$LINENO" 5 >>> + as_fn_error $? "SELinux explicitly required, but SELinux library not found" "$LINENO" 5 >>> fi >>> fi >>> fi >>> diff --git a/configure.ac b/configure.ac >>> index 6291872..97a9591 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -1945,22 +1945,9 @@ else >>> # See if we have the SELinux library >>> AC_CHECK_LIB(selinux, is_selinux_enabled, >>> have_selinux=yes, have_selinux=no) >>> - # See if we have the SELinux header with the NSCD permissions in it. >>> - if test x$have_selinux = xyes ; then >>> - AC_MSG_CHECKING([for NSCD Flask permissions in selinux/av_permissions.h]) >>> - AC_TRY_COMPILE([#include <selinux/av_permissions.h>], >>> - [#ifdef NSCD__SHMEMHOST >>> - return 0; >>> - #else >>> - #error NSCD__SHMEMHOST not defined >>> - #endif], >>> - have_selinux=yes, have_selinux=no) >>> - AC_MSG_RESULT($have_selinux) >>> - fi >>> - >>> if test x$with_selinux = xyes ; then >>> if test x$have_selinux = xno ; then >>> - AC_MSG_ERROR([SELinux explicitly required, but sufficiently recent SELinux library not found]) >>> + AC_MSG_ERROR([SELinux explicitly required, but SELinux library not found]) >>> fi >>> fi >>> fi >>> diff --git a/nscd/selinux.c b/nscd/selinux.c >>> index 46b0ea9..2f143a5 100644 >>> --- a/nscd/selinux.c >>> +++ b/nscd/selinux.c >> >> Do we still need to include av_permissions.h after this change? > > Fixed. Good catch. We do not. > >>> @@ -44,35 +44,31 @@ >>> /* Global variable to tell if the kernel has SELinux support. */ >>> int selinux_enabled; >>> >>> -/* Define mappings of access vector permissions to request types. */ >>> -static const access_vector_t perms[LASTREQ] = >>> +/* Define mappings of request type to AVC permission name. */ >>> +static const char *perms[LASTREQ] = >>> { >>> - [GETPWBYNAME] = NSCD__GETPWD, >>> - [GETPWBYUID] = NSCD__GETPWD, >>> - [GETGRBYNAME] = NSCD__GETGRP, >>> - [GETGRBYGID] = NSCD__GETGRP, >>> - [GETHOSTBYNAME] = NSCD__GETHOST, >>> - [GETHOSTBYNAMEv6] = NSCD__GETHOST, >>> - [GETHOSTBYADDR] = NSCD__GETHOST, >>> - [GETHOSTBYADDRv6] = NSCD__GETHOST, >>> - [GETSTAT] = NSCD__GETSTAT, >>> - [SHUTDOWN] = NSCD__ADMIN, >>> - [INVALIDATE] = NSCD__ADMIN, >>> - [GETFDPW] = NSCD__SHMEMPWD, >>> - [GETFDGR] = NSCD__SHMEMGRP, >>> - [GETFDHST] = NSCD__SHMEMHOST, >>> - [GETAI] = NSCD__GETHOST, >>> - [INITGROUPS] = NSCD__GETGRP, >>> -#ifdef NSCD__GETSERV >>> - [GETSERVBYNAME] = NSCD__GETSERV, >>> - [GETSERVBYPORT] = NSCD__GETSERV, >>> - [GETFDSERV] = NSCD__SHMEMSERV, >>> -#endif >>> -#ifdef NSCD__GETNETGRP >>> - [GETNETGRENT] = NSCD__GETNETGRP, >>> - [INNETGR] = NSCD__GETNETGRP, >>> - [GETFDNETGR] = NSCD__SHMEMNETGRP, >>> -#endif >>> + [GETPWBYNAME] = "getpwd", >>> + [GETPWBYUID] = "getpwd", >>> + [GETGRBYNAME] = "getgrp", >>> + [GETGRBYGID] = "getgrp", >>> + [GETHOSTBYNAME] = "gethost", >>> + [GETHOSTBYNAMEv6] = "gethost", >>> + [GETHOSTBYADDR] = "gethost", >>> + [GETHOSTBYADDRv6] = "gethost", >>> + [SHUTDOWN] = "admin", >>> + [GETSTAT] = "getstat", >>> + [INVALIDATE] = "admin", >>> + [GETFDPW] = "shmempwd", >>> + [GETFDGR] = "shmemgrp", >>> + [GETFDHST] = "shmemhost", >>> + [GETAI] = "gethost", >>> + [INITGROUPS] = "getgrp", >>> + [GETSERVBYNAME] = "getserv", >>> + [GETSERVBYPORT] = "getserv", >>> + [GETFDSERV] = "shmemserv", >>> + [GETNETGRENT] = "getnetgrp", >>> + [INNETGR] = "getnetgrp", >>> + [GETFDNETGR] = "shmemnetgrp", >>> }; >>> >>> /* Store an entry ref to speed AVC decisions. */ >>> @@ -344,7 +340,16 @@ nscd_avc_init (void) >>> >>> >>> /* Check the permission from the caller (via getpeercon) to nscd. >>> - Returns 0 if access is allowed, 1 if denied, and -1 on error. */ >>> + Returns 0 if access is allowed, 1 if denied, and -1 on error. >>> + >>> + The SELinux policy, enablement, and permission bits are all dynamic and the >>> + caching done by glibc is not entirely correct. This nscd support should be >>> + rewritten to use selinux_check_permission. A rewrite is risky though and >>> + requires some refactoring. Currently we use symbolic mappings instead of >>> + compile time constants (which selinux upstream says are going away), and we >> >> Other references seem to use "SELinux". > > Fixed. It should be SELinux. > >>> + use security_deny_unknown to determine what to do if selinux-policy doesn't > > Fixed. It should be selinux-policy* (there are several policy packages). > >>> + have a definition for the the permission or object class we are looking >>> + up. */ >>> int >>> nscd_request_avc_has_perm (int fd, request_type req) >>> { >>> @@ -354,6 +359,33 @@ nscd_request_avc_has_perm (int fd, request_type req) >>> security_id_t ssid = NULL; >>> security_id_t tsid = NULL; >>> int rc = -1; >>> + security_class_t sc_nscd = 0; >>> + access_vector_t perm = 0; >>> + int avc_deny_unknown = 0; >> >> These initializations are not strictly required. > > Removed. You're right in that they are all assigned before use. > >>> + >>> + /* Check if SELinux denys or allows unknown object classes >>> + and permissions. It is 0 if they are allowed, 1 if they >>> + are not allowed and -1 on error. */ >>> + if ((avc_deny_unknown = security_deny_unknown ()) == -1) >>> + dbg_log (_("Error querying policy for undefined object classes " >>> + "or permissions.")); >>> + >>> + /* Get the security class for nscd. If this fails we will likely be >>> + unable to do anything unless avc_deny_unknown is 0. */ >>> + if ((sc_nscd = string_to_security_class ("nscd")) == 0 >> >> I don't know if there is anything in the GNU coding style on this but >> it I think it looks neater to move the assignment out of the >> conditional like "perm" below. > > Fixed. I agree it's easier to read. Nothing explicitly in GNU, but > peer review like this helps keep things readable. Thanks again for the > review! > >>> + && avc_deny_unknown == 1) >>> + dbg_log (_("Error getting security class for nscd.")); >>> + >>> + /* Convert permission to AVC bits. */ >>> + perm = string_to_av_perm (sc_nscd, perms[req]); >>> + if (perm == 0 && avc_deny_unknown == 1) >>> + dbg_log (_("Error translating permission name " >>> + "\"%s\" to access vector bit."), perms[req]); >>> + >>> + /* If the nscd security class was not found or perms were not >>> + found and AVC does not deny unknown values then allow it. */ >>> + if ((sc_nscd == 0 || perm == 0) && avc_deny_unknown == 0) >>> + return 0; >>> >>> if (getpeercon (fd, &scon) < 0) >>> { >>> @@ -372,15 +404,7 @@ nscd_request_avc_has_perm (int fd, request_type req) >>> goto out; >>> } >>> >>> -#ifndef NSCD__GETSERV >>> - if (perms[req] == 0) >>> - { >>> - dbg_log (_("compile-time support for database policy missing")); >>> - goto out; >>> - } >>> -#endif >>> - >>> - rc = avc_has_perm (ssid, tsid, SECCLASS_NSCD, perms[req], &aeref, NULL) < 0; >>> + rc = avc_has_perm (ssid, tsid, sc_nscd, perm, &aeref, NULL) < 0; >> >> This treats all errors as access denied, but the code already does >> that so not a problem (indeed the only caller only checks for >> success). > > Fixed (added a comment to explain why we do this). > > /* The SELinux API for avc_has_perm conflates access denied and error into > the return code -1, while nscd_request_avs_has_perm has distinct error > (-1) and denied (1) return codes. We map the avc_has_perm access denied or > error into an access denied at the nscd interface level (we do accurately > report error for the getpeercon, getcon, and avc_context_to_sid interfaces > used above). */ > > The problem is that nscd_request_avc_has_perm has a tristate return, > with 0 being allowed, 1 being denied, and -1 being error. > > Unfortunately the avc_has_perm SELinux interface doesn't distinguish > between error and denied returning -1 for both. > > The `< 0' does the mapping for us from `-1' denied or error from > avc_has_perm to `1' from nscd_request_avc_has_perm. > > This is sufficiently complicated that I've added a comment there to explain > why we do this. Checked in v2 with all of the above fixes. commit 0699f766b10c86912b75f35bef697106b70c1cf6 Author: Carlos O'Donell <carlos@redhat.com> Date: Thu Apr 10 18:31:53 2014 -0400 nscd: Make SELinux checks dynamic. The SELinux team has indicated to me that glibc's SELinux checks in nscd are not being carried out as they would expect the API to be used today. They would like to move away from static header defines for class and permissions and instead use dynamic checks at runtime that provide an answer which is dependent on the runtime status of SELinux i.e. more dynamic. The following patch is a minimal change that moves us forward in this direction. It does the following: * Stop checking for SELinux headers that define NSCD__SHMEMHOST. Check only for the presence or absence of the library. * Don't encode the specific SELinux permission constants into a table at build time, and instead use the symbolic name for the permission as expected. * Lookup the "What do we do if we don't know this permission?" policy and use that if we find SELinux's policy is older than the glibc policy e.g. we make a request for a permission that SELinux doesn't know about. * Lastly, translate the class and permission and then make the permission check. This is done every time we lookup a permission, and this is the expected way to use the API. SELinux will optimize this for us, and we expect the network latencies to hide these extra library calls. Tested on x86, x86-64, and via Fedora Rawhide since November 2013. See: https://sourceware.org/ml/libc-alpha/2014-04/msg00179.html Cheers, Carlos.
diff --git a/configure b/configure index abefcdb..8b0b222 100755 --- a/configure +++ b/configure @@ -6938,38 +6938,9 @@ else have_selinux=no fi - # See if we have the SELinux header with the NSCD permissions in it. - if test x$have_selinux = xyes ; then - { $as_echo "$as_me:${as_lineno-$LINENO}: checking for NSCD Flask permissions in selinux/av_permissions.h" >&5 -$as_echo_n "checking for NSCD Flask permissions in selinux/av_permissions.h... " >&6; } - cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ -#include <selinux/av_permissions.h> -int -main () -{ -#ifdef NSCD__SHMEMHOST - return 0; - #else - #error NSCD__SHMEMHOST not defined - #endif - ; - return 0; -} -_ACEOF -if ac_fn_c_try_compile "$LINENO"; then : - have_selinux=yes -else - have_selinux=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $have_selinux" >&5 -$as_echo "$have_selinux" >&6; } - fi - if test x$with_selinux = xyes ; then if test x$have_selinux = xno ; then - as_fn_error $? "SELinux explicitly required, but sufficiently recent SELinux library not found" "$LINENO" 5 + as_fn_error $? "SELinux explicitly required, but SELinux library not found" "$LINENO" 5 fi fi fi diff --git a/configure.ac b/configure.ac index 6291872..97a9591 100644 --- a/configure.ac +++ b/configure.ac @@ -1945,22 +1945,9 @@ else # See if we have the SELinux library AC_CHECK_LIB(selinux, is_selinux_enabled, have_selinux=yes, have_selinux=no) - # See if we have the SELinux header with the NSCD permissions in it. - if test x$have_selinux = xyes ; then - AC_MSG_CHECKING([for NSCD Flask permissions in selinux/av_permissions.h]) - AC_TRY_COMPILE([#include <selinux/av_permissions.h>], - [#ifdef NSCD__SHMEMHOST - return 0; - #else - #error NSCD__SHMEMHOST not defined - #endif], - have_selinux=yes, have_selinux=no) - AC_MSG_RESULT($have_selinux) - fi - if test x$with_selinux = xyes ; then if test x$have_selinux = xno ; then - AC_MSG_ERROR([SELinux explicitly required, but sufficiently recent SELinux library not found]) + AC_MSG_ERROR([SELinux explicitly required, but SELinux library not found]) fi fi fi diff --git a/nscd/selinux.c b/nscd/selinux.c index 46b0ea9..2f143a5 100644 --- a/nscd/selinux.c +++ b/nscd/selinux.c @@ -44,35 +44,31 @@ /* Global variable to tell if the kernel has SELinux support. */ int selinux_enabled; -/* Define mappings of access vector permissions to request types. */ -static const access_vector_t perms[LASTREQ] = +/* Define mappings of request type to AVC permission name. */ +static const char *perms[LASTREQ] = { - [GETPWBYNAME] = NSCD__GETPWD, - [GETPWBYUID] = NSCD__GETPWD, - [GETGRBYNAME] = NSCD__GETGRP, - [GETGRBYGID] = NSCD__GETGRP, - [GETHOSTBYNAME] = NSCD__GETHOST, - [GETHOSTBYNAMEv6] = NSCD__GETHOST, - [GETHOSTBYADDR] = NSCD__GETHOST, - [GETHOSTBYADDRv6] = NSCD__GETHOST, - [GETSTAT] = NSCD__GETSTAT, - [SHUTDOWN] = NSCD__ADMIN, - [INVALIDATE] = NSCD__ADMIN, - [GETFDPW] = NSCD__SHMEMPWD, - [GETFDGR] = NSCD__SHMEMGRP, - [GETFDHST] = NSCD__SHMEMHOST, - [GETAI] = NSCD__GETHOST, - [INITGROUPS] = NSCD__GETGRP, -#ifdef NSCD__GETSERV - [GETSERVBYNAME] = NSCD__GETSERV, - [GETSERVBYPORT] = NSCD__GETSERV, - [GETFDSERV] = NSCD__SHMEMSERV, -#endif -#ifdef NSCD__GETNETGRP - [GETNETGRENT] = NSCD__GETNETGRP, - [INNETGR] = NSCD__GETNETGRP, - [GETFDNETGR] = NSCD__SHMEMNETGRP, -#endif + [GETPWBYNAME] = "getpwd", + [GETPWBYUID] = "getpwd", + [GETGRBYNAME] = "getgrp", + [GETGRBYGID] = "getgrp", + [GETHOSTBYNAME] = "gethost", + [GETHOSTBYNAMEv6] = "gethost", + [GETHOSTBYADDR] = "gethost", + [GETHOSTBYADDRv6] = "gethost", + [SHUTDOWN] = "admin", + [GETSTAT] = "getstat", + [INVALIDATE] = "admin", + [GETFDPW] = "shmempwd", + [GETFDGR] = "shmemgrp", + [GETFDHST] = "shmemhost", + [GETAI] = "gethost", + [INITGROUPS] = "getgrp", + [GETSERVBYNAME] = "getserv", + [GETSERVBYPORT] = "getserv", + [GETFDSERV] = "shmemserv", + [GETNETGRENT] = "getnetgrp", + [INNETGR] = "getnetgrp", + [GETFDNETGR] = "shmemnetgrp", }; /* Store an entry ref to speed AVC decisions. */ @@ -344,7 +340,16 @@ nscd_avc_init (void) /* Check the permission from the caller (via getpeercon) to nscd. - Returns 0 if access is allowed, 1 if denied, and -1 on error. */ + Returns 0 if access is allowed, 1 if denied, and -1 on error. + + The SELinux policy, enablement, and permission bits are all dynamic and the + caching done by glibc is not entirely correct. This nscd support should be + rewritten to use selinux_check_permission. A rewrite is risky though and + requires some refactoring. Currently we use symbolic mappings instead of + compile time constants (which selinux upstream says are going away), and we + use security_deny_unknown to determine what to do if selinux-policy doesn't + have a definition for the the permission or object class we are looking + up. */ int nscd_request_avc_has_perm (int fd, request_type req) { @@ -354,6 +359,33 @@ nscd_request_avc_has_perm (int fd, request_type req) security_id_t ssid = NULL; security_id_t tsid = NULL; int rc = -1; + security_class_t sc_nscd = 0; + access_vector_t perm = 0; + int avc_deny_unknown = 0; + + /* Check if SELinux denys or allows unknown object classes + and permissions. It is 0 if they are allowed, 1 if they + are not allowed and -1 on error. */ + if ((avc_deny_unknown = security_deny_unknown ()) == -1) + dbg_log (_("Error querying policy for undefined object classes " + "or permissions.")); + + /* Get the security class for nscd. If this fails we will likely be + unable to do anything unless avc_deny_unknown is 0. */ + if ((sc_nscd = string_to_security_class ("nscd")) == 0 + && avc_deny_unknown == 1) + dbg_log (_("Error getting security class for nscd.")); + + /* Convert permission to AVC bits. */ + perm = string_to_av_perm (sc_nscd, perms[req]); + if (perm == 0 && avc_deny_unknown == 1) + dbg_log (_("Error translating permission name " + "\"%s\" to access vector bit."), perms[req]); + + /* If the nscd security class was not found or perms were not + found and AVC does not deny unknown values then allow it. */ + if ((sc_nscd == 0 || perm == 0) && avc_deny_unknown == 0) + return 0; if (getpeercon (fd, &scon) < 0) { @@ -372,15 +404,7 @@ nscd_request_avc_has_perm (int fd, request_type req) goto out; } -#ifndef NSCD__GETSERV - if (perms[req] == 0) - { - dbg_log (_("compile-time support for database policy missing")); - goto out; - } -#endif - - rc = avc_has_perm (ssid, tsid, SECCLASS_NSCD, perms[req], &aeref, NULL) < 0; + rc = avc_has_perm (ssid, tsid, sc_nscd, perm, &aeref, NULL) < 0; out: if (scon)
The SELinux team has indicated to me that glibc's SELinux checks in nscd are not being carried out as they would expect the API to be used today. They would like to move away from static header defines for class and permissions and instead use dynamic checks at runtime that provide an answer which is dependent on the runtime status of SELinux i.e. more dynamic. The following patch is a minimal change that moves us forward in this direction. It does the following: * Stop checking for SELinux headers that define NSCD__SHMEMHOST. Check only for the presence or absence of the library. * Don't encode the specific SELinux permission constants into a table at build time, and instead use the symbolic name for the permission as expected. * Lookup the "What do we do if we don't know this permission?" policy and use that if we find SELinux's policy is older than the glibc policy e.g. we make a request for a permission that SELinux doesn't know about. * Lastly, translate the class and permission and then make the permission check. This is done every time we lookup a permission, and this is the expected way to use the API. SELinux will optimize this for us, and we expect the network latencies to hide these extra library calls. Tested on x86, x86-64, and via Fedora Rawhide since November 2013. As an added benefit we remove two #ifdef's :-) If nobody objects I'll check this in after a couple days. Cheers, Carlos. 2014-04-10 Carlos O'Donell <carlos@redhat.com> * configure.ac: Remove SELinux header check. * configure: Regenerate. * nscd/selinux.c (perms): Array of const char* to permission names. (nscd_request_avc_has_perm): Call security_deny_unknown to find default policy. Call string_to_security_class and string_to_av_perm to translate strings. Enforce default policy and call avs_has_perm with results of translated strings. --- Cheers, Carlos.