[PATCH] Verify COFF symbol stringtab offset

[PATCH] Verify COFF symbol stringtab offset

[Keith Seitz]

This patch addresses an issue with malformed/fuzzed debug information that
was recently reported in gdb/30639. That bug specifically deals with
an ASAN issue, but the reproducer provided by the reporter causes a
another failure outside of ASAN:

$ ./gdb --data-directory data-directory -nx -q UAF_2
Reading symbols from /home/keiths/UAF_2...


Fatal signal: Segmentation fault
----- Backtrace -----
0x59a53a gdb_internal_backtrace_1
	../../src/gdb/bt-utils.c:122
0x59a5dd _Z22gdb_internal_backtracev
	../../src/gdb/bt-utils.c:168
0x786380 handle_fatal_signal
	../../src/gdb/event-top.c:889
0x7864ec handle_sigsegv
	../../src/gdb/event-top.c:962
0x7ff354c5fb6f ???
0x611f9a process_coff_symbol
	../../src/gdb/coffread.c:1556
0x611025 coff_symtab_read
	../../src/gdb/coffread.c:1172
0x60f8ff coff_read_minsyms
	../../src/gdb/coffread.c:549
0x60fe4b coff_symfile_read
	../../src/gdb/coffread.c:698
0xbde0f6 read_symbols
	../../src/gdb/symfile.c:772
0xbde7a3 syms_from_objfile_1
	../../src/gdb/symfile.c:966
0xbde867 syms_from_objfile
	../../src/gdb/symfile.c:983
0xbded42 symbol_file_add_with_addrs
	../../src/gdb/symfile.c:1086
0xbdf083 _Z24symbol_file_add_from_bfdRKN3gdb7ref_ptrI3bfd18gdb_bfd_ref_policyEEPKc10enum_flagsI16symfile_add_flagEPSt6vectorI14other_sectionsSaISC_EES8_I12objfile_flagEP7objfile
	../../src/gdb/symfile.c:1166
0xbdf0d2 _Z15symbol_file_addPKc10enum_flagsI16symfile_add_flagEPSt6vectorI14other_sectionsSaIS5_EES1_I12objfile_flagE
	../../src/gdb/symfile.c:1179
0xbdf197 symbol_file_add_main_1
	../../src/gdb/symfile.c:1203
0xbdf13e _Z20symbol_file_add_mainPKc10enum_flagsI16symfile_add_flagE
	../../src/gdb/symfile.c:1194
0x90f97f symbol_file_add_main_adapter
	../../src/gdb/main.c:549
0x90f895 catch_command_errors
	../../src/gdb/main.c:518
0x9109b6 captured_main_1
	../../src/gdb/main.c:1203
0x910fc8 captured_main
	../../src/gdb/main.c:1310
0x911067 _Z8gdb_mainP18captured_main_args
	../../src/gdb/main.c:1339
0x418c71 main
	../../src/gdb/gdb.c:39
---------------------
A fatal error internal to GDB has been detected, further
debugging is not possible.  GDB will now terminate.

This is a bug, please report it.  For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.

Segmentation fault (core dumped)

The issue here is that the COFF offset for the fuzzed symbol's
name is outside the string table. That is, the offset is greater
than the actual string table size.

coffread.c:getsymname actually contains a FIXME about this, and that's
what I've chosen to address to fix this issue, following what is done
in the DWARF reader:

$ ./gdb --data-directory data-directory -nx -q UAF_2
Reading symbols from /home/keiths/UAF_2...
COFF Error: string table offset (256) outside string table (length 0)
(gdb)

Unfortunately, I haven't any idea how else to test this patch since
COFF is not very common anymore. GCC removed support for it five
years ago with GCC 8.
---
 gdb/coffread.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index 13610998ad7..e00f5c55e4f 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -159,6 +159,7 @@ static file_ptr linetab_offset;
 static file_ptr linetab_size;
 
 static char *stringtab = NULL;
+static long stringtab_length = 0;
 
 extern void stabsread_clear_cache (void);
 
@@ -1303,6 +1304,7 @@ init_stringtab (bfd *abfd, file_ptr offset, gdb::unique_xmalloc_ptr<char> *stora
   /* This is in target format (probably not very useful, and not
      currently used), not host format.  */
   memcpy (stringtab, lengthbuf, sizeof lengthbuf);
+  stringtab_length = length;
   if (length == sizeof length)	/* Empty table -- just the count.  */
     return 0;
 
@@ -1322,8 +1324,9 @@ getsymname (struct internal_syment *symbol_entry)
 
   if (symbol_entry->_n._n_n._n_zeroes == 0)
     {
-      /* FIXME: Probably should be detecting corrupt symbol files by
-	 seeing whether offset points to within the stringtab.  */
+      if (symbol_entry->_n._n_n._n_offset > stringtab_length)
+	error (_("COFF Error: string table offset (%ld) outside string table (length %ld)"),
+	       symbol_entry->_n._n_n._n_offset, stringtab_length);
       result = stringtab + symbol_entry->_n._n_n._n_offset;
     }
   else
-- 
2.41.0

Re: [PATCH] Verify COFF symbol stringtab offset

[Tom Tromey]

>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:

Keith> This patch addresses an issue with malformed/fuzzed debug information that
Keith> was recently reported in gdb/30639. That bug specifically deals with
Keith> an ASAN issue, but the reproducer provided by the reporter causes a
Keith> another failure outside of ASAN:

Thank you.

Keith> Unfortunately, I haven't any idea how else to test this patch since
Keith> COFF is not very common anymore. GCC removed support for it five
Keith> years ago with GCC 8.

If it is really that dead, then -- like stabs -- I'd like to remove it.
These dead formats just make it harder to improve gdb while not
providing value to any user.

Actually with the advent of fuzzers, this code now has negative value,
because it wastes time coming up with patches to fix bugs in code no one
uses.

Hmm, I should probably go reply on the stabs thread.

Anyway, this is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] Verify COFF symbol stringtab offset

[Keith Seitz]

On 8/22/23 08:46, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Keith> Unfortunately, I haven't any idea how else to test this patch since
> Keith> COFF is not very common anymore. GCC removed support for it five
> Keith> years ago with GCC 8.
> 
> If it is really that dead, then -- like stabs -- I'd like to remove it.
> These dead formats just make it harder to improve gdb while not
> providing value to any user.
> 
> Actually with the advent of fuzzers, this code now has negative value,
> because it wastes time coming up with patches to fix bugs in code no one
> uses.

I was tempted to suggest either removing it or at least adding a --disable-reader-coff
option to disable the code, but I could not convince myself that there
wouldn't be relatively widespread disagreement.

If you feel there's a chance, let me know if you like either of these approaches,
and I will get it on my TODO list.

> Hmm, I should probably go reply on the stabs thread.
> 
> Anyway, this is ok.

Thank you again for your quick review! Pushed  58abdf88782.

Keith

Re: [PATCH] Verify COFF symbol stringtab offset

[Tom Tromey]

>>>>> "Keith" == Keith Seitz via Gdb-patches <gdb-patches@sourceware.org> writes:

>> If it is really that dead, then -- like stabs -- I'd like to remove
>> it.  These dead formats just make it harder to improve gdb while not
>> providing value to any user.  Actually with the advent of fuzzers,
>> this code now has negative value, because it wastes time coming up
>> with patches to fix bugs in code no one uses.

Keith> I was tempted to suggest either removing it or at least adding a
Keith> --disable-reader-coff option to disable the code, but I could not
Keith> convince myself that there wouldn't be relatively widespread
Keith> disagreement.

If we think the format is truly obsolete, then IMO it is better to just
delete it.

I never got back to replying about the stabs situation, but after Pedro
weighed in, I think we should move ahead with that.  However, we're
close-ish to GDB 14 now, so I plan to wait until after that branches.

The main problem for other formats is even knowing if they are in use.
Searching for buildsym-legacy, I see coffread (I guess COFF had some
kind of debuginfo?), dbxread (a.out but maybe stabs-like?), mdebugread
(ECOFF), xcoffread (yet another COFF).

My guess is that maybe only xcoffread could even be remotely relevant
but I don't really know how to tell.

Tom