Re: [PATCH] id: show SMACK security context

[Jarkko Sakkinen]

Hi

And thanks for replying! 
(and sorry for double reply)

On Mon, Apr 22, 2013, at 13:15, Pádraig Brady wrote:
> On 04/17/2013 09:30 PM, Jarkko Sakkinen wrote:
> > Enable showing SMACK security context with -Z command-line switch.
> > Adds dependency to libsmack.
> > ---
> >  configure.ac |  5 +++++
> >  src/id.c     | 21 +++++++++++++++++----
> >  src/local.mk |  2 +-
> >  3 files changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 3f0c58b..e001bd8 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -276,6 +276,11 @@ if test $ac_cv_func_syslog = no; then
> >    done
> >  fi
> >  
> > +AC_ARG_WITH([smack], AS_HELP_STRING([--with-smack], [Build with SMACK]))
> > +if test "x$with_smack" = "xyes"; then
> > +   PKG_CHECK_MODULES([LIBSMACK], [libsmack], [AC_DEFINE([HAVE_SMACK], [1], 
> > [FIXME])])
> > +fi
> > +
> 
> I think it's best to not introduce a new dependency on pkg-config.
> pkg-config can simplify autoconf for the developer but pushes
> complexity to the user through env variables etc. so is best avoided.

What direction should I take on detecting dependency here? Should
I start looking into gnulib?

> 
> >  AC_CACHE_CHECK([for 3-argument setpriority function],
> >    [utils_cv_func_setpriority],
> >    [AC_LINK_IFELSE(
> > diff --git a/src/id.c b/src/id.c
> > index b5a7214..86b63b4 100644
> > --- a/src/id.c
> > +++ b/src/id.c
> > @@ -24,6 +24,7 @@
> >  #include <grp.h>
> >  #include <getopt.h>
> >  #include <selinux/selinux.h>
> > +#include <sys/smack.h>
> 
> You can't unconditionally include here.
> See the selinux-h module in gnulib that supports its unconditional
> include.

Oops, my bad. Will fix this.

> 
> >  
> >  #include "system.h"
> >  #include "error.h"
> > @@ -107,6 +108,9 @@ main (int argc, char **argv)
> >  {
> >    int optc;
> >    int selinux_enabled = (is_selinux_enabled () > 0);
> > +#ifdef HAVE_SMACK
> > +  int smack_enabled = (smack_smackfs_path () != NULL);
> > +#endif
> >  
> >    /* If true, output the list of all group IDs. -G */
> >    bool just_group_list = false;
> > @@ -134,10 +138,16 @@ main (int argc, char **argv)
> >            break;
> >  
> >          case 'Z':
> > -          /* politely decline if we're not on a selinux-enabled kernel. */
> > +          /* politely decline if we're not on a SELinux/SMACK-enabled 
> > kernel. */
> > +#ifdef HAVE_SMACK
> > +          if (!selinux_enabled && !smack_enabled)
> > +            error (EXIT_FAILURE, 0,
> > +                   _("--context (-Z) works only on an 
> > SELinux/SMACK-enabled kernel"));
> > +#else
> >            if (!selinux_enabled)
> >              error (EXIT_FAILURE, 0,
> >                     _("--context (-Z) works only on an SELinux-enabled 
> > kernel"));
> > +#endif
> >            just_context = 1;
> >            break;
> >  
> > @@ -189,14 +199,17 @@ main (int argc, char **argv)
> >       and we're not in POSIXLY_CORRECT mode, get our context.  Otherwise,
> >       leave the context variable alone - it has been initialized to an
> >       invalid value that will be not displayed in print_full_info().  */
> > -  if (selinux_enabled
> > -      && n_ids == 0
> > +  if (n_ids == 0
> >        && (just_context
> >            || (default_format && ! getenv ("POSIXLY_CORRECT"))))
> >      {
> >        /* Report failure only if --context (-Z) was explicitly requested.  
> > */
> > -      if (getcon (&context) && just_context)
> > +      if (selinux_enabled && getcon (&context) && just_context)
> > +        error (EXIT_FAILURE, 0, _("can't get process context"));
> > +#ifdef HAVE_SMACK
> > +      else if (smack_enabled && smack_new_label_from_self ((char **) 
> > &context))
> >          error (EXIT_FAILURE, 0, _("can't get process context"));
> > +#endif
> 
> So smack defers to SELinux.
> In that case you probably don't want --with-smack above,
> and instead auto detect smack availability.

Well, actually you couldn't have SELinux and SMACK active in the
kernel at the same time. Kernel can only have one LSM enabled at
a time (and you cannot switch or disable LSM). So this essentially
detects, which one is enabled in the kernel.

> 
> >      }
> >  
> >    if (n_ids == 1)
> > diff --git a/src/local.mk b/src/local.mk
> > index 1ae9eff..67d6693 100644
> > --- a/src/local.mk
> > +++ b/src/local.mk
> > @@ -227,7 +227,7 @@ src_test_LDADD += $(LIB_EACCESS)
> >  copy_ldadd += $(LIB_SELINUX)
> >  src_chcon_LDADD += $(LIB_SELINUX)
> >  src_ginstall_LDADD += $(LIB_SELINUX)
> > -src_id_LDADD += $(LIB_SELINUX)
> > +src_id_LDADD += $(LIB_SELINUX) $(LIBSMACK_LIBS)
> >  src_ls_LDADD += $(LIB_SELINUX)
> >  src_mkdir_LDADD += $(LIB_SELINUX)
> >  src_mkfifo_LDADD += $(LIB_SELINUX)
> > 
> 
> On a general note, smack is a simpler MAC system to SELinux,
> used primarily in intel derived embedded platforms.
> I notice small patch to ls here: https://github.com/promovicz/smack-util
> and a more encompassing busybox patch given the more embedded
> use of smack currently.

The target that I'm working these patches is Tizen PC. The utilities
that you are referring to are legacy. The exception is that 
coreutils-6.9 patch is used in Tizen Mobile. In Tizen PC we are 
happily using GPLv3 licensed code.

Instead library and command-line utils come from
https://github.com/smack-team/smack. In core utils we want to use
libsmack
to detect availability of SMACK LSM because it contains reliable code to
do
that and it would not feel right to replicate that code. If I had needed
code for taking label from /proc/self/attr/current, I would not added a
new dependency but instead would have wrote routines into coreutils.

This library is also used in our command-line utilities and RPM plug-in
that setups security contexes.

> 
> While overloading -Z to refer to both SELinux and smack seems OK,
> I'm worried about "writeable" options.  I.E. should cp -Z copy
> both smack and SELinux attributes? Should cp --context=... support
> both also, and how would that impact portability of scripts?

Well, cp does not need patching (at least we don't need it)
because it is already able to pass extended attributes through
on request. That's enough for our needs.

> 
> As the lines blur between embedded and traditional systems,
> I see coreutils being more applicable to this space (from a
> technical point of view, rather than a licensing one).
> Do you have a practical reason for this effort in an embeded space,
> or are you interested in smack in the "traditional" space?

I guess you got your answer in my previous comments.

> 
> thanks,
> Pádraig.

Thanks.

/Jarkko