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

[Will Newton <will dot newton at linaro dot org>]

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.



-- 
Will Newton
Toolchain Working Group, Linaro