On 25/01/2024 14:13, Pádraig Brady wrote:
On 25/01/2024 12:30, Johannes Segitz wrote:
Hello,

chown has a flag that prevents symlink following. chown/chmod is sometimes
used in %post/%pre sections of rpm packages to fix up permissions. When
this is done in user owned directories (somewhere along the path) this is a
security problem. chown allows users to handle this via the -h flag which
instructs it not to follow a symlink.

The attached patch adds this flag for chmod. I read
https://git.savannah.gnu.org/cgit/coreutils.git/plain/README-hacking
but chmod doesn't have an email listed, so I set the patch here.

Please CC me in replies, I'm not subscribed to the list.

We've been consolidating chown/chmod/chgrp recently,
and I was already looking at this.
I'll incorporate your patch with what I was working on.

This is the third and last part of the recent chown/chgrp/chmod
alignment series, the previous two being:
 https://github.com/coreutils/coreutils/commit/da091b3ab  add --from to chgrp
 https://github.com/coreutils/coreutils/commit/9cc8d6ff5  merge chown/chgrp 
sources

The attached adds -hHLP, --{no-,}dereference options to chmod,
to align with chown, and chgrp.  It also aligns with chmod on other systems.

thanks,
Pádraig.
From 8a1cbc81d5eebe0a6497886000dedd69939f0e59 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 28 Nov 2016 09:13:39 +0100
Subject: [PATCH 2/2] chmod: add support for -h, -H,-L,-P, --dereference
 options

There have been various requests to add -h to avoid following symlinks
for security reasons.  This wasn't provided previously as chmod(1)
already ignored symlinks unless specified on the command line.
Note chmod defaults to -H mode rather than the chown default of -P,
as usually chown can work directly on symlinks and so defaults
to not traversing those specified on the command line.

Adding -HLP will allow chmod to disable traversing CLI symlinks to dirs.
Adding -h will allow to disable following CLI symlinks to files/dirs,
  also operating on all symlinks on systems that support that.
Adding --dereference will be significant with -H (the default).  I.e.
  symlinks to dirs not recursed, but symlinks are dereferenced.
Adding these options will also be consistent with chown(1), chgrp(1),
and chmod(1) on other systems.

Note since chmod(1) currently ignores symlinks by default,
and -h is primarily a mechanism to avoid following symlinks, rather than
for operating on the symlink itself, we make -h try to chmod a symlink,
but ignore ENOTSUP.  In that way we're consistent with chown(1)
where it also ignores ENOTSUP for symlinks, and we don't fail when
trying to be extra secure with command line params.

* doc/coreutils.texi (chmod invocation): Reference the -H,-L,-P
descriptions, and adjust the corresponding macros to say
the default is -H or -P as appropriate.
Add --dereference and -h,--no-dereference descriptions.
* man/chmod.x: Adjust discussion of symlink handling.
* src/chmod.c (main): Accept new options and set
fts flags appropriately.
(process_file): Process / dereference symlinks as necessary.
* src/system.h (emit_symlink_recurse_options): A new function
refactored from chown.c and chmod.c usage().
* tests/chmod/symlinks.sh: New test for the new options.
* tests/local.mk: Reference the new test.
* NEWS: Mention the new feature.
---
 NEWS                    |   4 ++
 doc/coreutils.texi      |  47 +++++++++++++++---
 man/chmod.x             |  10 ++--
 src/chmod.c             | 105 +++++++++++++++++++++++++++++++++++-----
 src/chown.c             |  14 +-----
 src/system.h            |  18 +++++++
 tests/chmod/symlinks.sh |  87 +++++++++++++++++++++++++++++++++
 tests/local.mk          |   1 +
 8 files changed, 250 insertions(+), 36 deletions(-)
 create mode 100755 tests/chmod/symlinks.sh

diff --git a/NEWS b/NEWS
index f21efc7c0..3a7e2aa57 100644
--- a/NEWS
+++ b/NEWS
@@ -73,6 +73,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   chgrp now accepts the --from=OWNER:GROUP option to restrict changes to files
   with matching current OWNER and/or GROUP, as already supported by chown(1).
 
+  chmod adds support for -h, -H,-L,-P, and --dereference options, providing
+  more control over symlink handling.  This supports more secure handling of
+  CLI arguments, and is more consistent with chown, and chmod on other systems.
+
   cp now accepts the --keep-directory-symlink option (like tar), to preserve
   and follow existing symlinks to directories in the destination.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 09e3e9d3f..92c9ceefb 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -1428,10 +1428,14 @@ possibly allowing the attacker to escalate privileges.
 @opindex -P
 @cindex symbolic link to directory, never traverse
 Do not traverse any symbolic links.
+@end macro
+@choptP
+
+@macro choptDefault
 This is the default if none of @option{-H}, @option{-L},
 or @option{-P} is specified.
 @end macro
-@choptP
+@choptDefault
 
 @end table
 
@@ -11723,6 +11727,7 @@ Recursively change ownership of directories and their contents.
 @xref{Traversing symlinks}.
 
 @choptP
+@choptDefault
 @xref{Traversing symlinks}.
 
 @end table
@@ -11836,6 +11841,7 @@ Recursively change the group ownership of directories and their contents.
 @xref{Traversing symlinks}.
 
 @choptP
+@choptDefault
 @xref{Traversing symlinks}.
 
 @end table
@@ -11869,13 +11875,14 @@ chmod [@var{option}]@dots{} @{@var{mode} | --reference=@var{ref_file}@}@c
 @end example
 
 @cindex symbolic links, permissions of
-@command{chmod} never changes the permissions of symbolic links, since
-the @command{chmod} system call cannot change their permissions.
-This is not a problem since the permissions of symbolic links are
-never used.  However, for each symbolic link listed on the command
+@command{chmod} doesn't change the permissions of symbolic links, since
+the @command{chmod} system call cannot change their permissions on most systems,
+and most systems ignore permissions of symbolic links.
+However, for each symbolic link listed on the command
 line, @command{chmod} changes the permissions of the pointed-to file.
 In contrast, @command{chmod} ignores symbolic links encountered during
-recursive directory traversals.
+recursive directory traversals. Options that modify this behavior
+are described below.
 
 Only a process whose effective user ID matches the user ID of the file,
 or a process with appropriate privileges, is permitted to change the
@@ -11909,6 +11916,23 @@ The program accepts the following options.  Also see @ref{Common options}.
 Verbosely describe the action for each @var{file} whose permissions
 actually change.
 
+@item --dereference
+@opindex --dereference
+@cindex symbolic links, changing mode
+Do not act on symbolic links themselves but rather on what they point to.
+This is the default for command line arguments, but not for
+symbolic links encountered when recursing.
+@warnOptDerefWithRec
+
+@item -h
+@itemx --no-dereference
+@opindex -h
+@opindex --no-dereference
+@cindex symbolic links, changing mode
+Act on symbolic links themselves instead of what they point to.
+On systems that do not support this, no diagnostic is issued,
+but see @option{--verbose}.
+
 @item -f
 @itemx --silent
 @itemx --quiet
@@ -11952,6 +11976,17 @@ of the symbolic link, but rather that of the file it refers to.
 @cindex recursively changing access permissions
 Recursively change permissions of directories and their contents.
 
+@choptH
+@choptDefault
+@xref{Traversing symlinks}.
+
+@choptL
+@warnOptDerefWithRec
+@xref{Traversing symlinks}.
+
+@choptP
+@xref{Traversing symlinks}.
+
 @end table
 
 @exitstatus
diff --git a/man/chmod.x b/man/chmod.x
index 71de4b920..2954f55ee 100644
--- a/man/chmod.x
+++ b/man/chmod.x
@@ -60,17 +60,19 @@ file's group, with the same values; and the fourth for other users not
 in the file's group, with the same values.
 .PP
 .B chmod
-never changes the permissions of symbolic links; the
+doesn't change the permissions of symbolic links; the
 .B chmod
-system call cannot change their permissions.  This is not a problem
-since the permissions of symbolic links are never used.
+system call cannot change their permissions on most systems,
+and most systems ignore permissions of symbolic links.
 However, for each symbolic link listed on the command line,
 .B chmod
 changes the permissions of the pointed-to file.
 In contrast,
 .B chmod
 ignores symbolic links encountered during recursive directory
-traversals.
+traversals. Options that modify this behavior are described in the
+.B OPTIONS
+section.
 .SH "SETUID AND SETGID BITS"
 .B chmod
 clears the set-group-ID bit of a
diff --git a/src/chmod.c b/src/chmod.c
index fcd1d3e8f..f047c4834 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -74,6 +74,10 @@ static mode_t umask_value;
 /* If true, change the modes of directories recursively. */
 static bool recurse;
 
+/* 1 if --dereference, 0 if --no-dereference, -1 if neither has been
+   specified.  */
+static int dereference = -1;
+
 /* If true, force silence (suppress most of error messages). */
 static bool force_silent;
 
@@ -93,7 +97,8 @@ static struct dev_ino *root_dev_ino;
    non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
 enum
 {
-  NO_PRESERVE_ROOT = CHAR_MAX + 1,
+  DEREFERENCE_OPTION = CHAR_MAX + 1,
+  NO_PRESERVE_ROOT,
   PRESERVE_ROOT,
   REFERENCE_FILE_OPTION
 };
@@ -101,7 +106,9 @@ enum
 static struct option const long_options[] =
 {
   {"changes", no_argument, nullptr, 'c'},
+  {"dereference", no_argument, nullptr, DEREFERENCE_OPTION},
   {"recursive", no_argument, nullptr, 'R'},
+  {"no-dereference", no_argument, nullptr, 'h'},
   {"no-preserve-root", no_argument, nullptr, NO_PRESERVE_ROOT},
   {"preserve-root", no_argument, nullptr, PRESERVE_ROOT},
   {"quiet", no_argument, nullptr, 'f'},
@@ -207,6 +214,7 @@ process_file (FTS *fts, FTSENT *ent)
   const struct stat *file_stats = ent->fts_statp;
   struct change_status ch = {0};
   ch.status = CH_NO_STAT;
+  struct stat stat_buf;
 
   switch (ent->fts_info)
     {
@@ -244,9 +252,30 @@ process_file (FTS *fts, FTSENT *ent)
       break;
 
     case FTS_SLNONE:
-      if (! force_silent)
-        error (0, 0, _("cannot operate on dangling symlink %s"),
-               quoteaf (file_full_name));
+      if (dereference)
+        {
+          if (! force_silent)
+            error (0, 0, _("cannot operate on dangling symlink %s"),
+                   quoteaf (file_full_name));
+          break;
+        }
+      ch.status = CH_NOT_APPLIED;
+      break;
+
+    case FTS_SL:
+      if (dereference == 1)
+        {
+          if (fstatat (fts->fts_cwd_fd, file, &stat_buf, 0) != 0)
+            {
+              if (! force_silent)
+                error (0, errno, _("cannot dereference %s"),
+                       quoteaf (file_full_name));
+              break;
+            }
+
+          file_stats = &stat_buf;
+        }
+      ch.status = CH_NOT_APPLIED;
       break;
 
     case FTS_DC:		/* directory that causes cycles */
@@ -272,19 +301,28 @@ process_file (FTS *fts, FTSENT *ent)
       return false;
     }
 
-  if (ch.status == CH_NOT_APPLIED && ! S_ISLNK (file_stats->st_mode))
+  /* Only attempt to chmod symlinks directly, with -P or -h.
+     I.e., skip symlinks encountered in -H (default) mode.  */
+  if (ch.status == CH_NOT_APPLIED
+      && ! (S_ISLNK (file_stats->st_mode) && dereference == -1))
     {
       ch.old_mode = file_stats->st_mode;
       ch.new_mode = mode_adjust (ch.old_mode, S_ISDIR (ch.old_mode) != 0,
                                  umask_value, change, nullptr);
-      if (chmodat (fts->fts_cwd_fd, file, ch.new_mode) == 0)
+      if (fchmodat (fts->fts_cwd_fd, file, ch.new_mode,
+                    dereference ? 0 : AT_SYMLINK_NOFOLLOW) == 0)
         ch.status = CH_SUCCEEDED;
       else
         {
-          if (! force_silent)
-            error (0, errno, _("changing permissions of %s"),
-                   quoteaf (file_full_name));
-          ch.status = CH_FAILED;
+          if (! is_ENOTSUP (errno))
+            {
+              if (! force_silent)
+                error (0, errno, _("changing permissions of %s"),
+                       quoteaf (file_full_name));
+
+              ch.status = CH_FAILED;
+            }
+          /* else treat not supported as not applied.  */
         }
     }
 
@@ -387,6 +425,11 @@ With --reference, change the mode of each FILE to that of RFILE.\n\
   -c, --changes          like verbose but report only when a change is made\n\
   -f, --silent, --quiet  suppress most error messages\n\
   -v, --verbose          output a diagnostic for every file processed\n\
+"), stdout);
+      fputs (_("\
+      --dereference      affect the referent of each symbolic link,\n\
+                           rather than the symbolic link itself\n\
+  -h, --no-dereference   affect each symbolic link, rather than the referent\n\
 "), stdout);
       fputs (_("\
       --no-preserve-root  do not treat '/' specially (the default)\n\
@@ -399,6 +442,7 @@ With --reference, change the mode of each FILE to that of RFILE.\n\
       fputs (_("\
   -R, --recursive        change files and directories recursively\n\
 "), stdout);
+      emit_symlink_recurse_options ("-H");
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       fputs (_("\
@@ -423,6 +467,7 @@ main (int argc, char **argv)
   bool preserve_root = false;
   char const *reference_file = nullptr;
   int c;
+  int bit_flags = FTS_COMFOLLOW | FTS_PHYSICAL;
 
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -435,13 +480,35 @@ main (int argc, char **argv)
   recurse = force_silent = diagnose_surprises = false;
 
   while ((c = getopt_long (argc, argv,
-                           ("Rcfvr::w::x::X::s::t::u::g::o::a::,::+::=::"
+                           ("HLPRcfhvr::w::x::X::s::t::u::g::o::a::,::+::=::"
                             "0::1::2::3::4::5::6::7::"),
                            long_options, nullptr))
          != -1)
     {
       switch (c)
         {
+
+        case 'H': /* Traverse command-line symlinks-to-directories.  */
+          bit_flags = FTS_COMFOLLOW | FTS_PHYSICAL;
+          break;
+
+        case 'L': /* Traverse all symlinks-to-directories.  */
+          bit_flags = FTS_LOGICAL;
+          break;
+
+        case 'P': /* Traverse no symlinks-to-directories.  */
+          bit_flags = FTS_PHYSICAL;
+          break;
+
+        case 'h': /* --no-dereference: affect symlinks */
+          dereference = 0;
+          break;
+
+        case DEREFERENCE_OPTION: /* --dereference: affect the referent
+                                    of each symlink */
+          dereference = 1;
+          break;
+
         case 'r':
         case 'w':
         case 'x':
@@ -510,6 +577,18 @@ main (int argc, char **argv)
         }
     }
 
+  if (recurse)
+    {
+      if (bit_flags == FTS_PHYSICAL)
+        {
+          if (dereference == 1)
+            error (EXIT_FAILURE, 0,
+                   _("-R --dereference requires either -H or -L"));
+          dereference = 0;
+        }
+    }
+
+
   if (reference_file)
     {
       if (mode)
@@ -564,8 +643,8 @@ main (int argc, char **argv)
       root_dev_ino = nullptr;
     }
 
-  ok = process_files (argv + optind,
-                      FTS_COMFOLLOW | FTS_PHYSICAL | FTS_DEFER_STAT);
+  bit_flags |= FTS_DEFER_STAT;
+  ok = process_files (argv + optind, bit_flags);
 
   main_exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
 }
diff --git a/src/chown.c b/src/chown.c
index 1262fa7a8..48e0d9c5b 100644
--- a/src/chown.c
+++ b/src/chown.c
@@ -121,19 +121,7 @@ With --reference, change the group of each FILE to that of RFILE.\n\
       fputs (_("\
   -R, --recursive        operate on files and directories recursively\n\
 "), stdout);
-      fputs (_("\
-\n\
-The following options modify how a hierarchy is traversed when the -R\n\
-option is also specified.  If more than one is specified, only the final\n\
-one takes effect.\n\
-\n\
-  -H                     if a command line argument is a symbolic link\n\
-                         to a directory, traverse it\n\
-  -L                     traverse every symbolic link to a directory\n\
-                         encountered\n\
-  -P                     do not traverse any symbolic links (default)\n\
-\n\
-"), stdout);
+      emit_symlink_recurse_options ("-P");
       fputs (HELP_OPTION_DESCRIPTION, stdout);
       fputs (VERSION_OPTION_DESCRIPTION, stdout);
       if (chown_mode == CHOWN_CHOWN)
diff --git a/src/system.h b/src/system.h
index 69aed6ea2..b5d38bb18 100644
--- a/src/system.h
+++ b/src/system.h
@@ -623,6 +623,24 @@ the VERSION_CONTROL environment variable.  Here are the values:\n\
 "), stdout);
 }
 
+static inline void
+emit_symlink_recurse_options (char const *default_opt)
+{
+      printf (_("\
+\n\
+The following options modify how a hierarchy is traversed when the -R\n\
+option is also specified.  If more than one is specified, only the final\n\
+one takes effect. '%s' is the default.\n\
+\n\
+  -H                     if a command line argument is a symbolic link\n\
+                         to a directory, traverse it\n\
+  -L                     traverse every symbolic link to a directory\n\
+                         encountered\n\
+  -P                     do not traverse any symbolic links\n\
+\n\
+"), default_opt);
+}
+
 static inline void
 emit_exec_status (char const *program)
 {
diff --git a/tests/chmod/symlinks.sh b/tests/chmod/symlinks.sh
new file mode 100755
index 000000000..9e9470e83
--- /dev/null
+++ b/tests/chmod/symlinks.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+# Verify chmod symlink handling options
+
+# Copyright (C) 2024 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ chmod
+
+#dirs
+mkdir -p a/b a/c || framework_failure_
+#files
+touch a/b/file a/c/file || framework_failure_
+#dangling link
+ln -s foo a/dangle || framework_failure_
+#link to file
+ln -s ../b/file a/c/link || framework_failure_
+#link to dir
+ln -s b a/dirlink || framework_failure_
+
+# tree -F a
+#  a/
+#  |-- b/
+#  |   '-- file
+#  |-- c/
+#  |   |-- file
+#  |   '-- link -> ../b/file
+#  |-- dangle -> foo
+#  '-- dirlink -> b/
+
+reset_modes() { chmod 777 a/b a/c a/b/file a/c/file || fail=1; }
+count_755() { test "$(grep 'rwxr-xr-x' 'out' | wc -l)" = "$1"; }
+
+reset_modes
+# -R (with default -H) does not deref traversed symlinks (only cli args)
+chmod 755 -R a/c || fail=1
+ls -l a/b > out || framework_failure_
+count_755 0 || fail=1
+ls -lR a/c > out || framework_failure_
+count_755 1 || fail=1
+
+reset_modes
+# set a/c a/c/file and a/b/file (through symlink) to 755
+chmod 755 -LR a/c || fail=1
+ls -ld a/c a/c/file a/b/file > out || framework_failure_
+count_755 3 || fail=1
+
+reset_modes
+# do not set /a/b/file through symlink (should try to chmod the link itself)
+chmod 755 -RP a/c/ || fail=1
+ls -l a/b > out || framework_failure_
+count_755 0 || fail=1
+
+reset_modes
+# set /a/b/file through symlink
+chmod 755 --dereference a/c/link || fail=1
+ls -l a/b > out || framework_failure_
+count_755 1 || fail=1
+
+reset_modes
+# do not set /a/b/file through symlink (should try to chmod the link itself)
+chmod 755 --no-dereference a/c/link 2>err || fail=1
+ls -l a/b > out || framework_failure_
+count_755 0 || fail=1
+
+# Dangling links should not induce an error if not dereferencing
+for noderef in '-h' '-RP' '-P'; do
+  chmod 755 --no-dereference a/dangle 2>err || fail=1
+done
+# Dangling links should induce an error if dereferencing
+for deref in '' '--deref' '-R'; do
+  returns_ 1 chmod 755 $deref a/dangle 2>err || fail=1
+done
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 7cd1ef7b5..4075525ba 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -476,6 +476,7 @@ all_tests =					\
   tests/chmod/thru-dangling.sh			\
   tests/chmod/umask-x.sh			\
   tests/chmod/usage.sh				\
+  tests/chmod/symlinks.sh				\
   tests/chown/deref.sh				\
   tests/chown/preserve-root.sh			\
   tests/chown/separator.sh			\
-- 
2.43.0

Reply via email to