Bug 21786 - Stack-buffer-overflow in {coff,coff64}-rs6000.c
Summary: Stack-buffer-overflow in {coff,coff64}-rs6000.c
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.30
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-19 01:00 UTC by Ned Williamson
Modified: 2019-01-04 02:05 UTC (History)
1 user (show)

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


Attachments
testcase (85 bytes, application/octet-stream)
2017-07-19 01:00 UTC, Ned Williamson
Details
suggested patch (617 bytes, patch)
2017-07-19 01:01 UTC, Ned Williamson
Details | Diff
crash state (1.09 KB, text/plain)
2017-07-19 01:02 UTC, Ned Williamson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Williamson 2017-07-19 01:00:46 UTC
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.
Comment 1 Ned Williamson 2017-07-19 01:01:42 UTC
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.
Comment 2 Ned Williamson 2017-07-19 01:02:50 UTC
Created attachment 10271 [details]
crash state

Here is the crashing state when inspecting the crash using ASAN.
Comment 3 cvs-commit@gcc.gnu.org 2017-07-19 10:08:57 UTC
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.
Comment 4 Nick Clifton 2017-07-19 10:16:13 UTC
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
Comment 5 Ned Williamson 2017-07-19 13:36:56 UTC
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.
Comment 6 cvs-commit@gcc.gnu.org 2017-07-19 15:15:41 UTC
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.
Comment 7 Nick Clifton 2017-07-19 15:16:47 UTC
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
Comment 8 cvs-commit@gcc.gnu.org 2017-09-04 15:06:21 UTC
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.
Comment 9 cvs-commit@gcc.gnu.org 2019-01-04 02:05:42 UTC
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.