From: Alan Modra Date: Thu, 7 Oct 2021 00:49:53 +0000 (+1030) Subject: PR28423, use-after-free in objdump X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=66468343379e535c6804a075709b5b49b09c6ed8;p=binutils-gdb.git 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 689f9f5b37a..31d91190103 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) diff --git a/bfd/coff64-rs6000.c b/bfd/coff64-rs6000.c index e66d75f3713..04e0798bf69 100644 --- 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