| Summary: | Stack-buffer-overflow in {coff,coff64}-rs6000.c | ||
|---|---|---|---|
| Product: | binutils | Reporter: | Ned Williamson <ned> |
| Component: | binutils | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | nickc |
| Priority: | P2 | ||
| Version: | 2.30 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
| Attachments: |
testcase
suggested patch crash state |
||
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. |
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.