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.

1 files changed, 45 insertions, 4 deletions

diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index 689f9f5..31d9119 100644
--- 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)