[Bug binutils/25308] New: Multiple null pointer dereferencs in bfd module due to not checking return value of bfd_malloc

[v.manhnd at vincss dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=25308

            Bug ID: 25308
           Summary: Multiple null pointer dereferencs in bfd module due to
                    not checking return value of bfd_malloc
           Product: binutils
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: binutils
          Assignee: unassigned at sourceware dot org
          Reporter: v.manhnd at vincss dot net
  Target Milestone: ---

Created attachment 12141
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12141&action=edit
Crashes due to not checking return value of bfd_malloc

There are multiple null pointer dereferences in bfd module due to calling
bfd_malloc without checking if the return pointer is null.

## Analysis
Here is the list of vulnerable issues:
### Issue 1
In bfd/pef.c:bfd_pef_scan_start_address(), loaderbuf is malloced without
checking if the return pointer is null. Later, loaderbuf is feeded to bfd_read
=> Null pointer derefence.
--------------------------------------
  loaderbuf = bfd_malloc (loaderlen);
  if (bfd_seek (abfd, loadersec->filepos, SEEK_SET) < 0)
    goto error;
  if (bfd_bread ((void *) loaderbuf, loaderlen, abfd) != loaderlen)
    goto error;
--------------------------------------
### Issue 2, 3
In bfd/pef.c:bfd_pef_parse_function_stubs(), libraries and imports are malloced
without checking the return values. Later, they are passed to
bfd_pef_parse_imported_library and bfd_pef_parse_imported_symbol:
--------------------------------------
  libraries = bfd_malloc
    (header.imported_library_count * sizeof (bfd_pef_imported_library));
  imports = bfd_malloc
    (header.total_imported_symbol_count * sizeof (bfd_pef_imported_symbol));
...
      ret = bfd_pef_parse_imported_library
        (abfd, loaderbuf + 56 + (i * 24), 24, &libraries[i]);
...
      ret = (bfd_pef_parse_imported_symbol
             (abfd,
              loaderbuf + 56 + (header.imported_library_count * 24) + (i * 4),
              4, &imports[i]));
--------------------------------------
### Issue 4, 5
In bfd, pef.c:bfd_pef_parse_symbols(), codebuf and loadersec are malloced
without checking the return values. Later, they are passed to bfd_read:
--------------------------------------
  codesec = bfd_get_section_by_name (abfd, "code");
  if (codesec != NULL)
    {
      codelen = codesec->size;
      codebuf = bfd_malloc (codelen);
      if (bfd_seek (abfd, codesec->filepos, SEEK_SET) < 0)
        goto end;
      if (bfd_bread ((void *) codebuf, codelen, abfd) != codelen)
        goto end;
    }

  loadersec = bfd_get_section_by_name (abfd, "loader");
  if (loadersec != NULL)
    {
      loaderlen = loadersec->size;
      loaderbuf = bfd_malloc (loaderlen);
      if (bfd_seek (abfd, loadersec->filepos, SEEK_SET) < 0)
        goto end;
      if (bfd_bread ((void *) loaderbuf, loaderlen, abfd) != loaderlen)
        goto end;
    }
---------------------------------------
### Issue 6
In bfd/pef.c:bfd_pef_print_loader_section(), loaderbuf is malloced without
checking the return value. Later, loaderbuf is passed to bfd_bread:
---------------------------------------
  loaderbuf = bfd_malloc (loaderlen);

  if (bfd_seek (abfd, loadersec->filepos, SEEK_SET) < 0
      || bfd_bread ((void *) loaderbuf, loaderlen, abfd) != loaderlen
      || loaderlen < 56
      || bfd_pef_parse_loader_header (abfd, loaderbuf, 56, &header) < 0)
    {
      free (loaderbuf);
      return -1;
    }
---------------------------------------
### Issue 7
In bfd/mach-o.c:bfd_mach_o_core_fetch_environment(), dsym_filename is passed to
sprintf:
---------------------------------------
  dsym_filename = (char *)bfd_malloc (strlen (base_bfd->filename)
                                       + strlen (dsym_subdir) + 1
                                       + strlen (base_basename) + 1);
  sprintf (dsym_filename, "%s%s/%s",
           base_bfd->filename, dsym_subdir, base_basename);

---------------------------------------
### Issue 8
In bfd/elf-properties.c/_bfd_elf_convert_gnu_properties(), contents is malloced
without checking the return value. Later, contents is passed to
elf_write_gnu_properties:
---------------------------------------
  if (size > bfd_section_size (isec))
    {
      contents = (bfd_byte *) bfd_malloc (size);
      free (*ptr);
      *ptr = contents;
    }
  else
    contents = *ptr;

  *ptr_size = size;

  /* Generate the output .note.gnu.property section.  */
  elf_write_gnu_properties (ibfd, contents, list, size, 1 << align_shift);
---------------------------------------
### Issue 9
In bfd/elf32-arm.c:bfd_elf32_arm_vfp11_fix_veneer_locations(), tmp_name is
malloced without checking the return value. Later, tmp_name is passed to
sprintf:
---------------------------------------
  tmp_name = (char *) bfd_malloc ((bfd_size_type) strlen
                                  (VFP11_ERRATUM_VENEER_ENTRY_NAME) + 10);
...
              sprintf (tmp_name, VFP11_ERRATUM_VENEER_ENTRY_NAME,
                       errnode->u.b.veneer->u.v.id);
----------------------------------------
### Issue 10
In bfd/elf32-arm.c:bfd_elf32_arm_stm32l4xx_fix_veneer_locations(), tmp_name is
malloced without checking the return value. Later, tmp_name is passed to
sprintf:
----------------------------------------
tmp_name = (char *) bfd_malloc ((bfd_size_type) strlen
                                  (STM32L4XX_ERRATUM_VENEER_ENTRY_NAME) + 10);
...
              sprintf (tmp_name, STM32L4XX_ERRATUM_VENEER_ENTRY_NAME,
                       errnode->u.b.veneer->u.v.id);
----------------------------------------
### Issue 11
In bfd/elf32-arm.c:elf32_arm_filter_cmse_symbols(), cmse_name is malloced
without checking the return value. Later, cmse_name is passed to snprintf:
----------------------------------------
  cmse_name = (char *) bfd_malloc (maxnamelen);
...
        snprintf (cmse_name, maxnamelen, "%s%s", CMSE_PREFIX, name);
----------------------------------------
### Issue 12
In bfd/elf32-arm.c, edited_contents is malloced without checking the return
value. Later, it is used in many places, for example, copy_exidx_entry:
----------------------------------------
      bfd_vma add_to_offsets = 0;
...
                  copy_exidx_entry (output_bfd, edited_contents + out_index *
8,
                                    contents + in_index * 8, add_to_offsets);

----------------------------------------

## PoC
There are crash PoCs for issue 1 and issue 2. They are in the attachment.
Binutils-gdb is built in 32-bit version so that it is easier for bfd_malloc to
return null.
---------------Version------------------
Tested with version 39aa149769fd05fb6fade43bd41c1d7b6d63d06b of
github.com/bminor/binutils-gdb
---------Crash log issue 1--------------
root@manh-ubuntu16:~/fuzz/fuzz_binutils# valgrind
binutils-gdb-gcc-32/binutils/objdump  -x
crash-objdump-bfd_pef_scan_start_address 
==1089== Memcheck, a memory error detector
==1089== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==1089== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==1089== Command: binutils-gdb-gcc-32/binutils/objdump -x
crash-objdump-bfd_pef_scan_start_address
==1089== 
==1089== Invalid write of size 1
==1089==    at 0x403337C: __GI_mempcpy (in
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==1089==    by 0x40D1364: _IO_file_xsgetn (fileops.c:1392)
==1089==    by 0x40D333D: _IO_sgetn (genops.c:467)
==1089==    by 0x40C68F6: fread (iofread.c:38)
==1089==    by 0x86A6739: cache_bread_1 (cache.c:319)
==1089==    by 0x86A6864: cache_bread (cache.c:358)
==1089==    by 0x820ED2E: bfd_bread (bfdio.c:211)
==1089==    by 0x8634E6B: bfd_pef_scan_start_address (pef.c:483)
==1089==    by 0x8635167: bfd_pef_scan (pef.c:556)
==1089==    by 0x8635397: bfd_pef_object_p (pef.c:605)
==1089==    by 0x8212246: bfd_check_format_matches (format.c:322)
==1089==    by 0x8052A83: display_object_bfd (objdump.c:4163)
==1089==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==1089== 
==1089== 
==1089== Process terminating with default action of signal 11 (SIGSEGV)
==1089==  Access not within mapped region at address 0x0
==1089==    at 0x403337C: __GI_mempcpy (in
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==1089==    by 0x40D1364: _IO_file_xsgetn (fileops.c:1392)
==1089==    by 0x40D333D: _IO_sgetn (genops.c:467)
==1089==    by 0x40C68F6: fread (iofread.c:38)
==1089==    by 0x86A6739: cache_bread_1 (cache.c:319)
==1089==    by 0x86A6864: cache_bread (cache.c:358)
==1089==    by 0x820ED2E: bfd_bread (bfdio.c:211)
==1089==    by 0x8634E6B: bfd_pef_scan_start_address (pef.c:483)
==1089==    by 0x8635167: bfd_pef_scan (pef.c:556)
==1089==    by 0x8635397: bfd_pef_object_p (pef.c:605)
==1089==    by 0x8212246: bfd_check_format_matches (format.c:322)
==1089==    by 0x8052A83: display_object_bfd (objdump.c:4163)
==1089==  If you believe this happened as a result of a stack
==1089==  overflow in your program's main thread (unlikely but
==1089==  possible), you can try to increase the size of the
==1089==  main thread stack using the --main-stacksize= flag.
==1089==  The main thread stack size used in this run was 8388608.
==1089== 
==1089== HEAP SUMMARY:
==1089==     in use at exit: 10,821 bytes in 8 blocks
==1089==   total heap usage: 37 allocs, 29 frees, 19,532 bytes allocated
==1089== 
==1089== LEAK SUMMARY:
==1089==    definitely lost: 0 bytes in 0 blocks
==1089==    indirectly lost: 0 bytes in 0 blocks
==1089==      possibly lost: 0 bytes in 0 blocks
==1089==    still reachable: 10,821 bytes in 8 blocks
==1089==         suppressed: 0 bytes in 0 blocks
==1089== Rerun with --leak-check=full to see details of leaked memory
==1089== 
==1089== For counts of detected and suppressed errors, rerun with: -v
==1089== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)
---------------------------------------------
----------Crash log for issue 2--------------
root@manh-ubuntu16:~/fuzz/fuzz_binutils# valgrind
binutils-gdb-gcc-32/binutils/objdump  -x
crash-objdump-bfd_pef_parse_function_stubs 
==1117== Memcheck, a memory error detector
==1117== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==1117== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==1117== Command: binutils-gdb-gcc-32/binutils/objdump -x
crash-objdump-bfd_pef_parse_function_stubs
==1117== 

crash-objdump-bfd_pef_parse_function_stubs:     file format pef
crash-objdump-bfd_pef_parse_function_stubs
architecture: powerpc:common64, flags 0x000001ff:
HAS_RELOC, EXEC_P, HAS_LINENO, HAS_DEBUG, HAS_SYMS, HAS_LOCALS, DYNAMIC,
WP_TEXT, D_PAGED
start address 0x0000000000000000

==1117== Invalid write of size 4
==1117==    at 0x863489F: bfd_pef_parse_imported_library (pef.c:354)
==1117==    by 0x8635928: bfd_pef_parse_function_stubs (pef.c:761)
==1117==    by 0x8635F8B: bfd_pef_parse_symbols (pef.c:929)
==1117==    by 0x8636011: bfd_pef_count_symbols (pef.c:951)
==1117==    by 0x8636027: bfd_pef_get_symtab_upper_bound (pef.c:957)
==1117==    by 0x804AE3A: slurp_symtab (objdump.c:705)
==1117==    by 0x8052667: dump_bfd (objdump.c:4037)
==1117==    by 0x8052A97: display_object_bfd (objdump.c:4165)
==1117==    by 0x8052D1F: display_any_bfd (objdump.c:4255)
==1117==    by 0x8052D8C: display_file (objdump.c:4276)
==1117==    by 0x80537A5: main (objdump.c:4603)
==1117==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==1117== 
==1117== 
==1117== Process terminating with default action of signal 11 (SIGSEGV)
==1117==  Access not within mapped region at address 0x0
==1117==    at 0x863489F: bfd_pef_parse_imported_library (pef.c:354)
==1117==    by 0x8635928: bfd_pef_parse_function_stubs (pef.c:761)
==1117==    by 0x8635F8B: bfd_pef_parse_symbols (pef.c:929)
==1117==    by 0x8636011: bfd_pef_count_symbols (pef.c:951)
==1117==    by 0x8636027: bfd_pef_get_symtab_upper_bound (pef.c:957)
==1117==    by 0x804AE3A: slurp_symtab (objdump.c:705)
==1117==    by 0x8052667: dump_bfd (objdump.c:4037)
==1117==    by 0x8052A97: display_object_bfd (objdump.c:4165)
==1117==    by 0x8052D1F: display_any_bfd (objdump.c:4255)
==1117==    by 0x8052D8C: display_file (objdump.c:4276)
==1117==    by 0x80537A5: main (objdump.c:4603)
==1117==  If you believe this happened as a result of a stack
==1117==  overflow in your program's main thread (unlikely but
==1117==  possible), you can try to increase the size of the
==1117==  main thread stack using the --main-stacksize= flag.
==1117==  The main thread stack size used in this run was 8388608.
==1117== 
==1117== HEAP SUMMARY:
==1117==     in use at exit: 18,179 bytes in 10 blocks
==1117==   total heap usage: 67 allocs, 57 frees, 60,491 bytes allocated
==1117== 
==1117== LEAK SUMMARY:
==1117==    definitely lost: 0 bytes in 0 blocks
==1117==    indirectly lost: 0 bytes in 0 blocks
==1117==      possibly lost: 0 bytes in 0 blocks
==1117==    still reachable: 18,179 bytes in 10 blocks
==1117==         suppressed: 0 bytes in 0 blocks
==1117== Rerun with --leak-check=full to see details of leaked memory
==1117== 
==1117== For counts of detected and suppressed errors, rerun with: -v
==1117== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)
---------------------------------------------

--
Thanks & Regards,
Nguyen Duc Manh
VinCSS (a member of Vingroup)
[M] (+84) 346136886
[E] address@hidden
[W] www.vincss.net

-- 
You are receiving this mail because:
You are on the CC list for the bug.