Bug 23657

Summary: Out of bound memory access
Product: gdb Reporter: Jun <jxx13>
Component: gdbAssignee: Keith Seitz <keiths>
Status: RESOLVED FIXED    
Severity: critical CC: brobecker, keiths, mark, maxcrees, nickc, rwmacleod, tromey, vries
Priority: P2    
Version: HEAD   
Target Milestone: 9.1   
Host: Target:
Build: Last reconfirmed: 2018-09-15 00:00:00
Attachments: Sample to trigger the bug

Description Jun 2018-09-14 14:50:20 UTC
Created attachment 11246 [details]
Sample to trigger the bug

Our fuzzer finds the following bug: 

dwarf2read.c:19513:

19508	  if (str_offset >= sect->size)
19509	    error (_("%s pointing outside of %s section [in module %s]"),
19510		   form_name, sect_name, bfd_get_filename (abfd));
19511	  gdb_assert (HOST_CHAR_BIT == 8);
19512	  if (sect->buffer[str_offset] == '\0')

Line 19508 compares the access index with the size of section. This size, which is specified in the target, can be arbitrary value. Making it to be 0xffffffffffffffff (on X64), the comparison never works. This leads to out of bound access at line 19512. 

An test case running on x86_64 CentOS Linux 7 is attached. It can stably crash the most recent version of GDB on github. 

Stack trace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000586cb3 in read_indirect_string_at_offset_from (objfile=<optimized out>, str_offset=<optimized out>, sect=0xfe5db8, form_name=0x900f18 "DW_FORM_strp", sect_name=0x8fb438 ".debug_str", abfd=<optimized out>) at dwarf2read.c:19513
19513	    return NULL;
Missing separate debuginfos, use: debuginfo-install expat-2.1.0-10.el7_3.x86_64 glibc-2.17-196.el7.x86_64 gmp-6.0.0-12.el7_1.x86_64 libgcc-4.8.5-16.el7.x86_64 libstdc++-4.8.5-16.el7.x86_64 mpfr-3.1.1-4.el7.x86_64 ncurses-libs-5.9-13.20130511.el7.x86_64 python-libs-2.7.5-58.el7.x86_64 xz-libs-5.2.2-1.el7.x86_64
(gdb) info stack 
#0  0x0000000000586cb3 in read_indirect_string_at_offset_from (objfile=<optimized out>, str_offset=<optimized out>, sect=0xfe5db8, form_name=0x900f18 "DW_FORM_strp", sect_name=0x8fb438 ".debug_str", abfd=<optimized out>) at dwarf2read.c:19513
#1  0x0000000000589919 in read_indirect_line_string_at_offset (str_offset=<optimized out>, abfd=<optimized out>, dwarf2_per_objfile=<optimized out>) at dwarf2read.c:19539
#2  read_indirect_line_string (bytes_read_ptr=<optimized out>, cu_header=<optimized out>, buf=<optimized out>, abfd=<optimized out>, dwarf2_per_objfile=<optimized out>) at dwarf2read.c:19595
#3  read_attribute_value (reader=reader@entry=0x7fffffffdd20, attr=attr@entry=0x7fffffffda60, form=14, implicit_const=-1, info_ptr=0x7fffef2d6773 "S\227\003") at dwarf2read.c:19077
#4  0x000000000058d179 in read_attribute (info_ptr=<optimized out>, abbrev=<optimized out>, attr=0x7fffffffda60, reader=0x7fffffffdd20) at dwarf2read.c:19242
#5  partial_die_info::read (this=this@entry=0x7fffffffdb30, reader=reader@entry=0x7fffffffdd20, abbrev=..., info_ptr=<optimized out>) at dwarf2read.c:18562
#6  0x000000000058d972 in load_partial_dies (reader=reader@entry=0x7fffffffdd20, info_ptr=info_ptr@entry=0x7fffef2d6772 "\002S\227\003", building_psymtab=building_psymtab@entry=1) at dwarf2read.c:18370
#7  0x000000000059e11b in process_psymtab_comp_unit_reader (reader=reader@entry=0x7fffffffdd20, info_ptr=0x7fffef2d6772 "\002S\227\003", comp_unit_die=0x1014d40, has_children=<optimized out>, data=data@entry=0x7fffffffddb0) at dwarf2read.c:8030
#8  0x00000000005903f6 in init_cutu_and_read_dies (this_cu=this_cu@entry=0xfe6070, abbrev_table=<optimized out>, abbrev_table@entry=0x0, use_existing_cu=use_existing_cu@entry=0, keep=keep@entry=0, skip_partial=skip_partial@entry=false, 
    die_reader_func=0x59dd60 <process_psymtab_comp_unit_reader(die_reader_specs const*, gdb_byte const*, die_info*, int, void*)>, data=0x7fffffffddb0) at dwarf2read.c:7664
#9  0x0000000000593c3b in process_psymtab_comp_unit (this_cu=0xfe6070, want_partial_unit=want_partial_unit@entry=0, pretend_language=pretend_language@entry=language_minimal) at dwarf2read.c:8121
#10 0x00000000005a2988 in dwarf2_build_psymtabs_hard (dwarf2_per_objfile=0xfe5ca0) at dwarf2read.c:8481
#11 dwarf2_build_psymtabs (objfile=0xfda160) at dwarf2read.c:6305
#12 0x00000000006469ec in require_partial_symbols (objfile=objfile@entry=0xfda160, verbose=verbose@entry=0) at psymtab.c:86
#13 0x0000000000697b45 in read_symbols (objfile=objfile@entry=0xfda160, add_flags=..., add_flags@entry=...) at symfile.c:817
#14 0x0000000000697373 in syms_from_objfile_1 (add_flags=..., addrs=0x7fffffffdf70, objfile=0xfda160) at symfile.c:996
#15 syms_from_objfile (add_flags=..., addrs=0x20, objfile=0xfda160) at symfile.c:1012
#16 symbol_file_add_with_addrs (abfd=<optimized out>, name=name@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", add_flags=..., add_flags@entry=..., addrs=addrs@entry=0x0, flags=..., flags@entry=..., parent=parent@entry=0x0) at symfile.c:1119
#17 0x00000000006986ca in symbol_file_add_from_bfd (parent=0x0, flags=..., addrs=0x0, add_flags=..., name=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", abfd=<optimized out>) at symfile.c:1204
#18 symbol_file_add (name=name@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", add_flags=add_flags@entry=..., addrs=addrs@entry=0x0, flags=flags@entry=...) at symfile.c:1217
#19 0x0000000000698742 in symbol_file_add_main_1 (args=args@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", add_flags=..., flags=flags@entry=..., reloff=reloff@entry=0) at symfile.c:1240
#20 0x000000000069878d in symbol_file_add_main (args=args@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", add_flags=..., add_flags@entry=...) at symfile.c:1231
#21 0x0000000000617663 in symbol_file_add_main_adapter (arg=arg@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", from_tty=from_tty@entry=1) at main.c:403
#22 0x00000000006176f8 in catch_command_errors (command=command@entry=0x617650 <symbol_file_add_main_adapter(char const*, int)>, arg=arg@entry=0x7fffffffe567 "/tmp/binutils-2.30/binutils/objdump", from_tty=1) at main.c:379
#23 0x000000000061859d in captured_main_1 (context=0x7fffffffe0b0, this=<optimized out>) at main.c:1053
#24 captured_main (data=0x7fffffffe0b0) at main.c:1163
#25 gdb_main (args=args@entry=0x7fffffffe1e0) at main.c:1189
#26 0x000000000040f1d5 in main (argc=<optimized out>, argv=<optimized out>) at gdb.c:32
Comment 1 Tom Tromey 2018-09-15 03:10:35 UTC
IIUC the problem is that sect->size comes from a header somewhere,
but does not reflect the true size of the section.  It took me a while
to understand this, because from the report I thought you meant that the
comparison could not possibly be true -- but really it could be ==.

I am not sure where the correct fix is.
I notice readelf gives a warning for this file:

readelf: /tmp/pr-23657: Warning: Size of section 31 is larger than the entire file!

So maybe gdb could do this check.
Or maybe BFD ought to issue an error because the file is corrupted.

CCing Nick - Nick, do you think this should be handled in BFD?

I couldn't reproduce directly but valgrind shows the error.
Comment 2 Nick Clifton 2018-09-17 12:13:07 UTC
Hi Tom,
 
> I am not sure where the correct fix is.
> I notice readelf gives a warning for this file:

So does objdump, if you use an option which tries to reference
into the debug string section:

  % objdump -S objdump
  objdump: error: objdump(.debug_str) is too large (0xffffffffffffffff bytes)

> So maybe gdb could do this check.
> Or maybe BFD ought to issue an error because the file is corrupted.
> 
> CCing Nick - Nick, do you think this should be handled in BFD?

Both would be my suggestion.  You can never be too paranoid...

There is also the caveat that the test "if (str_offset >= sect->size)"
might be wrong if the .debug_str section is compressed.  (I am unfamiliar 
with the gdb sources, so I do not know if sect->size is the size from the 
ELF header, or the size after decompression). 

Cheers
  Nick
Comment 3 Tom Tromey 2018-09-17 12:41:11 UTC
(In reply to Nick Clifton from comment #2)

> There is also the caveat that the test "if (str_offset >= sect->size)"
> might be wrong if the .debug_str section is compressed.  (I am unfamiliar 
> with the gdb sources, so I do not know if sect->size is the size from the 
> ELF header, or the size after decompression). 

For a compressed section it comes from:

  descriptor->size = bfd_get_section_size (sectp);

Where would a section size sanity check belong in BFD?
Maybe we'd only want it for sections we actually read, so
bfd_get_full_section_contents (and then a copy in gdb since
gdb can mmap sections).

I wonder if there are other sanity checks that should be done.
Comment 4 Nick Clifton 2018-09-18 13:47:05 UTC
Hi Tom,

(In reply to Tom Tromey from comment #3)

> Where would a section size sanity check belong in BFD?

Mmmm, well I think that when the file is first read in would 
be the most reasonable place.  I am going to experiment with
a patch to update elf_swap_shdr_in (in bfd/elfcode.h) and see
how that fairs.

> Maybe we'd only want it for sections we actually read, so
> bfd_get_full_section_contents (and then a copy in gdb since
> gdb can mmap sections).

That would make sense, although I think that we whould also
issue a warning message (but not an error) if we detect
something funny going on when the section headers are read in.

Cheers
  Nick
Comment 5 Sourceware Commits 2018-09-18 15:55:21 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 8ff71a9c80cfcf64c54d4ae938c644b1b1ea19fb
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Sep 18 16:54:07 2018 +0100

    Add a warning to the bfd library for when it encounters an ELF file with an invalid section size.
    
    	PR 23657
    	* elfcode.h (elf_swap_shdr_in): Generate a warning message if an
    	ELF section has contents and size larger than the file size.
Comment 6 Keith Seitz 2019-10-17 16:17:52 UTC
This should now be fixed.
Comment 7 Tom de Vries 2019-10-28 16:34:45 UTC
(In reply to Keith Seitz from comment #6)
> This should now be fixed.

Is it?

I can't reproduce the sigsegv before and after the commit, but using valgrind I can get a "Invalid read of size 1" before and after the commit.

AFAIU, the commit just adds a warning, which indeed triggers. But the commit doesn't fix the out of bounds memory access.
Comment 8 Keith Seitz 2019-10-28 16:59:31 UTC
(In reply to Tom de Vries from comment #7)
> (In reply to Keith Seitz from comment #6)
> > This should now be fixed.
> 
> Is it?

It does with the attached reproducer with 950b74950f6 (which doesn't
appear recorded here for some reason).

> I can't reproduce the sigsegv before and after the commit, but using
> valgrind I can get a "Invalid read of size 1" before and after the commit.
> 
> AFAIU, the commit just adds a warning, which indeed triggers. But the commit
> doesn't fix the out of bounds memory access.

The bfd patch recorded here does only add a warning. However, my commit
will emit another warning and skip the section entirely. There is no way
for the code reported by this bug to get triggered, i.e., the entire
.debug_str section will not be read.

That invalid read must be another bug -- one I did not know about.
Comment 9 Tom de Vries 2019-10-28 17:38:01 UTC
(In reply to Keith Seitz from comment #8)
> (In reply to Tom de Vries from comment #7)
> > (In reply to Keith Seitz from comment #6)
> > > This should now be fixed.
> > 
> > Is it?
> 
> It does with the attached reproducer with 950b74950f6 (which doesn't
> appear recorded here for some reason).

The commit 950b74950f6 "DWARF reader: Reject sections with invalid sizes" mentions PR gdb/23567.  That looks like a typo.

> That invalid read must be another bug -- one I did not know about.

I have tested before and after commit 8ff71a9c80c, as I was not aware of another commit.
Comment 10 Tom de Vries 2019-10-30 06:52:33 UTC
FTR: Fixed by: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=950b74950f6020eda38647f22e9077ac7f68ca49 :

DWARF reader: Reject sections with invalid sizes
author	Keith Seitz <keiths@redhat.com>	
	Wed, 16 Oct 2019 18:33:59 +0000 (11:33 -0700)
committer	Keith Seitz <keiths@redhat.com>	
	Wed, 16 Oct 2019 18:35:16 +0000 (11:35 -0700)
commit	950b74950f6020eda38647f22e9077ac7f68ca49
tree	6179c525842b477617cbb1b97965222454e69ae6	tree
parent	ff371ec99988662e16b061fe0f66e989340f129a	commit | diff
DWARF reader: Reject sections with invalid sizes

This is another fuzzer bug, gdb/23567.  This time, the fuzzer has
specifically altered the size of .debug_str:

$ eu-readelf -S objdump
Section Headers:
[Nr] Name                 Type         Addr             Off      Size     ES Flags Lk Inf Al
[31] .debug_str           PROGBITS     0000000000000000 0057116d ffffffffffffffff  1 MS     0   0  1

When this file is loaded into GDB, the DWARF reader crashes attempting
to access the string table (or it may just store a bunch of nonsense):

[gdb-8.3-6-fc30]
$ gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
Segmentation fault (core dumped)

Nick has already committed a BFD patch to issue the warning seen above.

[gdb master 6acc1a0b]
$ gdb -BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...
(gdb) inf func
All defined functions:

File ./../include/dwarf2.def:
186: const

              8 *>(.:
                     ;'@�B);
747: const

              8 *�(.:
                     ;'@�B);
701: const

              8 *�D �
                     (.:
                        ;'@�B);
71: const

              8 *(.:
                    ;'@�B);
/* and more gibberish  */

Consider read_indirect_string_at_offset_from:

static const char *
read_indirect_string_at_offset_from (struct objfile *objfile,
                                     bfd *abfd, LONGEST str_offset,
                                     struct dwarf2_section_info *sect,
                                     const char *form_name,
                                     const char *sect_name)
{
  dwarf2_read_section (objfile, sect);
  if (sect->buffer == NULL)
    error (_("%s used without %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  if (str_offset >= sect->size)
    error (_("%s pointing outside of %s section [in module %s]"),
           form_name, sect_name, bfd_get_filename (abfd));
  gdb_assert (HOST_CHAR_BIT == 8);
  if (sect->buffer[str_offset] == '\0')
    return NULL;
  return (const char *) (sect->buffer + str_offset);
}

With sect_size being ginormous, the code attempts to access
sect->buffer[GINORMOUS], and depending on the layout of memory,
GDB either stores a bunch of gibberish strings or crashes.

This is an attempt to mitigate this by implementing a similar approach
used by BFD. In our case, we simply reject the section with the invalid
length:

$ ./gdb -nx -q objdump
BFD: warning: /path/to/objdump has a corrupt section with a size (ffffffffffffffff) larger than the file size
Reading symbols from /path/to/objdump...

warning: Discarding section .debug_str which has a section size (ffffffffffffffff) larger than the file size [in module /path/to/objdump]
DW_FORM_strp used without .debug_str section [in module /path/to/objdump]
(No debugging symbols found in /path/to/objdump)
(gdb)

Unfortunately, I have not found a way to regression test this, since it
requires poking ELF section headers.

gdb/ChangeLog:
2019-10-16  Keith Seitz  <keiths@redhat.com>

PR gdb/23567
* dwarf2read.c (dwarf2_per_objfile::locate_sections): Discard
sections whose size is greater than the file size.

Change-Id: I896ac3b4eb2207c54e8e05c16beab3051d9b4b2f
Comment 11 Joel Brobecker 2019-12-12 22:55:35 UTC
setting target milestone to 9.1 since the change was pushed to master prior to the gdb-9-branch being created.
Comment 12 jonesRebecca 2021-02-03 08:37:15 UTC Comment hidden (spam)