[PATCH] Reopen output file when writing archive output [BZ #27270][BZ #27284]
Siddhesh Poyarekar
siddhesh@gotplt.org
Mon Feb 1 09:25:01 GMT 2021
On 2/1/21 1:46 PM, Alan Modra wrote:
> On Mon, Feb 01, 2021 at 11:02:30AM +0530, Siddhesh Poyarekar wrote:
>> --- a/binutils/ar.c
>> +++ b/binutils/ar.c
>> @@ -1303,7 +1303,7 @@ write_archive (bfd *iarch)
>>
>> #if !defined (_WIN32) || defined (__CYGWIN32__)
>> ofd = dup (ofd);
>> - if (iarch == NULL || iarch->iostream == NULL)
>> + if (iarch->iostream == NULL && bfd_open_file (iarch) == NULL)
>> skip_stat = TRUE;
>> else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
>> bfd_fatal (old_name);
>
> This isn't the right way to go about reopening a file, and you shouldn't
> be including libbfd.h anywhere except in bfd/. See the notice:
> (This include file is not for users of the library.)
Ahh sorry, I didn't see that.
> In fact, the more I look at this code the less I like it. I think
> you've also broken _WIN32 builds as you won't be calling fstat but
> will pass an uninitialised target_stat to smart_rename. So in
WIN32 doesn't use the contents of target_stat, only the pointer to
indicate whether the file exists, since that's all it needs. Anyway, I
agree it's future-proof to keep it initialized for WIN32 as well in the
event that it uses the value in future.
> partially fixing a potential security bug we've broken binutils in
> multiple ways. (It's only a partial fix because the ar -M mri mode
> script support in arsup.c wasn't changed to use the new bfd_fdopenw.)
That doesn't really affect CVE-2021-20197 (which is specifically about
being able to trick smart_rename into setting permissions of arbitrary
files via symlinks), but I agree it's a good cleanup.
> Here's what I just threw together, as yet untested. Please review for
> any security faults still present. bfd_stat will do fstat on the file
> descriptor in this usage.
I've tested this patch and can verify that it fixes the breakage. It
looks good to me, thanks for your help!
Siddhesh
> diff --git a/binutils/ar.c b/binutils/ar.c
> index 24ff0920f40..0ecfa337228 100644
> --- a/binutils/ar.c
> +++ b/binutils/ar.c
> @@ -25,7 +25,6 @@
>
> #include "sysdep.h"
> #include "bfd.h"
> -#include "libbfd.h"
> #include "libiberty.h"
> #include "progress.h"
> #include "getopt.h"
> @@ -1255,10 +1254,8 @@ write_archive (bfd *iarch)
> 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));
> + old_name = xstrdup (bfd_get_filename (iarch));
> new_name = make_tempname (old_name, &ofd);
>
> if (new_name == NULL)
> @@ -1303,11 +1300,9 @@ write_archive (bfd *iarch)
>
> #if !defined (_WIN32) || defined (__CYGWIN32__)
> ofd = dup (ofd);
> - if (iarch == NULL || iarch->iostream == NULL)
> - skip_stat = TRUE;
> - else if (ofd == -1 || fstat (fileno ((FILE *) iarch->iostream), &target_stat) != 0)
> - bfd_fatal (old_name);
> #endif
> + if (ofd == -1 || bfd_stat (iarch, &target_stat) != 0)
> + bfd_fatal (old_name);
>
> if (!bfd_close (obfd))
> bfd_fatal (old_name);
> @@ -1318,7 +1313,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, ofd, &target_stat, 0) != 0)
> xexit (1);
> free (old_name);
> free (new_name);
> diff --git a/binutils/arsup.c b/binutils/arsup.c
> index 837011bdfd2..d4c98995e28 100644
> --- a/binutils/arsup.c
> +++ b/binutils/arsup.c
> @@ -42,6 +42,8 @@ extern int deterministic;
>
> static bfd *obfd;
> static char *real_name;
> +static char *temp_name;
> +static int real_ofd;
> static FILE *outfile;
>
> static void
> @@ -149,27 +151,24 @@ maybequit (void)
> void
> ar_open (char *name, int t)
> {
> - char *tname;
> - const char *bname = lbasename (name);
> - real_name = name;
> + real_name = xstrdup (name);
> + temp_name = make_tempname (real_name, &real_ofd);
>
> - /* Prepend tmp- to the beginning, to avoid file-name clashes after
> - truncation on filesystems with limited namespaces (DOS). */
> - if (asprintf (&tname, "%.*stmp-%s", (int) (bname - name), name, bname) == -1)
> + if (temp_name == NULL)
> {
> - fprintf (stderr, _("%s: Can't allocate memory for temp name (%s)\n"),
> + fprintf (stderr, _("%s: Can't open temporary file (%s)\n"),
> program_name, strerror(errno));
> maybequit ();
> return;
> }
>
> - obfd = bfd_openw (tname, NULL);
> + obfd = bfd_fdopenw (temp_name, NULL, real_ofd);
>
> if (!obfd)
> {
> fprintf (stderr,
> _("%s: Can't open output archive %s\n"),
> - program_name, tname);
> + program_name, temp_name);
>
> maybequit ();
> }
> @@ -344,10 +343,9 @@ ar_save (void)
> }
> else
> {
> - char *ofilename = xstrdup (bfd_get_filename (obfd));
> bfd_boolean skip_stat = FALSE;
> struct stat target_stat;
> - int ofd = -1;
> + int ofd = real_ofd;
>
> if (deterministic > 0)
> obfd->flags |= BFD_DETERMINISTIC_OUTPUT;
> @@ -355,17 +353,18 @@ 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 ((FILE *) obfd->iostream));
> - if (lstat (real_name, &target_stat) != 0)
> - skip_stat = TRUE;
> + ofd = dup (ofd);
> #endif
> -
> bfd_close (obfd);
>
> - smart_rename (ofilename, real_name, ofd,
> + if (ofd == -1 || fstat (ofd, &target_stat) != 0)
> + skip_stat = TRUE;
> +
> + smart_rename (temp_name, real_name, ofd,
> skip_stat ? NULL : &target_stat, 0);
> obfd = 0;
> - free (ofilename);
> + free (temp_name);
> + free (real_name);
> }
> }
>
> diff --git a/binutils/objcopy.c b/binutils/objcopy.c
> index 39a1ccbd7fe..0e1047e7482 100644
> --- a/binutils/objcopy.c
> +++ b/binutils/objcopy.c
> @@ -20,7 +20,6 @@
>
> #include "sysdep.h"
> #include "bfd.h"
> -#include "libbfd.h"
> #include "progress.h"
> #include "getopt.h"
> #include "libiberty.h"
> @@ -3768,7 +3767,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 ((FILE *) ibfd->iostream), in_stat) != 0)
> + if (ibfd == NULL || bfd_stat (ibfd, in_stat) != 0)
> {
> bfd_nonfatal_message (input_filename, NULL, NULL, NULL);
> status = 1;
>
>
>
More information about the Binutils
mailing list