binutils: Attempt to retain permissions on copying file
diff mbox series

Message ID 20210223065259.878257-1-siddhesh@gotplt.org
State New
Headers show
Series
  • binutils: Attempt to retain permissions on copying file
Related show

Commit Message

Siddhesh Poyarekar Feb. 23, 2021, 6:52 a.m. 
Writing into an existing file clears its S_ISUID and S_ISGID bits.
Attempt to restore those permission bits but don't fail if it doesn't
work.

Also, since the output file always exists (all callers create an empty
file before calling smart_rename), open the file without any
permission hints or O_CREAT.

binutils/

	* rename.c (simple_copy): Don't use O_CREAT.
	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:
	Attempt to retain permission bits.
---

Tested on x86_64 to verify that it retains setuid/setgid bits directly
as well as via symlink.  The patch goes on top of Alan Modra's patch to
tidy up for Windows and unconditionally call simple_copy so that this
(hopefully) doesn't break WIN32 again:

https://sourceware.org/pipermail/binutils/2021-February/115472.html

 binutils/rename.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

-- 
2.29.2

Comments

H.J. Lu via Binutils Feb. 23, 2021, 11:56 a.m. | #1 
On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:
> Writing into an existing file clears its S_ISUID and S_ISGID bits.

> Attempt to restore those permission bits but don't fail if it doesn't

> work.

> 

> Also, since the output file always exists (all callers create an empty

> file before calling smart_rename), open the file without any

> permission hints or O_CREAT.

> 

> binutils/

> 

> 	* rename.c (simple_copy): Don't use O_CREAT.

> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:

> 	Attempt to retain permission bits.


Thanks, I'll fold this into a followup patch of mine that makes use of
the temp file descriptor in smart_rename rather than reopening the
file.  I don't believe there is a security issue in reopening the
file, but this way there is one less directory operation.  I'm also
going to make use of the target_stat we already have rather than
calling stat again.

	* bucomm.h (smart_rename): Update prototype.
	* rename.c (smart_rename): Add fromfd and preserve_dates params.
	Pass fromfd and target_stat to simple_copy.  Call set_times
	when preserve_dates.
	(simple_copy): Accept fromfd rather than from filename.  Add
	target_stat param.  Rewind fromfd rather than opening.  Open
	"to" file without O_CREAT.  Try to preserve S_ISUID and S_ISGID.
	* ar.c (write_archive): Rename ofd to tmpfd.  Dup tmpfd before
	closing output temp file, and pass tmpfd to smart_rename.
	* arsup.c (temp_fd): Rename from real_fd.
	(ar_save): Dup temp_fd and pass to smart_rename.
	* objcopy.c (strip_main, copy_main): Likewise, and pass
	preserve_dates.

diff --git a/binutils/ar.c b/binutils/ar.c
index 44df48c5c6..fb19b14fec 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -1252,21 +1252,21 @@ write_archive (bfd *iarch)
   bfd *obfd;
   char *old_name, *new_name;
   bfd *contents_head = iarch->archive_next;
-  int ofd = -1;
+  int tmpfd = -1;
 
   old_name = xstrdup (bfd_get_filename (iarch));
-  new_name = make_tempname (old_name, &ofd);
+  new_name = make_tempname (old_name, &tmpfd);
 
   if (new_name == NULL)
     bfd_fatal (_("could not create temporary file whilst writing archive"));
 
   output_filename = new_name;
 
-  obfd = bfd_fdopenw (new_name, bfd_get_target (iarch), ofd);
+  obfd = bfd_fdopenw (new_name, bfd_get_target (iarch), tmpfd);
 
   if (obfd == NULL)
     {
-      close (ofd);
+      close (tmpfd);
       bfd_fatal (old_name);
     }
 
@@ -1297,6 +1297,7 @@ write_archive (bfd *iarch)
   if (!bfd_set_archive_head (obfd, contents_head))
     bfd_fatal (old_name);
 
+  tmpfd = dup (tmpfd);
   if (!bfd_close (obfd))
     bfd_fatal (old_name);
 
@@ -1306,7 +1307,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, NULL) != 0)
+  if (smart_rename (new_name, old_name, tmpfd, NULL, FALSE) != 0)
     xexit (1);
   free (old_name);
   free (new_name);
diff --git a/binutils/arsup.c b/binutils/arsup.c
index f7ce8f0bc8..9982484dbe 100644
--- a/binutils/arsup.c
+++ b/binutils/arsup.c
@@ -43,7 +43,7 @@ extern int deterministic;
 static bfd *obfd;
 static char *real_name;
 static char *temp_name;
-static int real_ofd;
+static int temp_fd;
 static FILE *outfile;
 
 static void
@@ -152,7 +152,7 @@ void
 ar_open (char *name, int t)
 {
   real_name = xstrdup (name);
-  temp_name = make_tempname (real_name, &real_ofd);
+  temp_name = make_tempname (real_name, &temp_fd);
 
   if (temp_name == NULL)
     {
@@ -162,7 +162,7 @@ ar_open (char *name, int t)
       return;
     }
 
-  obfd = bfd_fdopenw (temp_name, NULL, real_ofd);
+  obfd = bfd_fdopenw (temp_name, NULL, temp_fd);
 
   if (!obfd)
     {
@@ -348,6 +348,7 @@ ar_save (void)
       if (deterministic > 0)
         obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
 
+      temp_fd = dup (temp_fd);
       bfd_close (obfd);
 
       if (stat (real_name, &target_stat) != 0)
@@ -363,7 +364,7 @@ ar_save (void)
 	    }
 	}
 
-      smart_rename (temp_name, real_name, NULL);
+      smart_rename (temp_name, real_name, temp_fd, NULL, FALSE);
       obfd = 0;
       free (temp_name);
       free (real_name);
diff --git a/binutils/bucomm.h b/binutils/bucomm.h
index aa7e33d8cd..f1ae47fa1b 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 *, struct stat *);
+extern int smart_rename (const char *, const char *, int,
+			 struct stat *, bfd_boolean);
 
 
 /* In libiberty.  */
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index abbcb7c519..90ae0bd46b 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4837,6 +4837,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)
 	{
@@ -4846,7 +4847,11 @@ strip_main (int argc, char *argv[])
 
       if (output_file == NULL
 	  || filename_cmp (argv[i], output_file) == 0)
-	tmpname = make_tempname (argv[i], &tmpfd);
+	{
+	  tmpname = make_tempname (argv[i], &tmpfd);
+	  if (tmpfd >= 0)
+	    copyfd = dup (tmpfd);
+	}
       else
 	tmpname = output_file;
 
@@ -4864,14 +4869,18 @@ strip_main (int argc, char *argv[])
       if (status == 0)
 	{
 	  if (output_file != tmpname)
-	    status = (smart_rename (tmpname,
-				    output_file ? output_file : argv[i],
-				    preserve_dates ? &statbuf : NULL) != 0);
+	    status = smart_rename (tmpname,
+				   output_file ? output_file : argv[i],
+				   copyfd, &statbuf, preserve_dates) != 0;
 	  if (status == 0)
 	    status = hold_status;
 	}
       else
-	unlink_if_ordinary (tmpname);
+	{
+	  if (copyfd >= 0)
+	    close (copyfd);
+	  unlink_if_ordinary (tmpname);
+	}
       if (output_file != tmpname)
 	free (tmpname);
     }
@@ -5078,7 +5087,9 @@ copy_main (int argc, char *argv[])
   bfd_boolean formats_info = FALSE;
   bfd_boolean use_globalize = FALSE;
   bfd_boolean use_keep_global = FALSE;
-  int c, tmpfd = -1;
+  int c;
+  int tmpfd = -1;
+  int copyfd;
   struct stat statbuf;
   const bfd_arch_info_type *input_arch = NULL;
 
@@ -5916,10 +5927,15 @@ copy_main (int argc, char *argv[])
     }
 
   /* If there is no destination file, or the source and destination files
-     are the same, then create a temp and rename the result into the input.  */
+     are the same, then create a temp and copy the result into the input.  */
+  copyfd = -1;
   if (output_filename == NULL
       || filename_cmp (input_filename, output_filename) == 0)
-    tmpname = make_tempname (input_filename, &tmpfd);
+    {
+      tmpname = make_tempname (input_filename, &tmpfd);
+      if (tmpfd >= 0)
+	copyfd = dup (tmpfd);
+    }
   else
     tmpname = output_filename;
 
@@ -5934,11 +5950,15 @@ copy_main (int argc, char *argv[])
   if (status == 0)
     {
       if (tmpname != output_filename)
-	status = (smart_rename (tmpname, input_filename,
-				preserve_dates ? &statbuf : NULL) != 0);
+	status = smart_rename (tmpname, input_filename, copyfd,
+			       &statbuf, preserve_dates) != 0;
     }
   else
-    unlink_if_ordinary (tmpname);
+    {
+      if (copyfd >= 0)
+	close (copyfd);
+      unlink_if_ordinary (tmpname);
+    }
 
   if (tmpname != output_filename)
     free (tmpname);
diff --git a/binutils/rename.c b/binutils/rename.c
index 72a9323d72..f688f350d5 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -31,24 +31,21 @@
 /* The number of bytes to copy at once.  */
 #define COPY_BUF 8192
 
-/* Copy file FROM to file TO, performing no translations.
+/* Copy file FROMFD to file TO, performing no translations.
    Return 0 if ok, -1 if error.  */
 
 static int
-simple_copy (const char *from, const char *to)
+simple_copy (int fromfd, const char *to, struct stat *target_stat)
 {
-  int fromfd, tofd, nread;
+  int tofd, nread;
   int saved;
   char buf[COPY_BUF];
 
-  fromfd = open (from, O_RDONLY | O_BINARY);
-  if (fromfd < 0)
+  if (fromfd < 0
+      || lseek (fromfd, 0, SEEK_SET) != 0)
     return -1;
-#ifdef O_CREAT
-  tofd = open (to, O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0777);
-#else
-  tofd = creat (to, 0777);
-#endif
+
+  tofd = open (to, O_WRONLY | O_TRUNC | O_BINARY);
   if (tofd < 0)
     {
       saved = errno;
@@ -56,6 +53,7 @@ simple_copy (const char *from, const char *to)
       errno = saved;
       return -1;
     }
+
   while ((nread = read (fromfd, buf, sizeof buf)) > 0)
     {
       if (write (tofd, buf, nread) != nread)
@@ -67,7 +65,16 @@ simple_copy (const char *from, const char *to)
 	  return -1;
 	}
     }
+
   saved = errno;
+
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Writing to a setuid/setgid file may clear S_ISUID and S_ISGID.
+     Try to restore them, ignoring failure.  */
+  if (target_stat != NULL)
+    fchmod (tofd, target_stat->st_mode);
+#endif
+
   close (fromfd);
   close (tofd);
   if (nread < 0)
@@ -118,17 +125,17 @@ set_times (const char *destination, const struct stat *statbuf)
    various systems.  So now we just copy.  */
 
 int
-smart_rename (const char *from, const char *to,
-	      struct stat *target_stat)
+smart_rename (const char *from, const char *to, int fromfd,
+	      struct stat *target_stat, bfd_boolean preserve_dates)
 {
   int ret;
 
-  ret = simple_copy (from, to);
+  ret = simple_copy (fromfd, to, target_stat);
   if (ret != 0)
     non_fatal (_("unable to copy file '%s'; reason: %s"),
 	       to, strerror (errno));
 
-  if (target_stat != NULL)
+  if (preserve_dates)
     set_times (to, target_stat);
   unlink (from);
 


-- 
Alan Modra
Australia Development Lab, IBM
H.J. Lu via Binutils Feb. 23, 2021, 6:49 p.m. | #2 
On 23.02.2021 07:52, Siddhesh Poyarekar wrote:
> Writing into an existing file clears its S_ISUID and S_ISGID bits.

> Attempt to restore those permission bits but don't fail if it doesn't

> work.

> 

> Also, since the output file always exists (all callers create an empty

> file before calling smart_rename), open the file without any

> permission hints or O_CREAT.

> 

> binutils/

> 

> 	* rename.c (simple_copy): Don't use O_CREAT.

> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:

> 	Attempt to retain permission bits.



pay attention that __CYGWIN32__ is deprecated and
instead should be used __CYGWIN__

it exists only for obsolete compatibility

on 32bit platform

$ gcc -E -dM - </dev/null | grep CYGWIN
#define __CYGWIN__ 1
#define __CYGWIN32__ 1

on 64 bit

$ gcc -E -dM - </dev/null | grep CYGWIN
#define __CYGWIN__ 1
Siddhesh Poyarekar Feb. 24, 2021, 10:27 a.m. | #3 
On 2/23/21 5:26 PM, Alan Modra wrote:
> On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:

>> Writing into an existing file clears its S_ISUID and S_ISGID bits.

>> Attempt to restore those permission bits but don't fail if it doesn't

>> work.

>>

>> Also, since the output file always exists (all callers create an empty

>> file before calling smart_rename), open the file without any

>> permission hints or O_CREAT.

>>

>> binutils/

>>

>> 	* rename.c (simple_copy): Don't use O_CREAT.

>> 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:

>> 	Attempt to retain permission bits.

> 

> Thanks, I'll fold this into a followup patch of mine that makes use of

> the temp file descriptor in smart_rename rather than reopening the

> file.  I don't believe there is a security issue in reopening the

> file, but this way there is one less directory operation.  I'm also

> going to make use of the target_stat we already have rather than

> calling stat again.


I just tested tip and it looks in good shape.  I suppose this ought to 
go into 2.36 too since the simple_copy patch went in there too.

Thanks,
Siddhesh
H.J. Lu via Binutils Feb. 24, 2021, 11:47 p.m. | #4 
On Wed, Feb 24, 2021 at 03:57:10PM +0530, Siddhesh Poyarekar wrote:
> On 2/23/21 5:26 PM, Alan Modra wrote:

> > On Tue, Feb 23, 2021 at 12:22:59PM +0530, Siddhesh Poyarekar wrote:

> > > Writing into an existing file clears its S_ISUID and S_ISGID bits.

> > > Attempt to restore those permission bits but don't fail if it doesn't

> > > work.

> > > 

> > > Also, since the output file always exists (all callers create an empty

> > > file before calling smart_rename), open the file without any

> > > permission hints or O_CREAT.

> > > 

> > > binutils/

> > > 

> > > 	* rename.c (simple_copy): Don't use O_CREAT.

> > > 	(simple_copy)[!defined (_WIN32) || defined (__CYGWIN32__)]:

> > > 	Attempt to retain permission bits.

> > 

> > Thanks, I'll fold this into a followup patch of mine that makes use of

> > the temp file descriptor in smart_rename rather than reopening the

> > file.  I don't believe there is a security issue in reopening the

> > file, but this way there is one less directory operation.  I'm also

> > going to make use of the target_stat we already have rather than

> > calling stat again.

> 

> I just tested tip and it looks in good shape.  I suppose this ought to go

> into 2.36 too since the simple_copy patch went in there too.


I thought we had reverted all the rename.c changes on the branch?

Anyway, the following needs to go on mainline, illustrating why it's
not a good idea to rush patches into a stable branch.  Applied.

	PR 27456
	* rename.c (simple_copy): Mark target_stat ATTRIBUTE_UNUSED.

diff --git a/binutils/rename.c b/binutils/rename.c
index f688f350d5..861c2b56d1 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -35,7 +35,8 @@
    Return 0 if ok, -1 if error.  */
 
 static int
-simple_copy (int fromfd, const char *to, struct stat *target_stat)
+simple_copy (int fromfd, const char *to,
+	     struct stat *target_stat ATTRIBUTE_UNUSED)
 {
   int tofd, nread;
   int saved;


-- 
Alan Modra
Australia Development Lab, IBM
Siddhesh Poyarekar Feb. 25, 2021, 2:28 a.m. | #5 
On 2/25/21 5:17 AM, Alan Modra wrote:
> I thought we had reverted all the rename.c changes on the branch?

> 

> Anyway, the following needs to go on mainline, illustrating why it's

> not a good idea to rush patches into a stable branch.  Applied.


Got it, I'll keep that in mind for future changes.

Thanks,
Siddhesh
H.J. Lu via Binutils Feb. 26, 2021, 7:33 a.m. | #6 
I've pushed a series of the smart_rename patches to the branch in
order to correct the current failure to compile on MinGW.

-- 
Alan Modra
Australia Development Lab, IBM

Patch
diff mbox series 

diff --git a/binutils/rename.c b/binutils/rename.c
index 72a9323d72c..103f92866cd 100644
--- a/binutils/rename.c
+++ b/binutils/rename.c
@@ -41,14 +41,19 @@  simple_copy (const char *from, const char *to)
   int saved;
   char buf[COPY_BUF];
 
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Note permissions of the destination file before it is written to so that
+     we can try to restore it later.  */
+  struct stat to_stat;
+  mode_t to_mode = 0;
+  if (stat (to, &to_stat) == 0)
+    to_mode = to_stat.st_mode;
+#endif
+
   fromfd = open (from, O_RDONLY | O_BINARY);
   if (fromfd < 0)
     return -1;
-#ifdef O_CREAT
-  tofd = open (to, O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0777);
-#else
-  tofd = creat (to, 0777);
-#endif
+  tofd = open (to, O_WRONLY | O_TRUNC | O_BINARY);
   if (tofd < 0)
     {
       saved = errno;
@@ -67,7 +72,16 @@  simple_copy (const char *from, const char *to)
 	  return -1;
 	}
     }
+
   saved = errno;
+
+#if !defined (_WIN32) || defined (__CYGWIN32__)
+  /* Writing to a setuid/setgid file clears the S_ISUID and S_ISGID bits.
+     Try to restore them (ignore failure) before closing the file.  */
+  if (to_mode > 0)
+    fchmod (tofd, to_mode);
+#endif
+
   close (fromfd);
   close (tofd);
   if (nread < 0)