ls doesn't detect file capabilities properly

[Ivan Labath]

Hello,

ls calls cap_get_file() from has_capabilty() from print_color_indicator()
on file names ignoring their path, effectively checking wrong (if any) files
for capabilities.

Tested (not) to work on 7.5 and current git version.

Simple test case:

mkdir test test/dir
cd test
touch a dir/a
setcap '=' a
ls --color a dir

Both a's will be colored according to LS_COLORS ca entry. In most use cases
however the bug will lead to files with capabilities not being highlighted.

I believe the proper fix would be to check for capabilities in the same
place where all other stating is done (gobble_file()).

I don't have a patch though, as gobble_file() function is very long (300
lines) and complicated.

Greetings,
Ivan

[Pádraig Brady]

Ivan Labath wrote:

> Hello,
>
> ls calls cap_get_file() from has_capabilty() from print_color_indicator()
> on file names ignoring their path, effectively checking wrong (if any) files
> for capabilities.
>
> Tested (not) to work on 7.5 and current git version.
>
> Simple test case:
>
> mkdir test test/dir
> cd test
> touch a dir/a
> setcap '=' a
> ls --color a dir
>
> Both a's will be colored according to LS_COLORS ca entry. In most use cases
> however the bug will lead to files with capabilities not being highlighted.
>
> I believe the proper fix would be to check for capabilities in the same
> place where all other stating is done (gobble_file()).
>
> I don't have a patch though, as gobble_file() function is very long (300
> lines) and complicated.

Oops. You're correct.
I'll do a fix and test soon.

thanks,
Pádraig.

[Pádraig Brady]

Attached.

cheers,
Pádraig.

From 6fdafcb5568c9c1867dde4fe02c63e97d798a7e1 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <P@...>
Date: Sun, 8 Nov 2009 03:45:27 +0000
Subject: [PATCH] ls: fix capability coloring

Capability checking was incorrectly done on just the base name
rather than on the whole path.  Consequently there could be both
false positives and negatives when coloring files with capabilities.
Also capability checking was not done at all in certain cases for
non executable files.
* src/ls.c (struct fileinfo): Add a has_capability member.
(print_color_indicator): Refactor to pass just a fileinfo pointer
and a flag to say if we're dealing with a symlink target.
(print_name_with_quoting): Likewise.
(gobble_file): Set has_capability in the fileinfo struct.  Also do
a capability check even if executable coloring is disabled.
Ditto for SETUID and SETUID coloring.
(print_long_format): Adjust to refactored print_name_with_quoting.
(quote_name): Likewise.
(print_file_name_and_frills): Likewise.
* tests/ls/capability: Test the various false positive and negatives.
* THANKS: Add reporter (Ivan Labath)
---
 THANKS              |    1 +
 src/ls.c            |  146 ++++++++++++++++++++++++++++-----------------------
 tests/ls/capability |   36 ++++++++++---
 3 files changed, 111 insertions(+), 72 deletions(-)

diff --git a/THANKS b/THANKS
index c583e2a..e5fecb2 100644
--- a/THANKS
+++ b/THANKS
@@ -247,6 +247,7 @@ Ian Turner                          vectro@...
 Iida Yosiaki                        iida@...
 Ilya N. Golubev                     gin@...
 Ingo Saitz                          ingo@...
+Ivan Labath                         labath3@...
 Ivo Timmermans                      ivo@...
 James                               james@...
 James Antill                        jmanti%essex.ac.uk@...
diff --git a/src/ls.c b/src/ls.c
index 08fdf5f..2537aee 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -214,6 +214,9 @@ struct fileinfo
     /* For long listings, true if the file has an access control list,
        or an SELinux security context.  */
     enum acl_type acl_type;
+
+    /* For color listings, true if a regular file has capability info.  */
+    bool has_capability;
   };
 
 #define LEN_STR_PAIR(s) sizeof (s) - 1, s
@@ -241,9 +244,8 @@ static bool file_ignored (char const *name);
 static uintmax_t gobble_file (char const *name, enum filetype type,
                               ino_t inode, bool command_line_arg,
                               char const *dirname);
-static bool print_color_indicator (const char *name, mode_t mode, int linkok,
-                                   bool stat_ok, enum filetype type,
-                                   nlink_t nlink);
+static bool print_color_indicator (const struct fileinfo *f,
+                                   bool symlink_target);
 static void put_indicator (const struct bin_str *ind);
 static void add_ignore_pattern (const char *pattern);
 static void attach (char *dest, const char *dirname, const char *name);
@@ -264,11 +266,9 @@ static int format_user_width (uid_t u);
 static int format_group_width (gid_t g);
 static void print_long_format (const struct fileinfo *f);
 static void print_many_per_line (void);
-static size_t print_name_with_quoting (const char *p, mode_t mode,
-                                       int linkok, bool stat_ok,
-                                       enum filetype type,
+static size_t print_name_with_quoting (const struct fileinfo *f,
+                                       bool symlink_target,
                                        struct obstack *stack,
-                                       nlink_t nlink,
                                        size_t start_col);
 static void prep_non_filename_text (void);
 static bool print_type_indicator (bool stat_ok, mode_t mode,
@@ -2637,6 +2637,37 @@ unsigned_file_size (off_t size)
   return size + (size < 0) * ((uintmax_t) OFF_T_MAX - OFF_T_MIN + 1);
 }
 
+#ifdef HAVE_CAP
+/* Return true if NAME has a capability (see linux/capability.h) */
+static bool
+has_capability (char const *name)
+{
+  char *result;
+  bool has_cap;
+
+  cap_t cap_d = cap_get_file (name);
+  if (cap_d == NULL)
+    return false;
+
+  result = cap_to_text (cap_d, NULL);
+  cap_free (cap_d);
+  if (!result)
+    return false;
+
+  /* check if human-readable capability string is empty */
+  has_cap = !!*result;
+
+  cap_free (result);
+  return has_cap;
+}
+#else
+static bool
+has_capability (char const *name ATTRIBUTE_UNUSED)
+{
+  return false;
+}
+#endif
+
 /* Enter and remove entries in the table `cwd_file'.  */
 
 /* Empty the table of files.  */
@@ -2718,11 +2749,16 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
                  to see if it's executable.  */
               || (type == normal && (indicator_style == classify
                                      /* This is so that --color ends up
-                                        highlighting files with the executable
-                                        bit set even when options like -F are
-                                        not specified.  */
+                                        highlighting files with these mode
+                                        bits set even when options like -F are
+                                        not specified.  Note we do a redundant
+                                        stat in the very unlikely case were
+                                        C_CAP is set but not the others. */
                                      || (print_with_color
-                                         && is_colored (C_EXEC))
+                                         && (is_colored (C_EXEC)
+                                             || is_colored (C_SETUID)
+                                             || is_colored (C_SETGID)
+                                             || is_colored (C_CAP)))
                                      )))))
 
     {
@@ -2793,6 +2829,9 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
 
       f->stat_ok = true;
 
+      if (print_with_color && is_colored (C_CAP))
+        f->has_capability = has_capability (absolute_name);
+
       if (format == long_format || print_scontext)
         {
           bool have_selinux = false;
@@ -3761,18 +3800,14 @@ print_long_format (const struct fileinfo *f)
     }
 
   DIRED_FPUTS (buf, stdout, p - buf);
-  size_t w = print_name_with_quoting (f->name, FILE_OR_LINK_MODE (f), f->linkok,
-                                      f->stat_ok, f->filetype, &dired_obstack,
-                                      f->stat.st_nlink, p - buf);
+  size_t w = print_name_with_quoting (f, false, &dired_obstack, p - buf);
 
   if (f->filetype == symbolic_link)
     {
       if (f->linkname)
         {
           DIRED_FPUTS_LITERAL (" -> ", stdout);
-          print_name_with_quoting (f->linkname, f->linkmode, f->linkok - 1,
-                                   f->stat_ok, f->filetype, NULL,
-                                   f->stat.st_nlink, (p - buf) + w + 4);
+          print_name_with_quoting (f, true, NULL, (p - buf) + w + 4);
           if (indicator_style != none)
             print_type_indicator (true, f->linkmode, unknown);
         }
@@ -3948,19 +3983,20 @@ quote_name (FILE *out, const char *name, struct quoting_options const *options,
 }
 
 static size_t
-print_name_with_quoting (const char *p, mode_t mode, int linkok,
-                         bool stat_ok, enum filetype type,
-                         struct obstack *stack, nlink_t nlink,
+print_name_with_quoting (const struct fileinfo *f,
+                         bool symlink_target,
+                         struct obstack *stack,
                          size_t start_col)
 {
+  const char* name = symlink_target ? f->linkname : f->name;
+
   bool used_color_this_time
-    = (print_with_color
-       && print_color_indicator (p, mode, linkok, stat_ok, type, nlink));
+    = (print_with_color && print_color_indicator (f, symlink_target));
 
   if (stack)
     PUSH_CURRENT_DIRED_POS (stack);
 
-  size_t width = quote_name (stdout, p, filename_quoting_options, NULL);
+  size_t width = quote_name (stdout, name, filename_quoting_options, NULL);
   dired_pos += width;
 
   if (stack)
@@ -4012,9 +4048,7 @@ print_file_name_and_frills (const struct fileinfo *f, size_t start_col)
   if (print_scontext)
     printf ("%*s ", format == with_commas ? 0 : scontext_width, f->scontext);
 
-  size_t width = print_name_with_quoting (f->name, FILE_OR_LINK_MODE (f),
-                                          f->linkok, f->stat_ok, f->filetype,
-                                          NULL, f->stat.st_nlink, start_col);
+  size_t width = print_name_with_quoting (f, false, NULL, start_col);
 
   if (indicator_style != none)
     width += print_type_indicator (f->stat_ok, f->stat.st_mode, f->filetype);
@@ -4065,55 +4099,38 @@ print_type_indicator (bool stat_ok, mode_t mode, enum filetype type)
   return !!c;
 }
 
-#ifdef HAVE_CAP
-/* Return true if NAME has a capability (see linux/capability.h) */
-static bool
-has_capability (char const *name)
-{
-  char *result;
-  bool has_cap;
-
-  cap_t cap_d = cap_get_file (name);
-  if (cap_d == NULL)
-    return false;
-
-  result = cap_to_text (cap_d, NULL);
-  cap_free (cap_d);
-  if (!result)
-    return false;
-
-  /* check if human-readable capability string is empty */
-  has_cap = !!*result;
-
-  cap_free (result);
-  return has_cap;
-}
-#else
-static bool
-has_capability (char const *name ATTRIBUTE_UNUSED)
-{
-  return false;
-}
-#endif
-
 /* Returns whether any color sequence was printed. */
 static bool
-print_color_indicator (const char *name, mode_t mode, int linkok,
-                       bool stat_ok, enum filetype filetype,
-                       nlink_t nlink)
+print_color_indicator (const struct fileinfo *f, bool symlink_target)
 {
   enum indicator_no type;
   struct color_ext_type *ext; /* Color extension */
   size_t len; /* Length of name */
 
+  const char* name;
+  mode_t mode;
+  int linkok;
+  if (symlink_target)
+    {
+      name = f->linkname;
+      mode = f->linkmode;
+      linkok = f->linkok -1;
+    }
+  else
+    {
+      name = f->name;
+      mode = FILE_OR_LINK_MODE (f);
+      linkok = f->linkok;
+    }
+
   /* Is this a nonexistent file?  If so, linkok == -1.  */
 
   if (linkok == -1 && color_indicator[C_MISSING].string != NULL)
     type = C_MISSING;
-  else if (! stat_ok)
+  else if (!f->stat_ok)
     {
       static enum indicator_no filetype_indicator[] = FILETYPE_INDICATORS;
-      type = filetype_indicator[filetype];
+      type = filetype_indicator[f->filetype];
     }
   else
     {
@@ -4125,12 +4142,11 @@ print_color_indicator (const char *name, mode_t mode, int linkok,
             type = C_SETUID;
           else if ((mode & S_ISGID) != 0 && is_colored (C_SETGID))
             type = C_SETGID;
-          /* has_capability() called second for performance.  */
-          else if (is_colored (C_CAP) && has_capability (name))
+          else if (is_colored (C_CAP) && f->has_capability)
             type = C_CAP;
           else if ((mode & S_IXUGO) != 0 && is_colored (C_EXEC))
             type = C_EXEC;
-          else if ((1 < nlink) && is_colored (C_MULTIHARDLINK))
+          else if ((1 < f->stat.st_nlink) && is_colored (C_MULTIHARDLINK))
             type = C_MULTIHARDLINK;
         }
       else if (S_ISDIR (mode))
diff --git a/tests/ls/capability b/tests/ls/capability
index 958a8dd..fb75eae 100755
--- a/tests/ls/capability
+++ b/tests/ls/capability
@@ -33,13 +33,35 @@ grep '^#define HAVE_CAP 1$' $CONFIG_HEADER > /dev/null \
 # Don't let a different umask perturb the results.
 umask 22
 
-touch test
-setcap cap_net_bind_service=ep test \
-  || skip_test_ "setcap doesn't work"
+# We create 2 files of the same name as
+# before coreutils 8.1 only the name rather than
+# the full path was used to read the capabilities
+# thus giving false positives and negatives.
+mkdir test test/dir
+cd test
+touch cap_pos dir/cap_pos dir/cap_neg
+for file in cap_pos dir/cap_neg; do
+  setcap 'cap_net_bind_service=ep' $file ||
+    skip_test_ "setcap doesn't work"
+done
+
 code='30;41'
-LS_COLORS="ca=$code" \
-  ls --color=always test > out || fail=1
-printf "\033[0m\033[${code}mtest\033[0m\n\033[m" > out_ok || fail=1
-compare out out_ok || fail=1
+# Note we explicitly disable "executable" coloring
+# so that capability coloring is not dependent on it,
+# as was the case before coreutils 8.1
+for ex in '' ex=:; do
+  LS_COLORS="di=:${ex}ca=$code" \
+    ls --color=always cap_pos dir > out || fail=1
+
+  env printf "\
+\e[0m\e[${code}mcap_pos\e[0m
+
+dir:
+\e[${code}mcap_neg\e[0m
+cap_pos
+\e[m" > out_ok || skip_test_ "Error printing expected output"
+
+  compare out out_ok || fail=1
+done
 
 Exit $fail
--
1.6.2.5

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Pádraig Brady on 11/7/2009 9:09 PM:
> Attached.

This needs a NEWS entry.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEUEARECAAYFAkr2RWUACgkQ84KuGfSFAYDEHwCY5gzIwYwNFj+qN/GTV0ewycZc
ugCdFIqEa9TOe4YwT9RTRYJdfpgbVjA=
=n2f/
-----END PGP SIGNATURE-----

[Pádraig Brady]

Pádraig Brady wrote:
> Attached.
>
> cheers,
> Pádraig.
>

Oops for speed, this:

      if (print_with_color && is_colored (C_CAP))
        f->has_capability = has_capability (absolute_name);

should be:

      if (type == normal && print_with_color && is_colored (C_CAP))
        f->has_capability = has_capability (absolute_name);

cheers,
Pádraig.

[Pádraig Brady]

Eric Blake wrote:
> According to Pádraig Brady on 11/7/2009 9:09 PM:
>> Attached.
>
> This needs a NEWS entry.

Latest version at http://www.pixelbeat.org/ls-cap.diff

I'll push when I get back this evening
unless anyone wants to push beforehand.

cheers,
Pádraig.

[Jim Meyering]

Pádraig Brady wrote:
> Latest version at http://www.pixelbeat.org/ls-cap.diff

Thanks!
Looks fine, modulo a couple nits:

In ls.c,

  +      linkok = f->linkok -1;

s/-1/- 1/

  +                                        stat in the very unlikely case were
  +                                        C_CAP is set but not the others. */

s/were/where/


In tests/ls/capability, please change this
  \e[m" > out_ok || skip_test_ "Error printing expected output"
to this
  \e[m" > out_ok || framework_failure
or
  \e[m" > out_ok || fail=1
or maybe you'd like a new function, say fail_test_,
that exits right away and prints a reason for failure?

Most (all?) write failures should already cause test failures,
not "skipped" tests.  We tend to dismiss "skipped" tests.
------------------------------

Oops, the test fails for me on F12-beta, which has
libcap-2.16-5.fc12.x86_64.  I ran this:

    sudo make check -C tests TESTS=ls/capability VERBOSE=yes >& log

Sorry, but I won't have time to investigate tonight.






make: Entering directory `/home/j/w/co/cu/tests'
make  check-TESTS
make[1]: Entering directory `/home/j/w/co/cu/tests'
make[2]: Entering directory `/home/j/w/co/cu/tests'
FAIL: ls/capability
=======================================================
   GNU coreutils 8.0.107-3f836: tests/test-suite.log
=======================================================

1 of 1 test failed.

.. contents:: :depth: 2


FAIL: ls/capability (exit: 1)
=============================

+ ls --version
ls (GNU coreutils) 8.0.107-3f836
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Richard M. Stallman and David MacKenzie.
+ . ./test-lib.sh
++ unset function_test
++ eval 'function_test() { return 11; }; function_test'
+++ function_test
+++ return 11
++ test 11 '!=' 11
+++ pwd
++ test_dir_=/home/j/w/co/cu/tests
+++ this_test_
+++ echo ././ls/capability
+++ sed 's,.*/,,'
++ this_test=capability
+++ /home/j/w/co/cu/src/mktemp -d --tmp=/home/j/w/co/cu/tests cu-capability.XXXXXXXXXX
++ t_=/home/j/w/co/cu/tests/cu-capability.wtHWRVBq3k
++ trap remove_tmp_ 0
++ trap 'Exit $?' 1 2 13 15
++ cd /home/j/w/co/cu/tests/cu-capability.wtHWRVBq3k
++ diff --version
++ grep GNU
++ fail=0
+ require_root_
+ uid_is_privileged_
++ id -u
+ my_uid=0
+ case $my_uid in
+ NON_ROOT_USERNAME=nobody
++ id -g nobody
+ NON_ROOT_GROUP=99
+ grep '^#define HAVE_CAP 1$' /home/j/w/co/cu/lib/config.h
+ grep 'usage: setcap'
+ umask 22
+ mkdir test test/dir
+ cd test
+ touch cap_pos dir/cap_pos dir/cap_neg
+ for file in cap_pos dir/cap_neg
+ setcap cap_net_bind_service=ep cap_pos
+ for file in cap_pos dir/cap_neg
+ setcap cap_net_bind_service=ep dir/cap_neg
+ code='30;41'
+ for ex in ''\'''\''' ex=:
+ LS_COLORS='di=:ca=30;41'
+ ls --color=always cap_pos dir
+ env printf '\e[0m\e[30;41mcap_pos\e[0m

dir:
\e[30;41mcap_neg\e[0m
cap_pos
\e[m'
+ compare out out_ok
+ diff -u out out_ok
--- out 2009-11-08 18:17:45.286781576 +0100
+++ out_ok 2009-11-08 18:17:45.289781711 +0100
@@ -1,6 +1,6 @@
-cap_pos
+cap_pos

 dir:
-cap_neg
+cap_neg
 cap_pos
 
\ No newline at end of file
+ fail=1
+ for ex in ''\'''\''' ex=:
+ LS_COLORS='di=:ex=:ca=30;41'
+ ls --color=always cap_pos dir
+ env printf '\e[0m\e[30;41mcap_pos\e[0m

dir:
\e[30;41mcap_neg\e[0m
cap_pos
\e[m'
+ compare out out_ok
+ diff -u out out_ok
--- out 2009-11-08 18:17:45.295782282 +0100
+++ out_ok 2009-11-08 18:17:45.298781740 +0100
@@ -1,6 +1,6 @@
-cap_pos
+cap_pos

 dir:
-cap_neg
+cap_neg
 cap_pos
 
\ No newline at end of file
+ fail=1
+ Exit 1
+ set +e
+ exit 1
+ exit 1
+ remove_tmp_
+ __st=1
+ cleanup_
+ :
+ cd /home/j/w/co/cu/tests
+ chmod -R u+rwx /home/j/w/co/cu/tests/cu-capability.wtHWRVBq3k
+ rm -rf /home/j/w/co/cu/tests/cu-capability.wtHWRVBq3k
+ exit 1
======================================
1 of 1 test failed
See tests/test-suite.log
Please report to bug-coreutils@...
======================================
make[2]: *** [test-suite.log] Error 1
make[2]: Leaving directory `/home/j/w/co/cu/tests'
make[1]: *** [check-TESTS] Error 2
make[1]: Leaving directory `/home/j/w/co/cu/tests'
make: *** [check-am] Error 2
make: Leaving directory `/home/j/w/co/cu/tests'

[Pádraig Brady]

Jim Meyering wrote:
> Pádraig Brady wrote:
>> Latest version at http://www.pixelbeat.org/ls-cap.diff
>
> Oops, the test fails for me on F12-beta, which has
> libcap-2.16-5.fc12.x86_64.  I ran this:
>
>     sudo make check -C tests TESTS=ls/capability VERBOSE=yes >& log
>
> Sorry, but I won't have time to investigate tonight.

Heh, the test is good (fails for me too) but I didn't
actually run it in my rush out the door this morning
(I probably ran it as non root).  The particular problem
was not supporting the different way command line arguments
are processed.

Hopefully the latest version at the link above is OK.

I'll push it in the morning (12 hours).

cheers,
Pádraig.

[Jim Meyering]

Pádraig Brady wrote:
...
> Heh, the test is good (fails for me too) but I didn't
> actually run it in my rush out the door this morning
> (I probably ran it as non root).  The particular problem
> was not supporting the different way command line arguments
> are processed.
>
> Hopefully the latest version at the link above is OK.

Yes, that fixed it, indeed.
Thanks!