More fixes for buffer overruns instigated by corrupt binaries.
authorNick Clifton <nickc@redhat.com>
Mon, 3 Nov 2014 17:44:00 +0000 (17:44 +0000)
committerNick Clifton <nickc@redhat.com>
Mon, 3 Nov 2014 17:44:00 +0000 (17:44 +0000)
PR binutils/17512
* objdump.c (slurp_symtab): Fail gracefully if the table could not
be read.
(dump_relocs_in_section): Likewise.

* aoutx.h (slurp_symbol_table): Check that computed table size is
not bigger than the file from which is it being read.
(slurp_reloc_table): Likewise.
* coffcode.h (coff_slurp_line_table): Remove unneeded local
'warned'.  Do not try to print the details of a symbol with an
invalid index.
* coffgen.c (make_a_sectiobn_from_file): Check computed string
index against length of string table.
(bfd_coff_internal_syment_name): Check read in string offset
against length of string table.
(build_debug_section): Return a pointer to the section used.
(_bfd_coff_read_string_table): Store the length of the string
table in the coff_tdata structure.
(bfd_coff_free_symbols): Set the length of the string table to
zero when it is freed.
(coff_get_normalized_symtab): Check offsets against string table
or data table lengths as appropriate.
* cofflink.c (_bfd_coff_link_input_bfd): Check offset against
length of string table.
* compress.c (bfd_get_full_section_contents): Check computed size
against the size of the file.
* libcoff-in.h (obj_coff_strings_len): Define.
(struct coff_tdata): Add strings_len field.
* libcoff.h: Regenerate.
* peXXigen.c (pe_print_debugdata): Do not attempt to print the
data if the debug section is too small.
* xcofflink.c (xcoff_link_input_bfd):  Check offset against
length of string table.

12 files changed:
bfd/ChangeLog
bfd/aoutx.h
bfd/coffcode.h
bfd/coffgen.c
bfd/cofflink.c
bfd/compress.c
bfd/libcoff-in.h
bfd/libcoff.h
bfd/peXXigen.c
bfd/xcofflink.c
binutils/ChangeLog
binutils/objdump.c

index d2d8bae..591ff95 100644 (file)
@@ -1,5 +1,37 @@
 2014-11-03  Nick Clifton  <nickc@redhat.com>
 
+       PR binutils/17512
+       * aoutx.h (slurp_symbol_table): Check that computed table size is
+       not bigger than the file from which is it being read.
+       (slurp_reloc_table): Likewise.
+       * coffcode.h (coff_slurp_line_table): Remove unneeded local
+       'warned'.  Do not try to print the details of a symbol with an
+       invalid index.
+       * coffgen.c (make_a_sectiobn_from_file): Check computed string
+       index against length of string table.
+       (bfd_coff_internal_syment_name): Check read in string offset
+       against length of string table.
+       (build_debug_section): Return a pointer to the section used.
+       (_bfd_coff_read_string_table): Store the length of the string
+       table in the coff_tdata structure.
+       (bfd_coff_free_symbols): Set the length of the string table to
+       zero when it is freed.
+       (coff_get_normalized_symtab): Check offsets against string table
+       or data table lengths as appropriate.
+       * cofflink.c (_bfd_coff_link_input_bfd): Check offset against
+       length of string table.
+       * compress.c (bfd_get_full_section_contents): Check computed size
+       against the size of the file.
+       * libcoff-in.h (obj_coff_strings_len): Define.
+       (struct coff_tdata): Add strings_len field.
+       * libcoff.h: Regenerate.
+       * peXXigen.c (pe_print_debugdata): Do not attempt to print the
+       data if the debug section is too small.
+       * xcofflink.c (xcoff_link_input_bfd):  Check offset against
+       length of string table.
+
+2014-11-03  Nick Clifton  <nickc@redhat.com>
+
        * po/fi.po: Updated Finnish translation.
 
 2014-10-31  Andrew Pinski  <apinski@cavium.com>
index bef59b4..cb0887a 100644 (file)
@@ -1756,6 +1756,8 @@ NAME (aout, slurp_symbol_table) (bfd *abfd)
     return TRUE;               /* Nothing to do.  */
 
   cached_size *= sizeof (aout_symbol_type);
+  if (cached_size >= (bfd_size_type) bfd_get_size (abfd))
+    return FALSE;
   cached = (aout_symbol_type *) bfd_zmalloc (cached_size);
   if (cached == NULL)
     return FALSE;
@@ -2307,6 +2309,11 @@ NAME (aout, slurp_reloc_table) (bfd *abfd, sec_ptr asect, asymbol **symbols)
   if (reloc_size == 0)
     return TRUE;               /* Nothing to be done.  */
 
+  /* PR binutils/17512: Do not even try to
+     load the relocs if their size is corrupt.  */
+  if (reloc_size + asect->rel_filepos >= (bfd_size_type) bfd_get_size (abfd))
+    return FALSE;
+
   if (bfd_seek (abfd, asect->rel_filepos, SEEK_SET) != 0)
     return FALSE;
 
index 6678b88..48709f4 100644 (file)
@@ -4546,21 +4546,18 @@ coff_slurp_line_table (bfd *abfd, asection *asect)
 
       if (cache_ptr->line_number == 0)
        {
-         bfd_boolean warned;
          bfd_signed_vma symndx;
          coff_symbol_type *sym;
 
          nbr_func++;
-         warned = FALSE;
          symndx = dst.l_addr.l_symndx;
          if (symndx < 0
              || (bfd_vma) symndx >= obj_raw_syment_count (abfd))
            {
              (*_bfd_error_handler)
-               (_("%B: warning: illegal symbol index %ld in line numbers"),
-                abfd, (long) symndx);
-             symndx = 0;
-             warned = TRUE;
+               (_("%B: warning: illegal symbol index 0x%lx in line number entry %d"),
+                abfd, (long) symndx, counter);
+             continue;
            }
 
          /* FIXME: We should not be casting between ints and
@@ -4569,7 +4566,7 @@ coff_slurp_line_table (bfd *abfd, asection *asect)
                 ((symndx + obj_raw_syments (abfd))
                  ->u.syment._n._n_n._n_zeroes));
          cache_ptr->u.sym = (asymbol *) sym;
-         if (sym->lineno != NULL && ! warned)
+         if (sym->lineno != NULL)
            (*_bfd_error_handler)
              (_("%B: warning: duplicate line number information for `%s'"),
               abfd, bfd_asymbol_name (&sym->symbol));
index f18ddab..d0bf2c1 100644 (file)
@@ -84,9 +84,8 @@ make_a_section_from_file (bfd *abfd,
          strings = _bfd_coff_read_string_table (abfd);
          if (strings == NULL)
            return FALSE;
-         /* FIXME: For extra safety, we should make sure that
-             strindex does not run us past the end, but right now we
-             don't know the length of the string table.  */
+         if ((bfd_size_type)(strindex + 2) >= obj_coff_strings_len (abfd))
+           return FALSE;
          strings += strindex;
          name = (char *) bfd_alloc (abfd,
                                      (bfd_size_type) strlen (strings) + 1 + 1);
@@ -464,6 +463,8 @@ _bfd_coff_internal_syment_name (bfd *abfd,
          if (strings == NULL)
            return NULL;
        }
+      if (sym->_n._n_n._n_offset >= obj_coff_strings_len (abfd))
+       return NULL;
       return strings + sym->_n._n_n._n_offset;
     }
 }
@@ -1545,7 +1546,7 @@ coff_pointerize_aux (bfd *abfd,
    we didn't want to go to the trouble until someone needed it.  */
 
 static char *
-build_debug_section (bfd *abfd)
+build_debug_section (bfd *abfd, asection ** sect_return)
 {
   char *debug_section;
   file_ptr position;
@@ -1573,6 +1574,8 @@ build_debug_section (bfd *abfd)
       || bfd_bread (debug_section, sec_size, abfd) != sec_size
       || bfd_seek (abfd, position, SEEK_SET) != 0)
     return NULL;
+
+  * sect_return = sect;
   return debug_section;
 }
 
@@ -1640,7 +1643,9 @@ _bfd_coff_get_external_symbols (bfd *abfd)
 
 /* Read in the external strings.  The strings are not loaded until
    they are needed.  This is because we have no simple way of
-   detecting a missing string table in an archive.  */
+   detecting a missing string table in an archive.  If the strings
+   are loaded then the STRINGS and STRINGS_LEN fields in the
+   coff_tdata structure will be set.  */
 
 const char *
 _bfd_coff_read_string_table (bfd *abfd)
@@ -1702,6 +1707,7 @@ _bfd_coff_read_string_table (bfd *abfd)
     }
 
   obj_coff_strings (abfd) = strings;
+  obj_coff_strings_len (abfd) = strsize;
 
   return strings;
 }
@@ -1722,6 +1728,7 @@ _bfd_coff_free_symbols (bfd *abfd)
     {
       free (obj_coff_strings (abfd));
       obj_coff_strings (abfd) = NULL;
+      obj_coff_strings_len (abfd) = 0;
     }
   return TRUE;
 }
@@ -1742,13 +1749,22 @@ coff_get_normalized_symtab (bfd *abfd)
   char *raw_src;
   char *raw_end;
   const char *string_table = NULL;
-  char *debug_section = NULL;
+  asection * debug_sec = NULL;
+  char *debug_sec_data = NULL;
   bfd_size_type size;
 
   if (obj_raw_syments (abfd) != NULL)
     return obj_raw_syments (abfd);
 
-  size = obj_raw_syment_count (abfd) * sizeof (combined_entry_type);
+  size = obj_raw_syment_count (abfd);
+  if (size == 0)
+    return NULL;
+  /* PR binutils/17512: Do not even try to load
+     a symbol table bigger than the entire file...  */
+  if (size >= (bfd_size_type) bfd_get_size (abfd))
+    return NULL;
+
+  size *= sizeof (combined_entry_type);
   internal = (combined_entry_type *) bfd_zalloc (abfd, size);
   if (internal == NULL && size != 0)
     return NULL;
@@ -1819,11 +1835,14 @@ coff_get_normalized_symtab (bfd *abfd)
                  if (string_table == NULL)
                    return NULL;
                }
-
-             internal_ptr->u.syment._n._n_n._n_offset =
-               ((bfd_hostptr_t)
-                (string_table
-                 + (internal_ptr + 1)->u.auxent.x_file.x_n.x_offset));
+             if ((bfd_size_type)((internal_ptr + 1)->u.auxent.x_file.x_n.x_offset)
+                 >= obj_coff_strings_len (abfd))
+               internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t) _("<corrupt>");
+             else
+               internal_ptr->u.syment._n._n_n._n_offset =
+                 ((bfd_hostptr_t)
+                  (string_table
+                   + (internal_ptr + 1)->u.auxent.x_file.x_n.x_offset));
            }
          else
            {
@@ -1878,18 +1897,31 @@ coff_get_normalized_symtab (bfd *abfd)
                  if (string_table == NULL)
                    return NULL;
                }
-             internal_ptr->u.syment._n._n_n._n_offset =
-               ((bfd_hostptr_t)
-                (string_table
-                 + internal_ptr->u.syment._n._n_n._n_offset));
+             if (internal_ptr->u.syment._n._n_n._n_offset >= obj_coff_strings_len (abfd))
+               internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t) _("<corrupt>");
+             else
+               internal_ptr->u.syment._n._n_n._n_offset =
+                 ((bfd_hostptr_t)
+                  (string_table
+                   + internal_ptr->u.syment._n._n_n._n_offset));
            }
          else
            {
              /* Long name in debug section.  Very similar.  */
-             if (debug_section == NULL)
-               debug_section = build_debug_section (abfd);
-             internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t)
-               (debug_section + internal_ptr->u.syment._n._n_n._n_offset);
+             if (debug_sec_data == NULL)
+               debug_sec_data = build_debug_section (abfd, & debug_sec);
+             if (debug_sec_data != NULL)
+               {
+                 BFD_ASSERT (debug_sec != NULL);
+                 /* PR binutils/17512: Catch out of range offsets into the debug data.  */
+                 if (internal_ptr->u.syment._n._n_n._n_offset > debug_sec->size)
+                   internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t) _("<corrupt>");
+                 else
+                   internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t)
+                     (debug_sec_data + internal_ptr->u.syment._n._n_n._n_offset);
+               }
+             else
+               internal_ptr->u.syment._n._n_n._n_offset = (bfd_hostptr_t) "";
            }
        }
       internal_ptr += internal_ptr->u.syment.n_numaux;
index 8c3f71b..2782795 100644 (file)
@@ -2003,7 +2003,10 @@ _bfd_coff_link_input_bfd (struct coff_final_link_info *flaginfo, bfd *input_bfd)
                          if (strings == NULL)
                            return FALSE;
                        }
-                     filename = strings + auxp->x_file.x_n.x_offset;
+                     if ((bfd_size_type) auxp->x_file.x_n.x_offset >= obj_coff_strings_len (input_bfd))
+                       filename = _("<corrupt>");
+                     else
+                       filename = strings + auxp->x_file.x_n.x_offset;
                      indx = _bfd_stringtab_add (flaginfo->strtab, filename,
                                                 hash, copy);
                      if (indx == (bfd_size_type) -1)
index 20eef95..083a7df 100644 (file)
@@ -177,6 +177,13 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
   switch (sec->compress_status)
     {
     case COMPRESS_SECTION_NONE:
+      /* PR binutils/17512: Avoid malloc or file reading errors due to
+        ridiculous section sizes.  But ignore linker created objects
+        with no contents (yet).  */
+      if (bfd_get_size (abfd) > 0
+         && sz > (bfd_size_type) bfd_get_size (abfd))
+       return FALSE;
+
       if (p == NULL)
        {
          p = (bfd_byte *) bfd_malloc (sz);
index 6162f2e..6b6eb28 100644 (file)
@@ -35,6 +35,7 @@
 #define obj_coff_external_syms(bfd)   (coff_data (bfd)->external_syms)
 #define obj_coff_keep_syms(bfd)              (coff_data (bfd)->keep_syms)
 #define obj_coff_strings(bfd)        (coff_data (bfd)->strings)
+#define obj_coff_strings_len(bfd)     (coff_data (bfd)->strings_len)
 #define obj_coff_keep_strings(bfd)    (coff_data (bfd)->keep_strings)
 #define obj_coff_sym_hashes(bfd)      (coff_data (bfd)->sym_hashes)
 #define obj_coff_strings_written(bfd) (coff_data (bfd)->strings_written)
@@ -75,6 +76,8 @@ typedef struct coff_tdata
   /* The string table.  May be NULL.  Read by
      _bfd_coff_read_string_table.  */
   char *strings;
+  /* The length of the strings table.  For error checking.  */
+  bfd_size_type strings_len;
   /* If this is TRUE, the strings may not be freed.  */
   bfd_boolean keep_strings;
   /* If this is TRUE, the strings have been written out already.  */
index 12f19d0..7ed52de 100644 (file)
@@ -39,6 +39,7 @@
 #define obj_coff_external_syms(bfd)   (coff_data (bfd)->external_syms)
 #define obj_coff_keep_syms(bfd)              (coff_data (bfd)->keep_syms)
 #define obj_coff_strings(bfd)        (coff_data (bfd)->strings)
+#define obj_coff_strings_len(bfd)     (coff_data (bfd)->strings_len)
 #define obj_coff_keep_strings(bfd)    (coff_data (bfd)->keep_strings)
 #define obj_coff_sym_hashes(bfd)      (coff_data (bfd)->sym_hashes)
 #define obj_coff_strings_written(bfd) (coff_data (bfd)->strings_written)
@@ -79,6 +80,8 @@ typedef struct coff_tdata
   /* The string table.  May be NULL.  Read by
      _bfd_coff_read_string_table.  */
   char *strings;
+  /* The length of the strings table.  For error checking.  */
+  bfd_size_type strings_len;
   /* If this is TRUE, the strings may not be freed.  */
   bfd_boolean keep_strings;
   /* If this is TRUE, the strings have been written out already.  */
index 1a5cb31..f1a1189 100644 (file)
@@ -2514,6 +2514,13 @@ pe_print_debugdata (bfd * abfd, void * vfile)
                section->name);
       return TRUE;
     }
+  else if (section->size < size)
+    {
+      fprintf (file,
+               _("\nError: section %s contains the debug data starting address but it is too small\n"),
+               section->name);
+      return FALSE;
+    }
 
   fprintf (file, _("\nThere is a debug directory in %s at 0x%lx\n\n"),
           section->name, (unsigned long) addr);
@@ -2523,7 +2530,7 @@ pe_print_debugdata (bfd * abfd, void * vfile)
   fprintf (file,
           _("Type                Size     Rva      Offset\n"));
 
-  /* Read the whole section. */
+  /* Read the whole section.  */
   if (!bfd_malloc_and_get_section (abfd, section, &data))
     {
       if (data != NULL)
index 1ca9c2e..9522974 100644 (file)
@@ -4494,7 +4494,10 @@ xcoff_link_input_bfd (struct xcoff_final_link_info *flinfo,
                          if (strings == NULL)
                            return FALSE;
                        }
-                     filename = strings + aux.x_file.x_n.x_offset;
+                     if ((bfd_size_type) aux.x_file.x_n.x_offset >= obj_coff_strings_len (input_bfd))
+                       filename = _("<corrupt>");
+                     else
+                       filename = strings + aux.x_file.x_n.x_offset;
                      indx = _bfd_stringtab_add (flinfo->strtab, filename,
                                                 hash, copy);
                      if (indx == (bfd_size_type) -1)
index cb5ec95..1f551dd 100644 (file)
@@ -1,5 +1,12 @@
 2014-11-03  Nick Clifton  <nickc@redhat.com>
 
+       PR binutils/17512
+       * objdump.c (slurp_symtab): Fail gracefully if the table could not
+       be read.
+       (dump_relocs_in_section): Likewise.
+
+2014-11-03  Nick Clifton  <nickc@redhat.com>
+
        * po/fi.po: Updated Finnish translation.
        * po/sv.po: Updated Swedish translation.
 
index 413de56..f6c4c16 100644 (file)
@@ -562,7 +562,10 @@ slurp_symtab (bfd *abfd)
 
   storage = bfd_get_symtab_upper_bound (abfd);
   if (storage < 0)
-    bfd_fatal (bfd_get_filename (abfd));
+    {
+      non_fatal (_("failed to read symbol table from: %s"), bfd_get_filename (abfd));
+      bfd_fatal (_("error message was"));
+    }
   if (storage)
     sy = (asymbol **) xmalloc (storage);
 
@@ -3108,7 +3111,11 @@ dump_relocs_in_section (bfd *abfd,
   relcount = bfd_canonicalize_reloc (abfd, section, relpp, syms);
 
   if (relcount < 0)
-    bfd_fatal (bfd_get_filename (abfd));
+    {
+      printf ("\n");
+      non_fatal (_("failed to read relocs in: %s"), bfd_get_filename (abfd));
+      bfd_fatal (_("error message was"));
+    }
   else if (relcount == 0)
     printf (" (none)\n\n");
   else