[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