Created attachment 10269 [details] testcase `_bfd_xcoff_read_ar_hdr` and similar functions can call strtol on a string that is not null-terminated, leading to an out of bounds read on the stack. See the attached testcase.
Created attachment 10270 [details] suggested patch Here, I attach my suggested patch, fixing all places where I was able to trigger the bug using a variant of the original testcase.
Created attachment 10271 [details] crash state Here is the crashing state when inspecting the crash using ASAN.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=29866fa186ee3ebda5242221607dba360b2e541e commit 29866fa186ee3ebda5242221607dba360b2e541e Author: Nick Clifton <nickc@redhat.com> Date: Wed Jul 19 11:07:43 2017 +0100 Fix address violation when attempting to read a corrupt field in a COFF archive header structure. PR 21786 * coff-rs6000.c (_bfd_strntol): New function. (_bfd_strntoll): New function. (GET_VALUE_IN_FIELD): New macro. (EQ_VALUE_IN_FIELD): new macro. (_bfd_xcoff_slurp_armap): Use new macros. (_bfd_xcoff_archive_p): Likewise. (_bfd_xcoff_read_ar_hdr): Likewise. (_bfd_xcoff_openr_next_archived_file): Likewise. (_bfd_xcoff_stat_arch_elt): Likewise.
Hi Ned, Thanks for reporting this bug. Unfortunately the patch you proposed will not work as the numeric strings in the archive header structure are not guaranteed to be NUL terminated. In fact the specification explicitly states: 16 Archive Member Headers Each member (linker, longnames, or object-file member) is preceded by a header. An archive member header has the following format, in which each field is an ASCII text string that is left justified and padded with spaces to the end of the field. There is no terminating null character in any of these fields. This is from "Microsoft Portable Executable and Common Object File Format Specification Revision 8.3 – February 6, 2013" So whilst there *might* be a space at the end of the field there definitely will not be a NUL character. The alternative is to copy the field into a NUL terminated buffer before attempting to parse it, and this is what I have done with the patch recently committed. As a bonus I also fixed up the places where strtoll (instead of strtol) should have been used to read 20 character numeric values. Cheers Nick
Hi Nick, thanks for the great patch! It's much better than the suggested one. Thank you for pointing out the specification. I did see a crash in coff64-rs6000 as well, so that may need to use the new safe functions.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6c4e7b6bfbc4679f695106de2817ecf02b27c8be commit 6c4e7b6bfbc4679f695106de2817ecf02b27c8be Author: Nick Clifton <nickc@redhat.com> Date: Wed Jul 19 16:14:02 2017 +0100 Extend previous fix to coff-rs6000.c to coff64-rs6000.c PR 21786 * coff64-rs6000.c (_bfd_strntol): New function. (_bfd_strntoll): New function. (GET_VALUE_IN_FIELD): New macro. (xcoff64_slurp_armap): Use new macros.
Hi Ned, > I did see a crash in coff64-rs6000 as well, so that may need to use the new > safe functions. Ah - thanks for pointing that out. I have now checked in an additional patch to cover coff64-rs6000.c. Cheers Nick
The binutils-2_29-branch branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7e693dd5cede637daf4456a2a8f207be1044c6e8 commit 7e693dd5cede637daf4456a2a8f207be1044c6e8 Author: Nick Clifton <nickc@redhat.com> Date: Mon Sep 4 16:04:44 2017 +0100 Import patch from mainline to fix address violations when parsing corrupt COFF binaries PR 21786 * coff-rs6000.c (_bfd_strntol): New function. (_bfd_strntoll): New function. (GET_VALUE_IN_FIELD): New macro. (EQ_VALUE_IN_FIELD): new macro. (_bfd_xcoff_slurp_armap): Use new macros. (_bfd_xcoff_archive_p): Likewise. (_bfd_xcoff_read_ar_hdr): Likewise. (_bfd_xcoff_openr_next_archived_file): Likewise. (_bfd_xcoff_stat_arch_elt): Likewise. * coff64-rs6000.c (_bfd_strntol): New function. (_bfd_strntoll): New function. (GET_VALUE_IN_FIELD): New macro. (xcoff64_slurp_armap): Use new macros.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=677bd4c69d0eda4f2ae635d793f23c0b1413a9e9 commit 677bd4c69d0eda4f2ae635d793f23c0b1413a9e9 Author: Alan Modra <amodra@gmail.com> Date: Fri Jan 4 12:18:36 2019 +1030 PR24061, powerpc-ibm-aix-ar sets bogus file permissions when extracting Mode field should be read in octal, all the rest in decimal. Do so. PR 24061 PR 21786 * coff-rs6000.c (GET_VALUE_IN_FIELD): Add base parameter and adjust all callers. (EQ_VALUE_IN_FIELD): Likewise. * coff64-rs6000.c (GET_VALUE_IN_FIELD): Likewise.