]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
Revert "binutils: Make smart_rename safe too"
authorNick Clifton <nickc@redhat.com>
Fri, 5 Feb 2021 14:14:01 +0000 (14:14 +0000)
committerNick Clifton <nickc@redhat.com>
Fri, 5 Feb 2021 14:14:01 +0000 (14:14 +0000)
This reverts commit 014cc7f849e8209623fc99264814bce7b3b6faf2.
Given the problems associated with this patch and the others intended to fix the smart_rename CVE, the decision has been taken to$

binutils/ar.c
binutils/arsup.c
binutils/bucomm.h
binutils/objcopy.c
binutils/rename.c

index 40d38c4d8560f4e065575336d08d4c63ebad0a86..45a34e3a6cf92841f962eb50caad2916f92294e4 100644 (file)
@@ -1254,8 +1254,6 @@ write_archive (bfd *iarch)
   char *old_name, *new_name;
   bfd *contents_head = iarch->archive_next;
   int ofd = -1;
-  struct stat target_stat;
-  bfd_boolean skip_stat = FALSE;
 
   old_name = (char *) xmalloc (strlen (bfd_get_filename (iarch)) + 1);
   strcpy (old_name, bfd_get_filename (iarch));
@@ -1301,14 +1299,6 @@ write_archive (bfd *iarch)
   if (!bfd_set_archive_head (obfd, contents_head))
     bfd_fatal (old_name);
 
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-  ofd = dup (ofd);
-  if (iarch == NULL || iarch->iostream == NULL)
-    skip_stat = TRUE;
-  else if (ofd == -1 || fstat (fileno (iarch->iostream), &target_stat) != 0)
-    bfd_fatal (old_name);
-#endif
-
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
 
@@ -1318,7 +1308,7 @@ write_archive (bfd *iarch)
   /* We don't care if this fails; we might be creating the archive.  */
   bfd_close (iarch);
 
-  if (smart_rename (new_name, old_name, ofd, skip_stat ? NULL : &target_stat, 0) != 0)
+  if (smart_rename (new_name, old_name, 0) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
index 107829034c9a20e09640bd790bb55d6e2b431e75..5403a0c5d74ceec52469a4825b820c98164bdb85 100644 (file)
@@ -345,25 +345,13 @@ ar_save (void)
   else
     {
       char *ofilename = xstrdup (bfd_get_filename (obfd));
-      bfd_boolean skip_stat = FALSE;
-      struct stat target_stat;
-      int ofd = -1;
 
       if (deterministic > 0)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
 
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-      /* It's OK to fail; at worst it will result in SMART_RENAME using a slow
-         copy fallback to write the output.  */
-      ofd = dup (fileno (obfd->iostream));
-      if (lstat (real_name, &target_stat) != 0)
-       skip_stat = TRUE;
-#endif
-
       bfd_close (obfd);
 
-      smart_rename (ofilename, real_name, ofd,
-                   skip_stat ? NULL : &target_stat, 0);
+      smart_rename (ofilename, real_name, 0);
       obfd = 0;
       free (ofilename);
     }
index 7a0adfae56590f3f00e0ba59de899eaf3ed359c8..91f6a5b228fa35c2253121edb17264322e5d558d 100644 (file)
@@ -71,8 +71,7 @@ extern void print_version (const char *);
 /* In rename.c.  */
 extern void set_times (const char *, const struct stat *);
 
-extern int smart_rename (const char *, const char *, int, struct stat *, int);
-
+extern int smart_rename (const char *, const char *, int);
 
 /* In libiberty.  */
 void *xmalloc (size_t);
index 404232f778e72542dc1236e46f8ae2a0f0a7780e..00d751522c4d1fbdc894fe9a64e7f5500e6960fb 100644 (file)
@@ -4835,7 +4835,6 @@ strip_main (int argc, char *argv[])
       struct stat statbuf;
       char *tmpname;
       int tmpfd = -1;
-      int copyfd = -1;
 
       if (get_file_size (argv[i]) < 1)
        {
@@ -4849,12 +4848,7 @@ strip_main (int argc, char *argv[])
       else
        tmpname = output_file;
 
-      if (tmpname == NULL
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-         /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
-         || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
-#endif
-      )
+      if (tmpname == NULL)
        {
          bfd_nonfatal_message (argv[i], NULL, NULL,
                                _("could not create temporary file to hold stripped copy"));
@@ -4872,18 +4866,12 @@ strip_main (int argc, char *argv[])
          if (output_file != tmpname)
            status = (smart_rename (tmpname,
                                    output_file ? output_file : argv[i],
-                                   copyfd, &statbuf, preserve_dates) != 0);
+                                   preserve_dates) != 0);
          if (status == 0)
            status = hold_status;
        }
       else
-       {
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-         if (copyfd >= 0)
-           close (copyfd);
-#endif
-         unlink_if_ordinary (tmpname);
-       }
+       unlink_if_ordinary (tmpname);
       if (output_file != tmpname)
        free (tmpname);
     }
@@ -5091,7 +5079,6 @@ copy_main (int argc, char *argv[])
   bfd_boolean use_globalize = FALSE;
   bfd_boolean use_keep_global = FALSE;
   int c, tmpfd = -1;
-  int copyfd = -1;
   struct stat statbuf;
   const bfd_arch_info_type *input_arch = NULL;
 
@@ -5936,16 +5923,9 @@ copy_main (int argc, char *argv[])
   else
     tmpname = output_filename;
 
-  if (tmpname == NULL
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-      /* Retain a copy of TMPFD since we will need it for SMART_RENAME.  */
-      || (tmpfd >= 0 && (copyfd = dup (tmpfd)) == -1)
-#endif
-  )
-    {
-      fatal (_("warning: could not create temporary file whilst copying '%s', (error: %s)"),
-            input_filename, strerror (errno));
-    }
+  if (tmpname == NULL)
+    fatal (_("warning: could not create temporary file whilst copying '%s', (error: %s)"),
+          input_filename, strerror (errno));
 
   copy_file (input_filename, tmpname, tmpfd, &statbuf, input_target,
             output_target, input_arch);
@@ -5954,17 +5934,11 @@ copy_main (int argc, char *argv[])
       if (preserve_dates)
        set_times (tmpname, &statbuf);
       if (tmpname != output_filename)
-       status = (smart_rename (tmpname, input_filename, copyfd, &statbuf,
+       status = (smart_rename (tmpname, input_filename,
                                preserve_dates) != 0);
     }
   else
-    {
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-      if (copyfd >= 0)
-       close (copyfd);
-#endif
-      unlink_if_ordinary (tmpname);
-    }
+    unlink_if_ordinary (tmpname);
 
   if (tmpname != output_filename)
     free (tmpname);
index fece311791532c68d7fff2ee796d432a7a1d5fc9..65ad5bf52c457ccdf2c7ae5c3a24959fc4dc4ccc 100644 (file)
@@ -131,55 +131,17 @@ set_times (const char *destination, const struct stat *statbuf)
 #endif
 #endif
 
-#if !defined (_WIN32) || defined (__CYGWIN32__)
-/* Try to preserve the permission bits and ownership of an existing file when
-   rename overwrites it.  FD is the file being renamed and TARGET_STAT has the
-   status of the file that was overwritten.  */
-static void
-try_preserve_permissions (int fd, struct stat *target_stat)
-{
-  struct stat from_stat;
-  int ret = 0;
-
-  if (fstat (fd, &from_stat) != 0)
-    return;
-
-  int from_mode = from_stat.st_mode & 0777;
-  int to_mode = target_stat->st_mode & 0777;
-
-  /* Fix up permissions before we potentially lose ownership with fchown.
-     Clear the setxid bits because in case the fchown below fails then we don't
-     want to end up with a sxid file owned by the invoking user.  If the user
-     hasn't changed or if fchown succeeded, we add back the sxid bits at the
-     end.  */
-  if (from_mode != to_mode)
-    fchmod (fd, to_mode);
-
-  /* Fix up ownership, this will clear the setxid bits.  */
-  if (from_stat.st_uid != target_stat->st_uid
-      || from_stat.st_gid != target_stat->st_gid)
-    ret = fchown (fd, target_stat->st_uid, target_stat->st_gid);
-
-  /* Fix up the sxid bits if either the fchown wasn't needed or it
-     succeeded.  */
-  if (ret == 0)
-    fchmod (fd, target_stat->st_mode & 07777);
-}
-#endif
-
-/* Rename FROM to TO, copying if TO is either a link or is not a regular file.
-   FD is an open file descriptor pointing to FROM that we can use to safely fix
-   up permissions of the file after renaming.  TARGET_STAT has the file status
-   that is used to fix up permissions and timestamps after rename.  Return 0 if
-   ok, -1 if error and FD is closed before returning.  */
+/* Rename FROM to TO, copying if TO is a link.
+   Return 0 if ok, -1 if error.  */
 
 int
-smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
-             struct stat *target_stat ATTRIBUTE_UNUSED,
-             int preserve_dates ATTRIBUTE_UNUSED)
+smart_rename (const char *from, const char *to, int preserve_dates ATTRIBUTE_UNUSED)
 {
+  bfd_boolean exists;
+  struct stat s;
   int ret = 0;
-  bfd_boolean exists = target_stat != NULL;
+
+  exists = lstat (to, &s) == 0;
 
 #if defined (_WIN32) && !defined (__CYGWIN32__)
   /* Win32, unlike unix, will not erase `to' in `rename(from, to)' but
@@ -196,35 +158,36 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
       unlink (from);
     }
 #else
-  /* Avoid a full copy and use rename if we can fix up permissions of the
-     file after renaming, i.e.:
-
-     - TO is not a symbolic link
-     - TO is a regular file with only one hard link
-     - We have permission to write to TO
-     - FD is available to safely fix up permissions to be the same as the file
-       we overwrote with the rename.
-
-     Note though that the actual file on disk that TARGET_STAT describes may
-     have changed and we're only trying to preserve the status we know about.
-     At no point do we try to interact with the new file changes, so there can
-     only be two outcomes, i.e. either the external file change survives
-     without knowledge of our change (if it happens after the rename syscall)
-     or our rename and permissions fixup survive without any knowledge of the
-     external change.  */
+  /* Use rename only if TO is not a symbolic link and has
+     only one hard link, and we have permission to write to it.  */
   if (! exists
-      || (fd >= 0
-         && !S_ISLNK (target_stat->st_mode)
-         && S_ISREG (target_stat->st_mode)
-         && (target_stat->st_mode & S_IWUSR)
-         && target_stat->st_nlink == 1)
+      || (!S_ISLNK (s.st_mode)
+         && S_ISREG (s.st_mode)
+         && (s.st_mode & S_IWUSR)
+         && s.st_nlink == 1)
       )
     {
       ret = rename (from, to);
       if (ret == 0)
        {
          if (exists)
-           try_preserve_permissions (fd, target_stat);
+           {
+             /* Try to preserve the permission bits and ownership of
+                TO.  First get the mode right except for the setuid
+                bit.  Then change the ownership.  Then fix the setuid
+                bit.  We do the chmod before the chown because if the
+                chown succeeds, and we are a normal user, we won't be
+                able to do the chmod afterward.  We don't bother to
+                fix the setuid bit first because that might introduce
+                a fleeting security problem, and because the chown
+                will clear the setuid bit anyhow.  We only fix the
+                setuid bit if the chown succeeds, because we don't
+                want to introduce an unexpected setuid file owned by
+                the user running objcopy.  */
+             chmod (to, s.st_mode & 0777);
+             if (chown (to, s.st_uid, s.st_gid) >= 0)
+               chmod (to, s.st_mode & 07777);
+           }
        }
       else
        {
@@ -240,11 +203,9 @@ smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
        non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror (errno));
 
       if (preserve_dates)
-       set_times (to, target_stat);
+       set_times (to, &s);
       unlink (from);
     }
-  if (fd >= 0)
-    close (fd);
 #endif /* _WIN32 && !__CYGWIN32__ */
 
   return ret;