[Crash-utility] [PATCH] gdb: verify COFF symbol stringtab offset

[HAGIO KAZUHITO(萩尾 一仁)]

On 2023/10/27 12:44, lijiang wrote:
> On Wed, Oct 25, 2023 at 4:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
> wrote:
> 
>> On 2023/10/24 18:10, Lianbo Jiang wrote:
>>> This is a backport patch from gdb commit 58abdf887821 ("Verify COFF
>>> symbol stringtab offset").
>>>
>>> The AddressSanitizer reports a heap-use-after-free error as below:
>>>     gdb/coff-pe-read.c:137:27
>>>
>>> Add a COFF offset check to fix this issue.
>>>
>>> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=30640
>>> Signed-off-by: Lianbo Jiang <lijiang at redhat.com>
>>
>> Thank you for working on this issue.
>>
>>> ---
>>> Please see the CVE-2023-39129.
>>>
>>>    gdb-10.2.patch | 31 +++++++++++++++++++++++++++++++
>>>    1 file changed, 31 insertions(+)
>>>
>>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
>>> index 7f4b86350bde..98538dc1138b 100644
>>> --- a/gdb-10.2.patch
>>> +++ b/gdb-10.2.patch
>>> @@ -3156,4 +3156,35 @@ exit 0
>>>    +      else if (i >= 0 && encoded[i] == '$')
>>>             len0 = i;
>>>         }
>>> +
>>
>> No actual problem, but this empty '+' line is some strange... my working
> 
> 
> Thank you for the comment, Kazu.
> 
> After removing the empty "+" line, I got the following information during
> compiling process.
> ...
> patching file gdb-10.2/gdb/coffread.c
> Hunk #3 succeeded at 1318 with fuzz 1.

yes, because there is need to shift a line like this:

--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -3157,3 +3157,33 @@ exit 0
           len0 = i;
       }

+--- gdb-10.2/gdb/coffread.c.orig
++++ gdb-10.2/gdb/coffread.c

> ...
> 
> But, maybe we can ignore the above output.
> 
> 
>>
>> tree at a8e5e4cbae54 already has a space line at the end of file:
>>
>> $ tail -n 5 gdb-10.2.patch | tr ' ' '-'
>> -------else-if-(encoded[i]-==-'$')
>> +------else-if-(i->=-0-&&-encoded[i]-==-'$')
>> ---------len0-=-i;
>> -----}
>> -
>>
>> but anyway, I can fix this while merging.
>>
>> Ok.
> 
> 
>>> +--- gdb-10.2/gdb/coffread.c.orig
>>> ++++ gdb-10.2/gdb/coffread.c
>>> +@@ -159,6 +159,7 @@ static long linetab_offset;
>>> + static unsigned long linetab_size;
>>> +
>>> + static char *stringtab = NULL;
>>> ++static long stringtab_length = 0;
>>> +
>>> + extern void stabsread_clear_cache (void);
>>> +
>>> +@@ -1297,6 +1298,7 @@ init_stringtab (bfd *abfd, long offset,
>> gdb::unique_xmalloc_ptr<char> *storage)
>>> +   /* 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;
>>> +
>>> +@@ -1316,8 +1318,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
>>>
>>
>> Just out of curiosity, do you have any information that crash can be
> 
> 
> I did not reproduce it in crash-utility, and got the bug from the reporter.

OK, thanks.

> 
> 
>>
>> affected by this issue?  I don't know about the COFF, I wonder if the
> 
> 
> Me too.
> 
> embedded gdb does not use this, aside from an independent gdb..
>>
>>
> I just saw that the coff format was removed from gcc 8, so probably the gdb
> does not use the coff format.
> 
> I copied the following text from the link:
> ---
> Caveats
> Support for the obsolete SDB/coff debug info format has been removed. The
> option -gcoff no longer does anything.
> ...
> ---
> Here is the link: https://gcc.gnu.org/gcc-8/changes.html
> 
> But I'm not sure if there are people who might compile crash-utility with
> the old gcc( eg. < gcc 8), if no, we can drop this patch, and no need to
> fix it. Any thoughts?

I think that what compiles crash doesn't matter.  Apparently the issue 
was reproduced by a crafted debuginfo.

https://inbox.sourceware.org/gdb-patches/20230822152335.231921-1-keiths@redhat.com/T/#u

Crash may not be affected with crash-specific checks, but it will be 
very hard to prove it.  GDB maintainers decided to fix this, let's have 
it in the same way..  Hopefully it's removed in future :)

So applied.
https://github.com/crash-utility/crash/commit/fc6ed525407ffc6a181ab9b902f2fb23936309d2

Thanks,
Kazu