Bug 17510 - strings: crash when given a truncated ELF
Summary: strings: crash when given a truncated ELF
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.24
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-24 19:49 IST by Michal Zalewski
Modified: 2014-10-28 10:52 IST (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
strings-bfd-badptr (171 bytes, application/octet-stream)
2014-10-25 05:33 IST, Mike Frysinger
Details
Test case #2 (163 bytes, application/x-executable)
2014-10-25 17:30 IST, Michal Zalewski
Details
Improve handling of corrupt section groups. (811 bytes, patch)
2014-10-27 12:49 IST, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Zalewski 2014-10-24 19:49:58 IST
Test case:

http://lcamtuf.coredump.cx/strings-bfd-badptr

On the x86 Linux systems I tried this on, the test case causes dereference of a pointer in the vicinity of 0x41414141.

/mz
Comment 1 Mike Frysinger 2014-10-25 05:33:31 IST
Created attachment 7846 [details]
strings-bfd-badptr

please attach all files in the bug.  external links die.
Comment 2 Michal Zalewski 2014-10-25 17:30:02 IST
Created attachment 7848 [details]
Test case #2

Note that range checking problems are somewhat endemic across the function; here's a test case that crashes in a different location but due to the same --n_elt / ++idx pattern. This one looks like it leads to writes to arbitrary pointers.
Comment 3 cvs-commit@gcc.gnu.org 2014-10-27 12:46:32 IST
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  493a33860c71cac998f1a56d6d87d6faa801fbaa (commit)
      from  763905a3ad8f98d33bd9319790a8d53904554265 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=493a33860c71cac998f1a56d6d87d6faa801fbaa

commit 493a33860c71cac998f1a56d6d87d6faa801fbaa
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Oct 27 12:43:16 2014 +0000

    This patch closes a potential security hole in applications that use
    the bfd library to parse binaries containing maliciously corrupt section
    group headers.
    
    	PR binutils/17510
    	* elf.c (setup_group): Improve handling of corrupt group
    	sections.

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog |    6 ++++++
 bfd/elf.c     |   34 ++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)
Comment 4 Nick Clifton 2014-10-27 12:49:16 IST
Created attachment 7851 [details]
Improve handling of corrupt section groups.
Comment 5 Nick Clifton 2014-10-27 12:50:10 IST
Hi Markus,

  I have applied a patch (also uploaded to this PR) to fix this problem.  Please let me know if you find any more examples of corrupt binaries that can trigger this sort of problem.

Cheers
  Nick
Comment 6 Nick Clifton 2014-10-27 12:51:06 IST
oops - sorry, I meant "Michal" not "Markus".  Sorry Michal.
Comment 7 Michal Zalewski 2014-10-27 18:59:24 IST
Do you want me to file separate bugs for each?

For example, I have this in srec.c:

      char buf[10];
...
        sprintf (buf, "\\%03o", (unsigned int) c);

But with this test case, c will be -44, or "\1777777777777777777724", which sounds a lot longer than 9 characters.

http://lcamtuf.coredump.cx/strings-stack-overflow
Comment 8 Andreas Schwab 2014-10-27 21:03:48 IST
This should fix it:

diff --git a/bfd/srec.c b/bfd/srec.c
index 9ed2080..0c473b2 100644
--- a/bfd/srec.c
+++ b/bfd/srec.c
@@ -452,7 +452,7 @@ srec_scan (bfd *abfd)
 	case 'S':
 	  {
 	    file_ptr pos;
-	    char hdr[3];
+	    unsigned char hdr[3];
 	    unsigned int bytes, min_bytes;
 	    bfd_vma address;
 	    bfd_byte *data;
Comment 9 cvs-commit@gcc.gnu.org 2014-10-28 10:50:24 IST
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  708d7d0d11f0f2d776171979aa3479e8e12a38a0 (commit)
      from  6fb9c0f83252a79b2f1a3f8e75fa117ca7a4d589 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=708d7d0d11f0f2d776171979aa3479e8e12a38a0

commit 708d7d0d11f0f2d776171979aa3479e8e12a38a0
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Oct 28 10:48:14 2014 +0000

    This patch fixes a flaw in the SREC parser which could cause a stack overflow
    and potential secuiryt breach.
    
    	PR binutils/17510
    	* srec.c (srec_bad_byte): Increase size of buf to allow for
    	negative values.
    	(srec_scan): Use an unsigned char buffer to hold header bytes.

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog  |    8 ++++++++
 bfd/elf.c      |    2 +-
 bfd/peXXigen.c |    1 -
 bfd/srec.c     |    4 ++--
 4 files changed, 11 insertions(+), 4 deletions(-)
Comment 10 cvs-commit@gcc.gnu.org 2014-10-28 10:52:23 IST
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, binutils-2_25-branch has been updated
       via  b2f93c5011cab00f31669363577b938697752e43 (commit)
      from  a809b386e59dfcb3f4dedd8465975dabc55db5db (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b2f93c5011cab00f31669363577b938697752e43

commit b2f93c5011cab00f31669363577b938697752e43
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Oct 28 10:50:17 2014 +0000

    Import patches from the master branch which prevent seg-faults when parsing
    corrupt binaries.
    
    	2014-10-28  Andreas Schwab  <schwab@suse.de>
    		    Nick Clifton  <nickc@redhat.com>
    	PR binutils/17510
    	* srec.c (srec_bad_byte): Increase size of buf to allow for
    	negative values.
    	(srec_scan): Use an unsigned char buffer to hold header bytes.
    
    	2014-10-27  Nick Clifton  <nickc@redhat.com>
    	PR binutils/17512
    	* elf.c (bfd_section_from_shdr): Detect and warn about ELF
    	binaries with a group of sections linked by the string table
    	indicies.
    	* peXXigen.c (_bfd_XXi_swap_aouthdr_in): Handle corrupt binaries
    	with an invalid value for NumberOfRvaAndSizes.
    	(pe_print_edata): Detect out of range rvas and entry counts for
    	the Export Address table, Name Pointer table and Ordinal table.
    
    	PR binutils/17510
    	* elf.c (setup_group): Improve handling of corrupt group
    	sections.

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog  |   25 ++++++
 bfd/elf.c      |  226 +++++++++++++++++++++++++++++++++++++++-----------------
 bfd/peXXigen.c |   29 +++++++-
 bfd/srec.c     |    4 +-
 4 files changed, 212 insertions(+), 72 deletions(-)