[3/3] binutils: Make smart_rename safe too

Message ID 20201203144057.3897589-4-siddhesh@gotplt.org
State New
Headers show
Series
  • Safer file operations in ar and objcopy [#26945]
Related show

Commit Message

Siddhesh Poyarekar Dec. 3, 2020, 2:40 p.m.
smart_rename is capable of handling symlinks by copying and it also
tries to preserve ownership and permissions of files when they're
overwritten during the rename.  This is useful in objcopy where the
file properties need to be preserved.

However because smart_rename does this using file names, it leaves a
race window between renames and permission fixes.  This change removes
this race window by using file descriptors from the original BFDs that
were used to manipulate these files wherever possible.

The file that is to be renamed is also passed as a file descriptor so
that we use fchown/fchmod on the file descriptor, thus making sure
that we only modify the file we have opened to write.  Further, in
case the file is to be overwritten (as is the case in ar or objcopy),
the permissions that need to be restored are taken from the file
descriptor that was opened for input so that integrity of the file
status is maintained all the way through to the rename.

binutils/

	* rename.c
	* ar.c
	(write_archive) [!defined (_WIN32) || defined (__CYGWIN32__)]:
	Initialize TARGET_STAT and OFD to pass to SMART_RENAME.
	* arsup.c
	(ar_save) [defined (_WIN32) || defined (__CYGWIN32__)]:
	Likewise.
	* bucomm.h (smart_rename): Add new arguments to declaration.
	* objcopy.c
	(strip_main)[defined (_WIN32) || defined (__CYGWIN32__)]:
	Initialize COPYFD and pass to SMART_RENAME.
	(copy_main) [defined (_WIN32) || defined (__CYGWIN32__)]:
	Likewise.
	* rename.c (try_preserve_permissions): New function.
	(smart_rename): Use it and add new arguments.
---
 binutils/ar.c      |  12 +++++-
 binutils/arsup.c   |  14 ++++++-
 binutils/bucomm.h  |   3 +-
 binutils/objcopy.c |  42 +++++++++++++++----
 binutils/rename.c  | 101 +++++++++++++++++++++++++++++++--------------
 5 files changed, 130 insertions(+), 42 deletions(-)

-- 
2.26.2

Comments

Sebastian Huber Dec. 11, 2020, 7:30 a.m. | #1
Hello,

On 03/12/2020 15:40, Siddhesh Poyarekar wrote:
> +#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


I get a compiler error on FreeBSD 12 with this change:

../../sourceware-mirror-binutils-gdb-4483a8e/binutils/ar.c:1308:32: 
error: member reference base type 'void' is not a structure or union
   else if (ofd == -1 || fstat (fileno (iarch->iostream), &target_stat) 
!= 0)
                                ^~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/stdio.h:497:36: note: expanded from macro 'fileno'
#define fileno(p)       (!__isthreaded ? __sfileno(p) : (fileno)(p))
                                          ^~~~~~~~~~~~
/usr/include/stdio.h:489:26: note: expanded from macro '__sfileno'
#define __sfileno(p)    ((p)->_file)
                          ~~~^ ~~~~~
1 error generated.

Also in arsup.c:

../../sourceware-mirror-binutils-gdb-4483a8e/binutils/arsup.c:358:18: 
error: member reference base type 'void' is not a structure or union
       ofd = dup (fileno (obfd->iostream));
                  ^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/stdio.h:497:36: note: expanded from macro 'fileno'
#define fileno(p)       (!__isthreaded ? __sfileno(p) : (fileno)(p))
                                          ^~~~~~~~~~~~
/usr/include/stdio.h:489:26: note: expanded from macro '__sfileno'
#define __sfileno(p)    ((p)->_file)
                          ~~~^ ~~~~~
1 error generated.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
H.J. Lu via Binutils Dec. 11, 2020, 11:43 a.m. | #2
Hi Sebastian,

>> +  else if (ofd == -1 || fstat (fileno (iarch->iostream), &target_stat) != 0)


> I get a compiler error on FreeBSD 12 with this change:

> ../../sourceware-mirror-binutils-gdb-4483a8e/binutils/ar.c:1308:32: error: member reference base type 'void' is not a structure or union


Does this patch resolve the issue for you ?

Cheers
   Nick
diff --git a/binutils/ar.c b/binutils/ar.c
index 6598dd9012..83292232dd 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1305,7 +1305,7 @@ write_archive (bfd *iarch)
   ofd = dup (ofd);
   if (iarch == NULL || iarch->iostream == NULL)
     skip_stat = TRUE;
-  else if (ofd == -1 || fstat (fileno (iarch->iostream), &target_stat) != 0)
+  else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
     bfd_fatal (old_name);
 #endif
 
diff --git a/binutils/arsup.c b/binutils/arsup.c
index 8b4437ff41..dad4174e3c 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -355,7 +355,7 @@ ar_save (void)
 #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));
+      ofd = dup (fileno ((FILE *) obfd->iostream));
       if (lstat (real_name, &target_stat) != 0)
 	skip_stat = TRUE;
 #endif
Sebastian Huber Dec. 11, 2020, 12:10 p.m. | #3
Hello Nick,

On 11/12/2020 12:43, Nick Clifton wrote:
> Hi Sebastian,

> 

>>> +  else if (ofd == -1 || fstat (fileno (iarch->iostream), 

>>> &target_stat) != 0)

> 

>> I get a compiler error on FreeBSD 12 with this change:

>> ../../sourceware-mirror-binutils-gdb-4483a8e/binutils/ar.c:1308:32: 

>> error: member reference base type 'void' is not a structure or union

> 

> Does this patch resolve the issue for you ?


thanks for the patch. It partially resolved the issue. With the attached 
patch it works on FreeBSD 12 (objcopy.c was missing in your patch).

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
diff --git a/binutils/ar.c b/binutils/ar.c
index 6598dd90..83292232 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1305,7 +1305,7 @@ write_archive (bfd *iarch)
   ofd = dup (ofd);
   if (iarch == NULL || iarch->iostream == NULL)
     skip_stat = TRUE;
-  else if (ofd == -1 || fstat (fileno (iarch->iostream), &target_stat) != 0)
+  else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
     bfd_fatal (old_name);
 #endif
 
diff --git a/binutils/arsup.c b/binutils/arsup.c
index 8b4437ff..dad4174e 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -355,7 +355,7 @@ ar_save (void)
 #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));
+      ofd = dup (fileno ((FILE *) obfd->iostream));
       if (lstat (real_name, &target_stat) != 0)
 	skip_stat = TRUE;
 #endif
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 06ecf3e9..0ea3ea13 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -3745,7 +3745,7 @@ copy_file (const char *input_filename, const char *output_filename, int ofd,
   /* To allow us to do "strip *" without dying on the first
      non-object file, failures are nonfatal.  */
   ibfd = bfd_openr (input_filename, input_target);
-  if (ibfd == NULL || fstat (fileno (ibfd->iostream), in_stat) != 0)
+  if (ibfd == NULL || fstat (fileno ((FILE *) ibfd->iostream), in_stat) != 0)
     {
       bfd_nonfatal_message (input_filename, NULL, NULL, NULL);
       status = 1;
H.J. Lu via Binutils Dec. 11, 2020, 1:28 p.m. | #4
Hi Sebastian,

> thanks for the patch. It partially resolved the issue. With the attached patch it works on FreeBSD 12 (objcopy.c was missing in your patch).


Great - I have checked in your version of the patch.

Cheers
   Nick

binutils/ChangeLog
2020-12-11  Sebastian Huber  <sebastian.huber@embedded-brains.de>

	* ar.c (write_archive): Cast iostream pointer to FILE *.
	* arsup.c (ar_save): Likewise.
	* objcopy.c (copy_file): Likewise.

Patch

diff --git a/binutils/ar.c b/binutils/ar.c
index 225324208bd..6598dd9012c 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1254,6 +1254,8 @@  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));
@@ -1299,6 +1301,14 @@  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);
 
@@ -1308,7 +1318,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, 0) != 0)
+  if (smart_rename (new_name, old_name, ofd, skip_stat ? NULL : &target_stat, 0) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index a668f270f1a..8b4437ff417 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -345,13 +345,25 @@  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, 0);
+      smart_rename (ofilename, real_name, ofd,
+		    skip_stat ? NULL : &target_stat, 0);
       obfd = 0;
       free (ofilename);
     }
diff --git a/binutils/bucomm.h b/binutils/bucomm.h
index afb8e09c2fd..9613b922d5d 100644
--- a/binutils/bucomm.h
+++ b/binutils/bucomm.h
@@ -71,7 +71,8 @@  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);
+extern int smart_rename (const char *, const char *, int, struct stat *, int);
+
 
 /* In libiberty.  */
 void *xmalloc (size_t);
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index b6cf6ea4baa..04ba95ec140 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4815,6 +4815,7 @@  strip_main (int argc, char *argv[])
       struct stat statbuf;
       char *tmpname;
       int tmpfd = -1;
+      int copyfd = -1;
 
       if (get_file_size (argv[i]) < 1)
 	{
@@ -4828,7 +4829,12 @@  strip_main (int argc, char *argv[])
       else
 	tmpname = output_file;
 
-      if (tmpname == NULL)
+      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
+      )
 	{
 	  bfd_nonfatal_message (argv[i], NULL, NULL,
 				_("could not create temporary file to hold stripped copy"));
@@ -4846,12 +4852,18 @@  strip_main (int argc, char *argv[])
 	  if (output_file != tmpname)
 	    status = (smart_rename (tmpname,
 				    output_file ? output_file : argv[i],
-				    preserve_dates) != 0);
+				    copyfd, &statbuf, preserve_dates) != 0);
 	  if (status == 0)
 	    status = hold_status;
 	}
       else
-	unlink_if_ordinary (tmpname);
+	{
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+	  if (copyfd >= 0)
+	    close (copyfd);
+#endif
+	  unlink_if_ordinary (tmpname);
+	}
       if (output_file != tmpname)
 	free (tmpname);
     }
@@ -5059,6 +5071,7 @@  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;
 
@@ -5903,9 +5916,16 @@  copy_main (int argc, char *argv[])
   else
     tmpname = output_filename;
 
-  if (tmpname == NULL)
-    fatal (_("warning: could not create temporary file whilst copying '%s', (error: %s)"),
-	   input_filename, strerror (errno));
+  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));
+    }
 
   copy_file (input_filename, tmpname, tmpfd, &statbuf, input_target,
 	     output_target, input_arch);
@@ -5914,11 +5934,17 @@  copy_main (int argc, char *argv[])
       if (preserve_dates)
 	set_times (tmpname, &statbuf);
       if (tmpname != output_filename)
-	status = (smart_rename (tmpname, input_filename,
+	status = (smart_rename (tmpname, input_filename, copyfd, &statbuf,
 				preserve_dates) != 0);
     }
   else
-    unlink_if_ordinary (tmpname);
+    {
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+      if (copyfd >= 0)
+	close (copyfd);
+#endif
+      unlink_if_ordinary (tmpname);
+    }
 
   if (tmpname != output_filename)
     free (tmpname);
diff --git a/binutils/rename.c b/binutils/rename.c
index bf3b68d0462..6b9165ea1c9 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -131,17 +131,55 @@  set_times (const char *destination, const struct stat *statbuf)
 #endif
 #endif
 
-/* Rename FROM to TO, copying if TO is a link.
-   Return 0 if ok, -1 if error.  */
+#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.  */
 
 int
-smart_rename (const char *from, const char *to, int preserve_dates ATTRIBUTE_UNUSED)
+smart_rename (const char *from, const char *to, int fd ATTRIBUTE_UNUSED,
+	      struct stat *target_stat ATTRIBUTE_UNUSED,
+	      int preserve_dates ATTRIBUTE_UNUSED)
 {
-  bfd_boolean exists;
-  struct stat s;
   int ret = 0;
-
-  exists = lstat (to, &s) == 0;
+  bfd_boolean exists = target_stat != NULL;
 
 #if defined (_WIN32) && !defined (__CYGWIN32__)
   /* Win32, unlike unix, will not erase `to' in `rename(from, to)' but
@@ -158,36 +196,35 @@  smart_rename (const char *from, const char *to, int preserve_dates ATTRIBUTE_UNU
       unlink (from);
     }
 #else
-  /* Use rename only if TO is not a symbolic link and has
-     only one hard link, and we have permission to write to it.  */
+  /* 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.  */
   if (! exists
-      || (!S_ISLNK (s.st_mode)
-	  && S_ISREG (s.st_mode)
-	  && (s.st_mode & S_IWUSR)
-	  && s.st_nlink == 1)
+      || (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)
       )
     {
       ret = rename (from, to);
       if (ret == 0)
 	{
 	  if (exists)
-	    {
-	      /* 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);
-	    }
+	    try_preserve_permissions (fd, target_stat);
 	}
       else
 	{
@@ -203,9 +240,11 @@  smart_rename (const char *from, const char *to, int preserve_dates ATTRIBUTE_UNU
 	non_fatal (_("unable to copy file '%s'; reason: %s"), to, strerror (errno));
 
       if (preserve_dates)
-	set_times (to, &s);
+	set_times (to, target_stat);
       unlink (from);
     }
+  if (fd >= 0)
+    close (fd);
 #endif /* _WIN32 && !__CYGWIN32__ */
 
   return ret;