Bug 21787 - Heap-use-after-free in bfd_cache_close
Summary: Heap-use-after-free in bfd_cache_close
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.30
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-19 01:07 UTC by Ned Williamson
Modified: 2018-07-02 14:31 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
testcase (141 bytes, application/x-archive)
2017-07-19 01:07 UTC, Ned Williamson
Details
crash state (1.21 KB, text/plain)
2017-07-19 01:08 UTC, Ned Williamson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Williamson 2017-07-19 01:07:35 UTC
Created attachment 10272 [details]
testcase

A heap-use-after-free is triggered when the provided file is scanned using `./objdump -x use-after-free`. All targets must be enabled; I believe the bug is in the XCOFF archive format.

I used ASAN to detect the crash. If you have trouble reproducing this (valgrind may work as well, but I haven't checked), let me know.
Comment 1 Ned Williamson 2017-07-19 01:08:17 UTC
Created attachment 10273 [details]
crash state

ASAN output for crashing state.
Comment 2 cvs-commit@gcc.gnu.org 2017-07-19 13:50:25 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=909e4e716c4d77e33357bbe9bc902bfaf2e1af24

commit 909e4e716c4d77e33357bbe9bc902bfaf2e1af24
Author: Nick Clifton <nickc@redhat.com>
Date:   Wed Jul 19 14:49:12 2017 +0100

    Fix use-after-free error when parsing a corrupt nested archive.
    
    	PR 21787
    	* archive.c (bfd_generic_archive_p): If the bfd does not have the
    	correct magic bytes at the start, set the error to wrong format
    	and clear the format selector before returning NULL.
Comment 3 Nick Clifton 2017-07-19 13:59:55 UTC
Hi Ned,

  Thanks for reporting this bug.

  The problem is an interesting one because the code is actually set up to
  prevent the use-after-free from happening.  In the normal course of events
  when an element of an archive is no longer needed and the memory is freed,
  all pointers to it will be tidied away.  But in this case, because the
  archive is corrupt, the wrong set of memory releasing functions are being
  used and the pointers are not tidied away.  Later on, when another element
  is freed, the stale pointer is encountered and the bug triggered.

  I have checked in a patch to fix the problem.  It makes the test for a 
  genuine archive be more aggressive when it signals that an archive is 
  corrupt, forcing the format matching mechanism to declare it as unrecognised
  long before it can cause any memory corruption.

Cheers
  Nick
Comment 4 cvs-commit@gcc.gnu.org 2017-09-04 15:15:54 UTC
The binutils-2_29-branch branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a541116faa5716d5c1a4bc9d1567855902b89a87

commit a541116faa5716d5c1a4bc9d1567855902b89a87
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Sep 4 16:14:19 2017 +0100

    Import patch from mainline to improve reporting of corrupt archives.
    
    	PR 21787
    	* archive.c (bfd_generic_archive_p): If the bfd does not have the
    	correct magic bytes at the start, set the error to wrong format
    	and clear the format selector before returning NULL.
Comment 5 cvs-commit@gcc.gnu.org 2018-07-02 14:31:33 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=eac61af65bcd24a48633da375527eb3f36ab47ed

commit eac61af65bcd24a48633da375527eb3f36ab47ed
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Jun 28 08:02:42 2018 -0600

    Allow BFD to recognize macOS universal libraries
    
    Bug #13157 is about a gdb regression, where previously it could handle
    universal libraries, but now cannot.
    
    gdb isn't working for me on macOS for other reasons, so I wrote this
    small test program to show the problem:
    
        #include <config.h>
        #include <stdio.h>
        #include <stdlib.h>
        #include <bfd.h>
    
        void
        die (const char *what)
        {
          fprintf (stderr, "die: %s\n", what);
          exit (1);
        }
    
        int
        main (int argc, char **argv)
        {
          bfd *file = bfd_openr (argv[1], NULL);
          if (file == NULL)
    	die ("couldn't open");
    
          if (!bfd_check_format (file, bfd_archive))
    	die ("not an archive");
    
          printf ("yay\n");
    
          bfd_close (file);
          return 0;
        }
    
    Then I built a simple universal binary.  With git master BFD, I get:
    
        $ ./doit ./universal-exe
        die: not an archive
    
    Jeff Muizelaar tracked this down to the BFD change for PR binutils/21787.
    This patch changed bfd_generic_archive_p to sometimes reset the BFD's
    "format" field.
    
    However, simply changing bfd_generic_archive_p regressed the test case
    in that bug.
    
    Debugging PR binutils/21787 again, what I saw is that the mach-o
    universal binary support acts like a bfd_archive but does not provide
    a _close_and_cleanup function.  However, if a BFD appears as an
    archive member, it must always remove its own entry from its parent's
    map.  Otherwise, when the parent is destroyed, the already-destroyed
    child BFD will be referenced.  mach-o does not use the usual archive
    member support, so simply using _bfd_archive_close_and_cleanup (as
    other targets do) will not work.
    
    This patch fixes the problem by introducing a new
    _bfd_unlink_from_archive_parent function, then arranging for it to be
    called in the mach-o case.
    
    Ok?
    
    bfd/ChangeLog
    2018-07-02  Jeff Muizelaar  <jrmuizel@gmail.com>
    	    Tom Tromey  <tom@tromey.com>
    
    	PR 13157
    	PR 21787
    	* mach-o.c (bfd_mach_o_fat_close_and_cleanup): New function.
    	(bfd_mach_o_close_and_cleanup): Redefine.
    	* archive.c (_bfd_unlink_from_archive_parent): New function,
    	extracted from..
    	(_bfd_archive_close_and_cleanup): ..here.
    	(bfd_generic_archive_p): Do not clear archive's format.
    	* libbfd-in.h (_bfd_unlink_from_archive_parent): Declare.
    	* libbfd.h: Regenerate.