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

[lijiang]

On Fri, Oct 27, 2023 at 1:12 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:

> 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:
>
> Good to me, thanks.


> --- 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
>
>
Ok, thanks.

Lianbo


> Thanks,
> Kazu
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20231027/3a7c5e73/attachment.htm>