From: Pádraig Brady <P@draigBrady.com>
Date: Tue, 19 Mar 2024 23:34:31 +0000 (+0000)
Subject: chmod: fix TOCTOU security issue with symlink replacement
X-Git-Tag: v9.5~24
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=425b8a2f534fe02e8c1e39ad6a3d2c18eca12de3;p=thirdparty%2Fcoreutils.git

chmod: fix TOCTOU security issue with symlink replacement

This is an issue with -[H]R mode, where an attacker
may replace a traversed file with a symlink
between where we stat() the file and chmod() the file.

* src/chmod.c (process_file): Remove the first !S_ISLNK guard
as that's now just an optimization, and instead consistently
apply fchmodat() to files/symlinks.  Ensure AT_SYMLINK_NOFOLLOW
is set when traversing in default (-H) mode.
* NEWS: Mention the bug fix.
Fixes https://bugs.gnu.org/11108
---

diff --git a/NEWS b/NEWS
index 0f4503cbbb..b3004273b6 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  chmod -R now avoids a race where an attacker may replace a traversed file
+  with a symlink, causing chmod to operate on an unintended file.
+  [This bug was present in "the beginning".]
+
   cp, mv, and install no longer issue spurious diagnostics like "failed
   to preserve ownership" when copying to GNU/Linux CIFS file systems.
   They do this by working around some Linux CIFS bugs.
diff --git a/src/chmod.c b/src/chmod.c
index 835b75d9ac..b95371d0d3 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -301,18 +301,16 @@ process_file (FTS *fts, FTSENT *ent)
       return false;
     }
 
-  /* With -H (default) or -P, (without -h), avoid operating on symlinks.
-     With -L, S_ISLNK should be false, and with -RP, dereference is 0.  */
-  if (ch.status == CH_NOT_APPLIED
-      && ! (S_ISLNK (file_stats->st_mode) && dereference == -1))
+  if (ch.status == CH_NOT_APPLIED)
     {
       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);
-      /* XXX: Racy if FILE is now replaced with a symlink, which is
-         a potential security issue with -[H]R.  */
+      bool follow_symlink = !!dereference;
+      if (dereference == -1) /* -H with/without -R, -P without -R.  */
+        follow_symlink = ent->fts_level == 0;
       if (fchmodat (fts->fts_cwd_fd, file, ch.new_mode,
-                    dereference ? 0 : AT_SYMLINK_NOFOLLOW) == 0)
+                    follow_symlink ? 0 : AT_SYMLINK_NOFOLLOW) == 0)
         ch.status = CH_SUCCEEDED;
       else
         {
@@ -590,6 +588,9 @@ main (int argc, char **argv)
         }
     }
 
+  if (dereference == -1 && bit_flags == FTS_LOGICAL)
+    dereference = 1;
+
   if (reference_file)
     {
       if (mode)