Re: [PATCH] ls: show SMACK label of a file when available

[Jarkko Sakkinen]

Thanks for the quick response! I'll revise the patch accordingly.

/Jarkko


On Tue, May 14, 2013, at 18:37, Pádraig Brady wrote:
> On 05/13/2013 11:27 AM, Jarkko Sakkinen wrote:
> > Read security.SMACK64 and display it when '-Z' option is given
> > to the ls command.
> > ---
> >  src/local.mk |    1 +
> >  src/ls.c     |   73 
> > ++++++++++++++++++++++++++++++++++++++++++++++------------
> >  2 files changed, 59 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/local.mk b/src/local.mk
> > index 0570d73..95c0672 100644
> > --- a/src/local.mk
> > +++ b/src/local.mk
> > @@ -230,6 +230,7 @@ src_ginstall_LDADD += $(LIB_SELINUX)
> >  src_id_LDADD += $(LIB_SELINUX)
> >  src_id_LDADD += $(LIB_SMACK)
> >  src_ls_LDADD += $(LIB_SELINUX)
> > +src_ls_LDADD += $(LIB_SMACK)
> >  src_mkdir_LDADD += $(LIB_SELINUX)
> >  src_mkfifo_LDADD += $(LIB_SELINUX)
> >  src_mknod_LDADD += $(LIB_SELINUX)
> > diff --git a/src/ls.c b/src/ls.c
> > index f1ed43b..5b0c190 100644
> > --- a/src/ls.c
> > +++ b/src/ls.c
> > @@ -115,6 +115,11 @@
> >  # include <sys/capability.h>
> >  #endif
> >  
> > +#ifdef HAVE_SMACK
> > +# include <sys/smack.h>
> > +# include <attr/xattr.h>
> > +#endif
> > +
> >  #define PROGRAM_NAME (ls_mode == LS_LS ? "ls" \
> >                        : (ls_mode == LS_MULTI_COL \
> >                           ? "dir" : "vdir"))
> > @@ -350,7 +355,16 @@ static struct pending *pending_dirs;
> >  
> >  static struct timespec current_time;
> >  
> > -static bool print_scontext;
> > +enum scontext_type
> > +  {
> > +  no_context,
> > +  selinux_context,
> > +#ifdef HAVE_SMACK
> > +  smack_context,
> > +#endif
> > +  };
> > +
> > +static enum scontext_type print_scontext;
> >  static char UNKNOWN_SECURITY_CONTEXT[] = "?";
> >  
> >  /* Whether any of the files has an ACL.  This affects the width of the
> > @@ -1363,7 +1377,7 @@ main (int argc, char **argv)
> >  
> >    format_needs_stat = sort_type == sort_time || sort_type == sort_size
> >      || format == long_format
> > -    || print_scontext
> > +    || print_scontext != no_context
> >      || print_block_size;
> >    format_needs_type = (! format_needs_stat
> >                         && (recursive
> > @@ -1565,7 +1579,7 @@ decode_switches (int argc, char **argv)
> >    ignore_mode = IGNORE_DEFAULT;
> >    ignore_patterns = NULL;
> >    hide_patterns = NULL;
> > -  print_scontext = false;
> > +  print_scontext = no_context;
> >  
> >    /* FIXME: put this in a function.  */
> >    {
> > @@ -1941,7 +1955,11 @@ decode_switches (int argc, char **argv)
> >            break;
> >  
> >          case 'Z':
> > -          print_scontext = true;
> > +          print_scontext = selinux_context;
> > +#ifdef HAVE_SMACK
> > +          if (smack_smackfs_path ())
> > +            print_scontext = smack_context;
> > +#endif
> >            break;
> >  
> >          case_GETOPT_HELP_CHAR;
> > @@ -2995,17 +3013,20 @@ gobble_file (char const *name, enum filetype type, 
> > ino_t inode,
> >            && print_with_color && is_colored (C_CAP))
> >          f->has_capability = has_capability_cache (absolute_name, f);
> >  
> > -      if (format == long_format || print_scontext)
> > +      if (format == long_format || print_scontext != no_context)
> >          {
> >            bool have_selinux = false;
> >            bool have_acl = false;
> > -          int attr_len = getfilecon_cache (absolute_name, f, do_deref);
> > -          err = (attr_len < 0);
> >  
> > -          if (err == 0)
> > -            have_selinux = ! STREQ ("unlabeled", f->scontext);
> > -          else
> > +          if (print_scontext == selinux_context)
> >              {
> > +            int attr_len = getfilecon_cache (absolute_name, f, do_deref);
> > +            err = (attr_len < 0);
> 
> Indentation is off here.
> You need to indent 2 more after a {
> 
> > +
> > +            if (err == 0)
> > +              have_selinux = ! STREQ ("unlabeled", f->scontext);
> > +            else
> > +              {
> >                f->scontext = UNKNOWN_SECURITY_CONTEXT;
> >  
> >                /* When requesting security context information, don't make
> > @@ -3014,7 +3035,29 @@ gobble_file (char const *name, enum filetype type, 
> > ino_t inode,
> >                   failure isn't in the same class as a stat failure.  */
> >                if (errno == ENOTSUP || errno == EOPNOTSUPP || errno == 
> > ENODATA)
> >                  err = 0;
> > +              }
> > +            }
> > +#ifdef HAVE_SMACK
> > +          else
> 
> This needs to be 'else if (print_scontext == smack_context)'
> otherwise the smack context would be printed with just -l ?
> 
> > +            {
> > +            ssize_t ret = 0;
> > +
> > +            ret = getxattr (absolute_name, "security.SMACK64", NULL, 0);
> > +            if (ret < 0)
> 
> This check looks wrong, and should also include errno == ERANGE probably.
> 
> > +              goto done_xattr;
> > +            f->scontext = xmalloc (ret + 1);
> > +            ret = getxattr (absolute_name, "security.SMACK64", 
> > f->scontext, ret);
> > +done_xattr:
> 
> So smack has no equiv to getfilecon, and uses the lower level getxattr?
> That's fair enough, but it would be good to encapsulate that
> in a getsmackcon_cache(). I.E. have the equivalent to getfilecon_cache()
> which would have the advantage of better structure, honoring of do_deref,
> and avoiding redundant calls on unsupported file systems.
> 
> > +            if (ret < 0)
> > +              {
> > +              f->scontext = UNKNOWN_SECURITY_CONTEXT;
> > +              if (err == ENOTSUP || err == ENOATTR)
> > +                err = 0;
> > +              }
> > +            else
> > +              have_selinux = (ret > 0 && !STREQ ("_", f->scontext));
> >              }
> > +#endif
> >  
> >            if (err == 0 && format == long_format)
> >              {
> > @@ -3111,7 +3154,7 @@ gobble_file (char const *name, enum filetype type, 
> > ino_t inode,
> >              }
> >          }
> 
> All the following changes aren't strictly required.
> I'd change the enum def to explicitly set no_context = 0,
> and then it would be fine to drop all the following I think.
> 
> thanks,
> Pádraig.
> 
> >  
> > -      if (print_scontext)
> > +      if (print_scontext != no_context)
> >          {
> >            int len = strlen (f->scontext);
> >            if (scontext_width < len)
> > @@ -3846,7 +3889,7 @@ print_long_format (const struct fileinfo *f)
> >  
> >    DIRED_INDENT ();
> >  
> > -  if (print_owner || print_group || print_author || print_scontext)
> > +  if (print_owner || print_group || print_author || print_scontext != 
> > no_context)
> >      {
> >        DIRED_FPUTS (buf, stdout, p - buf);
> >  
> > @@ -3859,7 +3902,7 @@ print_long_format (const struct fileinfo *f)
> >        if (print_author)
> >          format_user (f->stat.st_author, author_width, f->stat_ok);
> >  
> > -      if (print_scontext)
> > +      if (print_scontext != no_context)
> >          format_user_or_group (f->scontext, 0, scontext_width);
> >  
> >        p = buf;
> > @@ -4207,7 +4250,7 @@ print_file_name_and_frills (const struct fileinfo *f, 
> > size_t start_col)
> >              : human_readable (ST_NBLOCKS (f->stat), buf, human_output_opts,
> >                                ST_NBLOCKSIZE, output_block_size));
> >  
> > -  if (print_scontext)
> > +  if (print_scontext != no_context)
> >      printf ("%*s ", format == with_commas ? 0 : scontext_width, 
> > f->scontext);
> >  
> >    size_t width = print_name_with_quoting (f, false, NULL, start_col);
> > @@ -4417,7 +4460,7 @@ length_of_file_name_and_frills (const struct fileinfo 
> > *f)
> >                                              output_block_size))
> >                  : block_size_width);
> >  
> > -  if (print_scontext)
> > +  if (print_scontext != no_context)
> >      len += 1 + (format == with_commas ? strlen (f->scontext) : 
> > scontext_width);
> >  
> >    quote_name (NULL, f->name, filename_quoting_options, &name_width);
> > 
>