Bug 24829 - readelf: multi interger overflow in readelf.c and dwarf.c
Summary: readelf: multi interger overflow in readelf.c and dwarf.c
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.33
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-21 17:00 UTC by tfx
Modified: 2019-08-23 09:39 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2019-07-25 00:00:00


Attachments
poc-interger-overflow (1.86 KB, application/x-object)
2019-07-21 17:00 UTC, tfx
Details
Proposed patch (276 bytes, patch)
2019-07-25 12:14 UTC, Nick Clifton
Details | Diff
poc5 (589 bytes, application/octet-stream)
2019-08-20 16:15 UTC, tfx
Details
Another patch (863 bytes, patch)
2019-08-22 15:07 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tfx 2019-07-21 17:00:27 UTC
Created attachment 11914 [details]
poc-interger-overflow

Hi Nick,

An interger overflow issue was discovered in readelf. 
I built 32bit readelf (GNU Binutils) 2.32.51.20190715 use commit 3719fd55 in Ubuntu 16.04 TLS .

The source code with problem show as follow. 

readelf.c:13347

>   rloc = start + rp->r_offset;
>   if ((rloc + reloc_size) > end || (rloc < start))
>   {
>      warn (_("skipping invalid relocation offset 0x%lx in section %s\n"),
>     (unsigned long) rp->r_offset,
>     printable_section_name (filedata, section));
>      continue;
>    }
	 

rp->r_offset is from input file, reloc_size is a value in (1, 2, 3, 4, 8).
When (rloc = start + rp->r_offset) == 0xFFFFFFFF in line 13347,  rloc + reloc_size will cause integer overflow in line 13348. 
Finally, program will crash in write access violation in byte_put_little_endian function in elfcomm.c.
Maybe it can fix like this:
   if ((rloc + reloc_size) > end || (rloc < start) || (rloc + reloc_size) < start)



Triggering the bug requires accurate input. I'm not sure the poc file can trigger a crash in your environment.
You can try using gdb.
> file readelf
> r -a poc1

The crash output show as follow.

Stopped reason: SIGSEGV
0x080c9169 in byte_put_little_endian (field=0xffffffff <error: Cannot access memory at address 0xffffffff>, value=0x12004004aa, size=0x2)
    at elfcomm.c:81
81            field[1] = (value >> 8) & 0xff;
gdb-peda$ bt
#0  0x080c9169 in byte_put_little_endian (field=0xffffffff <error: Cannot access memory at address 0xffffffff>, value=0x12004004aa,
    size=0x2) at elfcomm.c:81
#1  0x0804c819 in apply_relocations (filedata=0x812d908, section=0x8130fe8, start=0x812eae8 "j", size=0x20, relocs_return=0x0,
    num_relocs_return=0x0) at readelf.c:13433
#2  0x0808d27c in process_notes_at (filedata=0x812d908, section=0x8130fe8, offset=0x21c, length=0x20, align=0x4) at readelf.c:19098
#3  0x0808cfc8 in process_note_sections (filedata=0x812d908) at readelf.c:19372
#4  0x0805f480 in process_notes (filedata=0x812d908) at readelf.c:19408
#5  0x08053059 in process_object (filedata=0x812d908) at readelf.c:19778
#6  0x0804b5d9 in process_file (file_name=0xffffd439 "poc-readelf-a/poc3") at readelf.c:20190
#7  0x0804a86a in main (argc=0x3, argv=0xffffd264) at readelf.c:20249
#8  0xf7e13637 in __libc_start_main () from /lib/i386-linux-gnu/libc.so.6
#9  0x080494a1 in _start ()
gdb-peda$
Comment 1 Nick Clifton 2019-07-25 12:14:40 UTC
Created attachment 11922 [details]
Proposed patch

Hi tfx,

  Thanks for the detailed bug report.

  You are right - I am having difficulty reproducing the bug in my
  test environment, but I agree that the overflow can happen.

  Please could you try out this proposed patch and let me know if
  it solves the problem for you ?

  Thanks.

Cheers
  Nick
Comment 2 tfx 2019-07-26 11:54:57 UTC
Hi Nick,

  I tested this patch and it successfully fixed this problem.

  Thanks for your work.

Cheers
Comment 3 cvs-commit@gcc.gnu.org 2019-08-05 09:41:51 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit e17869db99195849826eaaf5d2d0eb2cfdd7a2a7
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Aug 5 10:40:35 2019 +0100

    Catch potential integer overflow in readelf when processing corrupt binaries.
    
    	PR 24829
    	* readelf.c (apply_relocations): Catch potential integer overflow
    	whilst checking reloc location against section size.
Comment 4 Nick Clifton 2019-08-05 09:58:03 UTC
Patch applied.
Comment 5 Trupti Pardeshi 2019-08-12 13:54:35 UTC
Hello,

May I know if Binutils-2.31 is also affected and requires this fix? Any heads up will be appreciated.

Thank you in advance.

Best Regards,
Comment 6 Nick Clifton 2019-08-12 15:17:48 UTC
Hi Trupti,

(In reply to Trupti Pardeshi from comment #5)

> May I know if Binutils-2.31 is also affected and requires this fix? 

Yes and no.  Yes it is affected (as is the 2.32 release), but in my
opinion no fix is needed as this is a corner case that is only
triggered by a malicious input file.  In the normal course of events
users will never run into this bug.

Cheers
  Nick
Comment 7 tfx 2019-08-20 16:11:59 UTC
Hi Nick, 

I found several similar problems in dwarf.c

You can reproduce it use "readelf -w poc5" with ASAN.
The crash output show as follow.

 Line Number Statements:
ASAN:DEADLYSIGNAL
=================================================================
==1276==ERROR: AddressSanitizer: SEGV on unknown address 0x1bf66161 (pc 0x08234f98 bp 0xffc3aa88 sp 0xffc3a7e0 T0)
    #0 0x8234f97 in display_debug_lines_raw ./src/binutils/dwarf.c:3840:18
    #1 0x8234f97 in display_debug_lines ./src/binutils/dwarf.c:4825
    #2 0x81984d7 in display_debug_section ./src/binutils/readelf.c:14231:18
    #3 0x81984d7 in process_section_contents ./src/binutils/readelf.c:14322
    #4 0x8178730 in process_object ./src/binutils/readelf.c:19760:9
    #5 0x8140c51 in process_file ./src/binutils/readelf.c:20190:13
    #6 0x8140c51 in main ./src/binutils/readelf.c:20249
    #7 0xf7ce1636 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18636)
    #8 0x806254c in _start (/vul/readelf/readelf-pat+0x806254c)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./src/binutils/dwarf.c:3840:18 in display_debug_lines_raw
==1276==ABORTING



The source code with problem show as follow. 

dwarf.c

 2064       if (block_start + uvalue > end || data < block_start)
 2065         {
 2066           warn (_("Corrupt attribute block length: %lx\n"), (long) uvalue);
 2067           uvalue = end - block_start;
 2068         }


 2084       data = block_start + uvalue;
 2085       if (block_start + uvalue > end || data < block_start)
 2086         {
 2087           warn (_("Corrupt attribute block length: %lx\n"), (long) uvalue);
 2088           uvalue = end - block_start;
 2089         }


 2105       data = block_start + uvalue;
 2106       if (block_start + uvalue > end || data < block_start)
 2107         {
 2108           warn (_("Corrupt attribute block length: %lx\n"), (long) uvalue);
 2109           uvalue = end - block_start;
 2110         }



 2127       data = block_start + uvalue;
 2128       if (block_start + uvalue > end
 2129           /* PR 17531: file: 5b5f0592.  */
 2130           || data < block_start)
 2131         {
 2132           warn (_("Corrupt attribute block length: %lx\n"), (long) uvalue);
 2133           uvalue = end - block_start;
 2134         }
 2135       if (do_loc)
 2136         data = block_start + uvalue;
 2137       else
 2138         data = display_block (block_start, uvalue, end, delimiter);
 2139       break;

When "uvalue" is a specific value,  "block_start + uvalue" will cause integer overflow. This will cause a wrong "data" value and trigger crash.

3840               op_code = *data++;


It seems that reporting this type of bug has no meaning. What do you think?
Comment 8 tfx 2019-08-20 16:15:23 UTC
Created attachment 11954 [details]
poc5
Comment 9 Nick Clifton 2019-08-21 10:31:17 UTC
(In reply to tfx from comment #7)
Hi tfx,

> You can reproduce it use "readelf -w poc5" with ASAN.
> The crash output show as follow.

Again I cannot reproduce this failure. :-(
Part of the problem is that I am unable to build 32-bit binaries
with address sanitization enabled.  I think that this is a limitation
of the Fedora distribution, but I do not know of any way around the
problem.

>  2063       data = block_start + uvalue;
>  2064       if (block_start + uvalue > end || data < block_start)
>  2065         {
>  2066           warn (_("Corrupt attribute block length: %lx\n"), (long)
> uvalue);
>  2067           uvalue = end - block_start;
>  2068         }


> When "uvalue" is a specific value,  "block_start + uvalue" will cause
> integer overflow. This will cause a wrong "data" value and trigger crash.

I get the "block_start + uvalue" can overflow, but won't this trigger
the "data < block_start" part of the test ?  Which in turn will reset
uvalue to a sane number, and so allow the rest of the code to continue ?


> It seems that reporting this type of bug has no meaning. What do you think?

Oh no, they are definitely worth reporting.  It is just proving to be
very hard for me to track down the cause of the problems and come up
with fixes that will work.

Cheers
  Nick
Comment 10 Alan Modra 2019-08-21 12:32:57 UTC
> I get the "block_start + uvalue" can overflow, but won't this trigger the "data < block_start" part of the test?

Not necessarily.  The pointers may only be 32 bit, which with a 64-bit uvalue leads to many values of uvalue > 4G that wrap to a "valid" range.  Pointer comparisons are a pain.  It's much better in this situation to calculate the max valid size left then compare that with uvalue.
Comment 11 tfx 2019-08-21 14:23:56 UTC
(In reply to Nick Clifton from comment #9)
> (In reply to tfx from comment #7)
> 
> > You can reproduce it use "readelf -w poc5" with ASAN.
> > The crash output show as follow.
> 
> Again I cannot reproduce this failure. :-(
> Part of the problem is that I am unable to build 32-bit binaries
> with address sanitization enabled.  I think that this is a limitation
> of the Fedora distribution, but I do not know of any way around the
> problem.
> 

You can try to only build part of 32-bit binaries.
CC="clang -m32" CXX="clang -m32" CFLAGS="-m32 -fsanitize=address" CXXFLAGS="-m32 -fsanitize=address" ./configure
make all-binutils


> >  2063       data = block_start + uvalue;
> >  2064       if (block_start + uvalue > end || data < block_start)
> >  2065         {
> >  2066           warn (_("Corrupt attribute block length: %lx\n"), (long)
> > uvalue);
> >  2067           uvalue = end - block_start;
> >  2068         }
> 
> 
> > When "uvalue" is a specific value,  "block_start + uvalue" will cause
> > integer overflow. This will cause a wrong "data" value and trigger crash.
> 
> I get the "block_start + uvalue" can overflow, but won't this trigger
> the "data < block_start" part of the test ?  Which in turn will reset
> uvalue to a sane number, and so allow the rest of the code to continue ?
> 

I found root of the problem is that "data < block_start" is compiled as "uvalue < 0" in gcc. When integer overflow, these two statements have different judgment results.

Debug 32bit readelf with asan use poc5 in gdb 

(gdb) info reg eax   
eax            0x26262626       640034342
(gdb) info reg edx
edx            0xf5e03b33       -169854157
(gdb)
eax is "uvalue"
edx is "block_start"
data is 0xf5e03b33 + 0x26262626 = 0x1c066159

"data < block_start" is true. But "uvalue < 0" is false, code in braces is skipped, "uvalue" can't be reset.

It just happens when block_start > 0x80000000 and "block_start + uvalue >= 0x1`00000000".

Trigger this crash is difficult, but the overflow can happen.
I only trigger crash in 32bit readelf with ASAN. 


(In reply to Alan Modra from comment #10)
> > I get the "block_start + uvalue" can overflow, but won't this trigger the "data < block_start" part of the test?
> 
> Not necessarily.  The pointers may only be 32 bit, which with a 64-bit
> uvalue leads to many values of uvalue > 4G that wrap to a "valid" range. 
> Pointer comparisons are a pain.  It's much better in this situation to
> calculate the max valid size left then compare that with uvalue.

I think that's a good solution.
Comment 12 Nick Clifton 2019-08-22 15:07:10 UTC
Created attachment 11961 [details]
Another patch

OK, in which case please could you try out this patch and let me know if it
fixes the bug ?

Cheers
  Nick
Comment 13 tfx 2019-08-22 16:39:18 UTC
(In reply to Nick Clifton from comment #12)
> Created attachment 11961 [details]
> Another patch
> 
> OK, in which case please could you try out this patch and let me know if it
> fixes the bug ?
> 
> Cheers
>   Nick


I tested this patch and it successfully fixed these problems.
Thanks for your work.

Cheers
Comment 14 cvs-commit@gcc.gnu.org 2019-08-23 09:39:10 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit afc72f154d6c59367e2f1cdf3ead5035748e2b61
Author: Nick Clifton <nickc@redhat.com>
Date:   Fri Aug 23 10:37:51 2019 +0100

    Prevent a potential illegal memory access in the DWARF parser when processing a corrupt file.
    
    	PR 24829
    	* dwarf.c (check_uvalue): New function.  Ensures that a block's
    	size is valid.
    	(read_and_display_attr_value): Use check_value when processsing
    	DW_FORM_block<n> attributes.
Comment 15 Nick Clifton 2019-08-23 09:39:47 UTC
Hi tfx,

  Great - in which case I have applied the patch.

Cheers
  Nick