PR28423, use-after-free in objdump

XCOFF archives use a bi-directional linked list for file members.  So
one member points to both the previous member and the next member.
Members may not be sequentially ordered in the file.  This of course
is over-engineered nonsense and an attractive target for fuzzers.
(There is even a free list of members!)  The testcase in PR28423 is an
XCOFF archive with one member pointing to itself, which results in
lots of bad behaviour.  For example, "ar t" never terminates.

The use-after-free with "objdump -r" happens like this:  The first
archive element is opened, its symbols are read and "canonicalized"
for objdump, then relocations are read and printed.  Those relocations
use the canonicalized symbols, and also happen to be cached by the
coff bfd backend support.  objdump frees the symbols.  The next
archive element is then opened.  This must be done before the first
element is closed, because finding the next element uses data held in
the currect element.  Unfortunately the next element happens to be the
original, so we aren't opening, we're reopening a bfd which has cached
data.  When the relocations are printed they use the cached copy
containing references to the freed canonical symbols.

This patch adds a little sanity checking to the XCOFF "open next
archive file" support, so that it rejects archive members pointing at
themselves.  That is sufficient to cure this problem.  Anything more
is overkill.  If someone deliberately fuzzes an XCOFF archive with an
element loop then reports an "ar" bug when it runs forever, they will
find their bug report closed WONTFIX.

	PR 28423
	* coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Save size occupied
	by member name in areltdata.extra_size.
	(_bfd_xcoff_openr_next_archived_file): Sanity check nextoff.
	* coff64-rs6000.c (xcoff64_openr_next_archived_file): Call
	_bfd_xcoff_openr_next_archived_file.

diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index 689f9f5b37a787c52a82ba941fdb30ffa559edcd..31d91190103a9be5434c6d900e9385638a155e25 100644 (file)
--- a/bfd/coff-rs6000.c
+++ b/bfd/coff-rs6000.c
@@ -1669,6 +1669,10 @@ _bfd_xcoff_read_ar_hdr (bfd *abfd)
       ret->filename = (char *) hdrp + SIZEOF_AR_HDR_BIG;
     }
 
+  /* Size occupied by the header above that covered in the fixed
+     SIZEOF_AR_HDR or SIZEOF_AR_HDR_BIG.  */
+  ret->extra_size = namlen + (namlen & 1) + SXCOFFARFMAG;
+
   /* Skip over the XCOFFARFMAG at the end of the file name.  */
   if (bfd_seek (abfd, (file_ptr) ((namlen & 1) + SXCOFFARFMAG), SEEK_CUR) != 0)
     return NULL;
@@ -1682,6 +1686,7 @@ bfd *
 _bfd_xcoff_openr_next_archived_file (bfd *archive, bfd *last_file)
 {
   file_ptr filestart;
+  file_ptr laststart, lastend;
 
   if (xcoff_ardata (archive) == NULL)
     {
@@ -1692,9 +1697,27 @@ _bfd_xcoff_openr_next_archived_file (bfd *archive, bfd *last_file)
   if (! xcoff_big_format_p (archive))
     {
       if (last_file == NULL)
-       filestart = bfd_ardata (archive)->first_file_filepos;
+       {
+         filestart = bfd_ardata (archive)->first_file_filepos;
+         laststart = 0;
+         lastend = SIZEOF_AR_FILE_HDR;
+       }
       else
-       GET_VALUE_IN_FIELD (filestart, arch_xhdr (last_file)->nextoff, 10);
+       {
+         struct areltdata *arel = arch_eltdata (last_file);
+
+         GET_VALUE_IN_FIELD (filestart, arch_xhdr (last_file)->nextoff, 10);
+         laststart = last_file->proxy_origin;
+         lastend = laststart + arel->parsed_size;
+         laststart -= SIZEOF_AR_HDR + arel->extra_size;
+       }
+
+      /* Sanity check that we aren't pointing into the previous element.  */
+      if (filestart != 0 && filestart >= laststart && filestart < lastend)
+       {
+         bfd_set_error (bfd_error_malformed_archive);
+         return NULL;
+       }
 
       if (filestart == 0
          || EQ_VALUE_IN_FIELD (filestart, xcoff_ardata (archive)->memoff, 10)
@@ -1707,9 +1730,27 @@ _bfd_xcoff_openr_next_archived_file (bfd *archive, bfd *last_file)
   else
     {
       if (last_file == NULL)
-       filestart = bfd_ardata (archive)->first_file_filepos;
+       {
+         filestart = bfd_ardata (archive)->first_file_filepos;
+         laststart = 0;
+         lastend = SIZEOF_AR_FILE_HDR_BIG;
+       }
       else
-       GET_VALUE_IN_FIELD (filestart, arch_xhdr_big (last_file)->nextoff, 10);
+       {
+         struct areltdata *arel = arch_eltdata (last_file);
+
+         GET_VALUE_IN_FIELD (filestart, arch_xhdr_big (last_file)->nextoff, 10);
+         laststart = last_file->proxy_origin;
+         lastend = laststart + arel->parsed_size;
+         laststart -= SIZEOF_AR_HDR_BIG + arel->extra_size;
+       }
+
+      /* Sanity check that we aren't pointing into the previous element.  */
+      if (filestart != 0 && filestart >= laststart && filestart < lastend)
+       {
+         bfd_set_error (bfd_error_malformed_archive);
+         return NULL;
+       }
 
       if (filestart == 0
          || EQ_VALUE_IN_FIELD (filestart, xcoff_ardata_big (archive)->memoff, 10)

diff --git a/bfd/coff64-rs6000.c b/bfd/coff64-rs6000.c
index e66d75f3713edbe1b1d93de44f375641f9083f33..04e0798bf69cf10ca70079cae67a2a8f8e0af483 100644 (file)
--- a/bfd/coff64-rs6000.c
+++ b/bfd/coff64-rs6000.c
@@ -1938,8 +1938,6 @@ xcoff64_archive_p (bfd *abfd)
 static bfd *
 xcoff64_openr_next_archived_file (bfd *archive, bfd *last_file)
 {
-  bfd_vma filestart;
-
   if ((xcoff_ardata (archive) == NULL)
       || ! xcoff_big_format_p (archive))
     {
@@ -1947,27 +1945,7 @@ xcoff64_openr_next_archived_file (bfd *archive, bfd *last_file)
       return NULL;
     }
 
-  if (last_file == NULL)
-    {
-      filestart = bfd_ardata (archive)->first_file_filepos;
-    }
-  else
-    {
-      filestart = bfd_scan_vma (arch_xhdr_big (last_file)->nextoff,
-                               (const char **) NULL, 10);
-    }
-
-  if (filestart == 0
-      || filestart == bfd_scan_vma (xcoff_ardata_big (archive)->memoff,
-                                   (const char **) NULL, 10)
-      || filestart == bfd_scan_vma (xcoff_ardata_big (archive)->symoff,
-                                   (const char **) NULL, 10))
-    {
-      bfd_set_error (bfd_error_no_more_archived_files);
-      return NULL;
-    }
-
-  return _bfd_get_elt_at_filepos (archive, (file_ptr) filestart);
+  return _bfd_xcoff_openr_next_archived_file (archive, last_file);
 }
 
 /* We can't use the usual coff_sizeof_headers routine, because AIX