Message ID | 20210223065259.878257-1-siddhesh@gotplt.org |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
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
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
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
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
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
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
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)