Bug 28659 - UBSan seems to complain about an "integer overflow" in dwfl_segment_report_module
Summary: UBSan seems to complain about an "integer overflow" in dwfl_segment_report_mo...
Status: RESOLVED FIXED
Alias: None
Product: elfutils
Classification: Unclassified
Component: libdw (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Mark Wielaard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-06 13:01 UTC by Evgeny Vereshchagin
Modified: 2021-12-09 20:24 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2021-12-08 00:00:00


Attachments
File triggering an integer overflow (63 bytes, application/octet-stream)
2021-12-06 13:01 UTC, Evgeny Vereshchagin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Evgeny Vereshchagin 2021-12-06 13:01:19 UTC
Created attachment 13825 [details]
File triggering an integer overflow

One of systemd fuzz targets run under UBSan discovered an "integer-overflow". It can be reproduced by building the master branch of the elfutils repository with UBSan and passing the attachment to `./src/stack`:
```
$ git describe
elfutils-0.186-7-g99782bd2

autoreconf -i -f
./configure --enable-maintainer-mode CFLAGS='-g -O1 -fno-omit-frame-pointer' --enable-sanitize-undefined
make -j$(nproc) V=1
LD_PRELOAD="./libelf/libelf.so ./libdw/libdw.so" UBSAN_OPTIONS=print_stacktrace=1:print_summary=1  ./src/stack --core ../oss-fuzz-41570
dwfl_segment_report_module.c:400:55: runtime error: signed integer overflow: 65535 * 65312 cannot be represented in type 'int'
    #0 0x7f48c29c057e in dwfl_segment_report_module /home/vagrant/elfutils/libdwfl/dwfl_segment_report_module.c:400
    #1 0x7f48c29c7656 in _new.dwfl_core_file_report /home/vagrant/elfutils/libdwfl/core-file.c:559
    #2 0x402b0f in parse_opt /home/vagrant/elfutils/src/stack.c:595
    #3 0x7f48c1c42f81 in __argp_parse (/lib64/libc.so.6+0x10cf81)
    #4 0x403d98 in main /home/vagrant/elfutils/src/stack.c:695
    #5 0x7f48c1b5db74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
    #6 0x4024cd in _start (/home/vagrant/elfutils/src/stack+0x4024cd)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior dwfl_segment_report_module.c:400:55 in
```

It was also reported in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=41570
```
	Running: /mnt/scratch0/clusterfuzz/bot/inputs/fuzzer-testcases/crash-4613ca6be3014e2d8019781ba5061e2ebaad886f
dwfl_segment_report_module.c:400:55: runtime error: signed integer overflow: 65535 * 65312 cannot be represented in type 'int'
    #0 0x51915d in dwfl_segment_report_module /src/elfutils/libdwfl/dwfl_segment_report_module.c:400:55
    #1 0x4b86e7 in dwfl_core_file_report /src/elfutils/libdwfl/core-file.c:559:17
    #2 0x4b363e in LLVMFuzzerTestOneInput /src/fuzz-dwfl-core.c:52:6
    #3 0x43f1b3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp:0
    #4 0x42aac2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:324:6
    #5 0x43058a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp:0
    #6 0x4594b2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #7 0x7f0cfd13e0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/libc-start.c:308:16
    #8 0x407d4d in _start
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior dwfl_segment_report_module.c:400:55 in
```
Comment 1 Mark Wielaard 2021-12-08 21:43:04 UTC
Proposed patch:
https://sourceware.org/pipermail/elfutils-devel/2021q4/004475.html

Note that the overflow is actually harmless, the shdrs_end is sanity checked later in the code and we don't really need the shdrs (like the comment just before this code already explains).
Comment 2 Evgeny Vereshchagin 2021-12-08 22:39:58 UTC
> Note that the overflow is actually harmless

It is but since the fuzz target ran into it almost as soon as it started it prevented the fuzz target from discovering new issues that can be less harmless though.

Looks like the issue is gone. Thanks!

FWIW judging by https://github.com/evverx/elfutils/pull/40#issuecomment-989275575, it fixed one LGTM alert as well. I'm not sure if I mentioned this anywhere but LGTM builds those reports on a daily basis and those reports can be found at https://lgtm.com/projects/g/evverx/elfutils/alerts/?mode=tree .
Comment 3 Mark Wielaard 2021-12-09 19:30:02 UTC
Thanks for testing, pushed as:

commit b9ed67836b6f4e580927b4e8e1c8517e70a086be
Author: Mark Wielaard <mark@klomp.org>
Date:   Wed Dec 8 22:20:17 2021 +0100

    libdwfl: Don't trust e_shentsize in dwfl_segment_report_module
    
    When calulating the possible section header table end us the actual size
    of the section headers (sizeof (Elf32_Shdr) or sizeof (Elf64_Shdr)),
    not the ELF header e_shentsize value, which can be corrupted. This
    prevents a posssible overflow, but we check the shdrs_end is sane
    later anyway.
    
    https://sourceware.org/bugzilla/show_bug.cgi?id=28659
    
    Signed-off-by: Mark Wielaard <mark@klomp.org>

> it fixed one LGTM alert as well. I'm not sure if I mentioned this anywhere but
> LGTM builds those reports on a daily basis and those reports can be found at
> https://lgtm.com/projects/g/evverx/elfutils/alerts/?mode=tree .

Hmmm. At first I thought this was pretty useful to add to our own buildbot CI setup. But it comes with a horribly proprietary license :{ "CodeQL can’t be used for automated analysis, continuous integration or continuous delivery" Sigh.
Comment 4 Evgeny Vereshchagin 2021-12-09 20:24:22 UTC
> But it comes with a horribly proprietary license

Unfortunately LGTM (like many other CI services) is tightly coupled with GitHub (where it can be used for automated analysis of open source projects) so in theory it should be possible to keep a read-only fork of the elfutils repository updated by a cron script run by buildbot. I'm not sure whether it's worth it though.