Re: [PATCH] nscd: Make SELinux checks dynamic.

["Carlos O'Donell" <carlos at redhat dot com>]

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.