Some fuzzer workarounds

Some fuzzer workarounds

[Mark Wielaard]

Hi,

I looked over the "ClusterFuzz-External via monorail" emails and found
some "real" issues. But in general it is hard to determined what this
cluster is complaining about. The emails are somewhat opaque and don't
contain proper backtraces (with file and line numbers), nor do they
contain any context on how the target was configured or with which
flags or arguments any fuzzing testcases were run.

The following fixes should fix reading of some broken ar archives and
misaligned access of the section zero Shdr for mmaped ELF files where
the start of the Elf image is at some offset from the start of the
map.

[PATCH 1/2] libelf: Take map offset into account for Shdr alignment
[PATCH 2/2] libelf: Make sure ar_size starts with a digit before

https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=fuzz

I haven't been able to replicate any other issues locally.  I don't
really trust the msan instrumentation, better use valgrind (although
both might be too slow for fuzzing).  There are also some other
misaligned type access checks reported by ubsan, but I don't know if
that is because of ALLOW_UNALIGNED is still defined or not (when
configuring with --enable-analyze-undefined ALLOW_UNALIGNED is not
defined, otherwise it is for some arches, including x86_64).

I don't mind getting rid of ALLOW_UNALIGNED, but it is some work.

Cheers,

Mark

[PATCH 1/2] libelf: Take map offset into account for Shdr alignment check in elf_begin

[Mark Wielaard]

The sh_num function tries to get at the zero section Shdr directly.
When the file is mmapped it has to make sure the offset into the file
to the start of the Elf structure is taken into account when trying to
cast the address to make sure the alignment is correct.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog   | 5 +++++
 libelf/elf_begin.c | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 29a8aae1..1883af07 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2022-03-17  Mark Wielaard  <mark@klomp.org>
+
+	* elf_begin.c (get_shnum): Take offset into account for Shdr
+	alignment check.
+
 2021-12-19  Mark Wielaard  <mark@klomp.org>
 
 	* elf_begin.c (file_read_elf): Cast ehdr to uintptr_t before e_shoff
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 0c9a988d..03b80185 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -158,7 +158,8 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes,
 
 	  if (likely (map_address != NULL) && e_ident[EI_DATA] == MY_ELFDATA
 	      && (ALLOW_UNALIGNED
-		  || (((size_t) ((char *) map_address + ehdr.e32->e_shoff))
+		  || (((size_t) ((char *) (map_address + ehdr.e32->e_shoff
+					   + offset)))
 		      & (__alignof__ (Elf32_Shdr) - 1)) == 0))
 	    /* We can directly access the memory.  */
 	    result = ((Elf32_Shdr *) ((char *) map_address + ehdr.e32->e_shoff
@@ -218,7 +219,8 @@ get_shnum (void *map_address, unsigned char *e_ident, int fildes,
 	  Elf64_Xword size;
 	  if (likely (map_address != NULL) && e_ident[EI_DATA] == MY_ELFDATA
 	      && (ALLOW_UNALIGNED
-		  || (((size_t) ((char *) map_address + ehdr.e64->e_shoff))
+		  || (((size_t) ((char *) (map_address + ehdr.e64->e_shoff
+					   + offset)))
 		      & (__alignof__ (Elf64_Shdr) - 1)) == 0))
 	    /* We can directly access the memory.  */
 	    size = ((Elf64_Shdr *) ((char *) map_address + ehdr.e64->e_shoff
-- 
2.30.2

[PATCH 2/2] libelf: Make sure ar_size starts with a digit before calling atol.

[Mark Wielaard]

The ar_size field is a 10 character string, not zero terminated, of
decimal digits right padded with spaces.  Make sure it actually starts
with a digit before calling atol on it.  We already make sure it is
zero terminated. Otherwise atol might produce unexpected results.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog   | 4 ++++
 libelf/elf_begin.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 1883af07..07dd905f 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,7 @@
+2022-03-17  Mark Wielaard  <mark@klomp.org>
+
+	* elf_begin.c (read_long_names): Check ar_size starts with a digit.
+
 2022-03-17  Mark Wielaard  <mark@klomp.org>
 
 	* elf_begin.c (get_shnum): Take offset into account for Shdr
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 03b80185..917e0c71 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -765,6 +765,11 @@ read_long_names (Elf *elf)
 	  *((char *) mempcpy (buf, hdr->ar_size, sizeof (hdr->ar_size))) = '\0';
 	  string = buf;
 	}
+
+      /* atol expects to see at least one digit.
+	 It also cannot be negative (-).  */
+      if (!isdigit(string[0]))
+	return NULL;
       len = atol (string);
 
       if (memcmp (hdr->ar_name, "//              ", 16) == 0)
-- 
2.30.2

Re: [PATCH 2/2] libelf: Make sure ar_size starts with a digit before calling atol.

[Evgeny Vereshchagin]

Hi,

> The ar_size field is a 10 character string, not zero terminated, of
> decimal digits right padded with spaces.  Make sure it actually starts
> with a digit before calling atol on it.  We already make sure it is
> zero terminated. Otherwise atol might produce unexpected results.

As far as I can tell the patch fixes that particular issue. Thanks! 

On a somewhat related note, looking at https://sourceware.org/bugzilla/show_bug.cgi?id=24085
where read_long_names started appending a trailing '\0' to strings without trailing spaces only I wonder if it would be better
to always append trailing zero bytes there? It would make ASan stop complaining
about read_long_names with ASAN_OPTIONS=strict_string_checks=1 (which is supposed to look for places where strings without trailing zeroes are passed
to functions expecting null-terminated strings). For example even with this patch applied run-arextract.sh seems to fail under
ASAN with strict_string_checks=1:
```
ASAN_OPTIONS=strict_string_checks=1 make check TESTS=run-arextract.sh VERBOSE=1
...
==126042==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd5d103c7c at pc 0x7fe87890156d bp 0x7ffd5d103ab0 sp 0x7ffd5d103260
READ of size 15 at 0x7ffd5d103c7c thread T0
    #0 0x7fe87890156c in StrtolFixAndCheck(void*, char const*, char**, char*, int) (/lib64/libasan.so.6+0x6b56c)
    #1 0x7fe878901865 in __interceptor_strtol (/lib64/libasan.so.6+0x6b865)
    #2 0x7fe87885ecf9 in atol /usr/include/stdlib.h:368
    #3 0x7fe87885ecf9 in read_long_names /home/vagrant/oss-fuzz/projects/elfutils/elfutils/libelf/elf_begin.c:773
    #4 0x7fe87885ecf9 in __libelf_next_arhdr_wrlock /home/vagrant/oss-fuzz/projects/elfutils/elfutils/libelf/elf_begin.c:919
    #5 0x7fe87885fb25 in elf_next /home/vagrant/oss-fuzz/projects/elfutils/elfutils/libelf/elf_next.c:63
    #6 0x401360 in main /home/vagrant/oss-fuzz/projects/elfutils/elfutils/tests/arextract.c:146
    #7 0x7fe87867555f in __libc_start_call_main (/lib64/libc.so.6+0x2d55f)
    #8 0x7fe87867560b in __libc_start_main_alias_1 (/lib64/libc.so.6+0x2d60b)
    #9 0x401654 in _start (/home/vagrant/oss-fuzz/projects/elfutils/elfutils/tests/arextract+0x401654)

Address 0x7ffd5d103c7c is located in stack of thread T0 at offset 284 in frame
    #0 0x7fe87885dbbf in __libelf_next_arhdr_wrlock /home/vagrant/oss-fuzz/projects/elfutils/elfutils/libelf/elf_begin.c:852
```
(In this particular case it's a false positive because in practice it's safe to pass strings like that there though)

Thanks,
Evgeny Vereshchagin

Re: [PATCH 2/2] libelf: Make sure ar_size starts with a digit before calling atol.

[Mark Wielaard]

Hi Evgeny,

On Fri, Mar 18, 2022 at 12:11:50PM +0300, Evgeny Vereshchagin wrote:
> > The ar_size field is a 10 character string, not zero terminated, of
> > decimal digits right padded with spaces.  Make sure it actually starts
> > with a digit before calling atol on it.  We already make sure it is
> > zero terminated. Otherwise atol might produce unexpected results.
> 
> As far as I can tell the patch fixes that particular issue. Thanks! 

Thanks for testing.

> On a somewhat related note, looking at
> https://sourceware.org/bugzilla/show_bug.cgi?id=24085 where
> read_long_names started appending a trailing '\0' to strings without
> trailing spaces only I wonder if it would be better to always append
> trailing zero bytes there? It would make ASan stop complaining about
> read_long_names with ASAN_OPTIONS=strict_string_checks=1 (which is
> supposed to look for places where strings without trailing zeroes
> are passed to functions expecting null-terminated strings).

I guess the idea is that there could be an atoi implementation that
starts from the end of the string? But I think that is super unlikely
since atoi (and strtol) is defined on the initial portion of the
character array. The algorithm is described as working from the start
and once a valid digit is found any non-digit terminates the
algorithm, there seems to be no requirement that that char should be a
zero terminator. So I think that asan strict-string check is not
really correct.

Also since the ar_size is defined as a character array that only
contains digits and (right padded) spaces (but no zero terminator), we
would have to copy the chars always if we would add a zero
terminator. Which is very unlikely (except when the size is larger
than 999999999 bytes, 953 MB.

Cheers,

Mark

Re: [PATCH 2/2] libelf: Make sure ar_size starts with a digit before calling atol.

[Evgeny Vereshchagin]

Hi Mark,

> I guess the idea is that there could be an atoi implementation that
> starts from the end of the string? But I think that is super unlikely
> since atoi (and strtol) is defined on the initial portion of the
> character array. The algorithm is described as working from the start
> and once a valid digit is found any non-digit terminates the
> algorithm, there seems to be no requirement that that char should be a
> zero terminator. So I think that asan strict-string check is not
> really correct.

The idea behind strict_string_checks is to just warn about functions expecting
null-terminated strings that process (potentially) binary data and can in theory get past the end
of the buffers because of that. It just looks for nulls and if they aren't there it complains.
It's off by default because it tends to produce false positives. But I think it's useful sometimes because
for example as far as I can remember it was able to find real heap-buffer-overflows in systemd at some point
and it has been on on the CI there since "string" functions were replaced with functions receiving buffers
and their lengths. Then again, I agree it doesn't seem to make much sense to make ASan happy here.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi,

> I looked over the "ClusterFuzz-External via monorail" emails and found
> some "real" issues.

Given that the new fuzz targets seem to just fail to compile with
```
projects/elfutils/fuzz-libdwfl.c:48:10: error: unused variable 'res' [-Werror,-Wunused-variable]
  Dwarf *res = dwfl_module_getdwarf(mod, &bias);
         ^
1 error generated.
```
I think before looking at those reports it would make sense to figure out what they are supposed to
test and how they were tested to make sure they don't produce false positives. If they
weren't actually tested I think it would make sense to revert them to avoid getting auto-generated CVEs
until they're in more or less good shape at least.

> There are also some other
> misaligned type access checks reported by ubsan, but I don't know if
> that is because of ALLOW_UNALIGNED is still defined or not (when
> configuring with --enable-analyze-undefined ALLOW_UNALIGNED is not
> defined, otherwise it is for some arches, including x86_64).

Looking at https://github.com/google/oss-fuzz/commit/8747524f04b1b906d4a21a6ade87f7803b3f9b8c, I think
I turned ALLOW_UNALIGNED off with UBSan there (and tested it in https://sourceware.org/bugzilla/show_bug.cgi?id=28720).

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi

> If they weren't actually tested I think it would make sense to revert them to avoid getting auto-generated CVEs
> until they're in more or less good shape at least.

I've just opened https://github.com/google/oss-fuzz/pull/7401 to weed out some false positives. 
Given that they are "security" issues and bash scripts generating CVEs rely on that label I hope they will be closed
as "invalid" or "wonfix". The issues found by fuzz-elf-get-sections (which was renamed to fuzz-libelf apparently) were
closed as "Verified" though so I'm not sure how it works exactly.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi

> Given that the new fuzz targets seem to just fail to compile with
> ```
> projects/elfutils/fuzz-libdwfl.c:48:10: error: unused variable 'res' [-Werror,-Wunused-variable]
>  Dwarf *res = dwfl_module_getdwarf(mod, &bias);
>         ^
> 1 error generated.
> ```

I've just opened https://github.com/google/oss-fuzz/pull/7408 where the fuzz targets are built with -Werror -Wall -Wextra
among other things.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Mark Wielaard]

Hi,

On Fri, Mar 18, 2022 at 10:26:16AM +0300, Evgeny Vereshchagin wrote:
> I think before looking at those reports it would make sense to
> figure out what they are supposed to test and how they were tested
> to make sure they don't produce false positives. If they weren't
> actually tested I think it would make sense to revert them to avoid
> getting auto-generated CVEs until they're in more or less good shape
> at least.

So I took the fuzz-libelf.c and fuzz-libdwfl.c files from the oss-fuzz
repo, tweaked them so they have a normal main that takes one file
argument to try to replicate the reports. That found some "real"
issues I submitted patches for. Then I ran afl-fuzz on them locally
during the weekend and found another issue for which I also submitted
a patch. There are several duplicates though and all the MSAN reported
issues seem bogus.

> > There are also some other
> > misaligned type access checks reported by ubsan, but I don't know if
> > that is because of ALLOW_UNALIGNED is still defined or not (when
> > configuring with --enable-analyze-undefined ALLOW_UNALIGNED is not
> > defined, otherwise it is for some arches, including x86_64).
> 
> Looking at
> https://github.com/google/oss-fuzz/commit/8747524f04b1b906d4a21a6ade87f7803b3f9b8c,
> I think I turned ALLOW_UNALIGNED off with UBSan there (and tested it
> in https://sourceware.org/bugzilla/show_bug.cgi?id=28720).

Yes, you are right, all the unaligned type access issues were "real"
(they occur with or without ALLOW_UNALIGNED on). It is not really a
big issue, but could potentially cause the compiler to do some
surprising optimisations. So I did fix them all.

Cheers,

Mark

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi Mark,

> So I took the fuzz-libelf.c and fuzz-libdwfl.c files from the oss-fuzz
> repo, tweaked them so they have a normal main that takes one file
> argument to try to replicate the reports. That found some "real"
> issues I submitted patches for. Then I ran afl-fuzz on them locally
> during the weekend and found another issue for which I also submitted
> a patch.

FWIW to test the "fuzz" branch I integrated the new fuzz targets into the elfutils build system
by analogy with https://sourceware.org/pipermail/elfutils-devel/2021q4/004615.html and
there they are linked with the main function automatically and it's also possible to pass --enable-afl
to ./configure to automatically run it with AFL. It still needs polishing but I wonder if it makes sense
to send that to the mailing list? I still think the fuzz targets should be kept and reviewed upstream
and that patch would make it possible.

It's being tested in https://github.com/evverx/elfutils/pull/72. I'll report back once I figure
out why the unit tests are failing on Fedora Rawhide:
https://copr-be.cloud.fedoraproject.org/results/packit/evverx-elfutils-72/fedora-rawhide-x86_64/03799633-elfutils/builder-live.log.gz


> There are several duplicates though and all the MSAN reported
> issues seem bogus.


I'm not sure all of them are bogus but I would ignore them for now. Once the new fuzz targets
are linked with zlib built with MSan bogus reports will be closed and I'll take a look at what's left
there.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi Mark,

> I'll report back once I figure
> out why the unit tests are failing on Fedora Rawhide:
> https://copr-be.cloud.fedoraproject.org/results/packit/evverx-elfutils-72/fedora-rawhide-x86_64/03799633-elfutils/builder-live.log.gz
> 

I tested the fuzz branch and I can confirm that all the issues reported by OSS-Fuzz found with ASan+UBSan are gone.
I kind of lost track of them at some point but the following issues can no longer be triggered:

             fuzz-libdwfl-crashes/oss-fuzz-45629 \
             fuzz-libdwfl-crashes/oss-fuzz-45634 \
             fuzz-libdwfl-crashes/oss-fuzz-45635 \
             fuzz-libdwfl-crashes/oss-fuzz-45636 \
             fuzz-libdwfl-crashes/oss-fuzz-45646 \
             fuzz-libelf-crashes/oss-fuzz-45637 \
             fuzz-libelf-crashes/oss-fuzz-45682

The unit tests have nothing to do with the fuzz branch because once I pointed the tests to the master branch they also
failed. Looking at "phdr[8]: unknown object file note type 3405650558 with owner name 'FDO' at offset 200" it seems
it's caused by the toolchain used there. On Fedora 35 the tests pass.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Mark Wielaard]

Hi Evgeny,

On Mon, 2022-03-21 at 17:33 +0300, Evgeny Vereshchagin wrote:
> I tested the fuzz branch and I can confirm that all the issues
> reported by OSS-Fuzz found with ASan+UBSan are gone.
> I kind of lost track of them at some point but the following issues
> can no longer be triggered:
> 
>              fuzz-libdwfl-crashes/oss-fuzz-45629 \
>              fuzz-libdwfl-crashes/oss-fuzz-45634 \
>              fuzz-libdwfl-crashes/oss-fuzz-45635 \
>              fuzz-libdwfl-crashes/oss-fuzz-45636 \
>              fuzz-libdwfl-crashes/oss-fuzz-45646 \
>              fuzz-libelf-crashes/oss-fuzz-45637 \
>              fuzz-libelf-crashes/oss-fuzz-45682

Great. Thanks for testing. All patches from the fuzz branch are now
merged. My local fuzzer also hasn't found any new issues for almost 24
hours now.

> The unit tests have nothing to do with the fuzz branch because once I
> pointed the tests to the master branch they also
> failed. Looking at "phdr[8]: unknown object file note type 3405650558
> with owner name 'FDO' at offset 200" it seems
> it's caused by the toolchain used there. On Fedora 35 the tests pass.

Ah, oops. Yeah that is:
https://systemd.io/COREDUMP_PACKAGE_METADATA/
https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects

There are actually patches to properly recognize those.
https://sourceware.org/pipermail/elfutils-devel/2021q4/thread.html#4375
I'll integrate those asap.

Cheers,

Mark

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi Mark,

> Great. Thanks for testing. All patches from the fuzz branch are now
> merged. My local fuzzer also hasn't found any new issues for almost 24
> hours now.

Thanks! I synced my fork with the elfutils repository and tonight it will be sent
to Coverity. If anything pops up I'll report it.

> Yeah that is:
> https://systemd.io/COREDUMP_PACKAGE_METADATA/
> https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects

That looks familiar. As far as I can remember fuzz-dwfl-core was introduced to fuzz that
part of systemd.

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi Mark,

>> So I took the fuzz-libelf.c and fuzz-libdwfl.c files from the oss-fuzz
>> repo, tweaked them so they have a normal main that takes one file
>> argument to try to replicate the reports. That found some "real"
>> issues I submitted patches for. Then I ran afl-fuzz on them locally
>> during the weekend and found another issue for which I also submitted
>> a patch.
> 
> FWIW to test the "fuzz" branch I integrated the new fuzz targets into the elfutils build system
> by analogy with https://sourceware.org/pipermail/elfutils-devel/2021q4/004615.html and
> there they are linked with the main function automatically and it's also possible to pass --enable-afl
> to ./configure to automatically run it with AFL.

I sent it to the mailing list: https://sourceware.org/pipermail/elfutils-devel/2022q1/004767.html

> 
>> There are several duplicates though and all the MSAN reported
>> issues seem bogus.
> 
> 
> I'm not sure all of them are bogus but I would ignore them for now. Once the new fuzz targets
> are linked with zlib built with MSan bogus reports will be closed and I'll take a look at what's left
> there.


I can also prevent OSS-Fuzz from reporting new bugs found by MSan
by setting the experimental flag

From https://google.github.io/oss-fuzz/getting-started/new-project-guide/#sanitizers
> If you want to test a particular sanitizer to see what crashes it generates
> without filing them in the issue tracker, you can set an experimental flag

It should help to figure out whether it makes sense to keep it without spamming the mailing list
in the process. What do you think?

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Mark Wielaard]

Hi Evgeny,

On Tue, Mar 22, 2022 at 07:59:57PM +0300, Evgeny Vereshchagin wrote:
> I can also prevent OSS-Fuzz from reporting new bugs found by MSan
> by setting the experimental flag
> 
> From https://google.github.io/oss-fuzz/getting-started/new-project-guide/#sanitizers
> > If you want to test a particular sanitizer to see what crashes it generates
> > without filing them in the issue tracker, you can set an experimental flag
> 
> It should help to figure out whether it makes sense to keep it without spamming the mailing list
> in the process. What do you think?

I think that is a good idea. I really believe all the issues reported
by MSAN are bogus.

While the UBSAN and ASAN issues seem reasonable. At least I have a fix
for the last one (45952 Misaligned-address in elf_cvt_gnuhash):
https://sourceware.org/pipermail/elfutils-devel/2022q1/004782.html
https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=fuzz

Cheers,

Mark

Re: Some fuzzer workarounds

[Evgeny Vereshchagin]

Hi Mark,

>> I can also prevent OSS-Fuzz from reporting new bugs found by MSan
>> by setting the experimental flag
>> 
>> From https://google.github.io/oss-fuzz/getting-started/new-project-guide/#sanitizers
>>> If you want to test a particular sanitizer to see what crashes it generates
>>> without filing them in the issue tracker, you can set an experimental flag
>> 
>> It should help to figure out whether it makes sense to keep it without spamming the mailing list
>> in the process. What do you think?
> 
> I think that is a good idea. I really believe all the issues reported
> by MSAN are bogus.

They are but all those issues should be gone once https://github.com/google/oss-fuzz/pull/7422 and
https://github.com/google/oss-fuzz/pull/7401 are merged. I ran the fuzzers with
those patches applied for a few hours and MSan didn't complain. I'll flip the
flag there a bit later today anyway and maybe bring it back in a month or so if it isn't noisy.

> 
> While the UBSAN and ASAN issues seem reasonable. At least I have a fix
> for the last one (45952 Misaligned-address in elf_cvt_gnuhash):
> https://sourceware.org/pipermail/elfutils-devel/2022q1/004782.html
> https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=fuzz

I rebased it on top my "fuzzing" branch and the fuzzers, static analyzers, the unit tests
on various architectures and so on confirmed that the issue is gone. Thanks!

Thanks,
Evgeny Vereshchagin

Re: Some fuzzer workarounds

[Mark Wielaard]

Hi Evgeny,

On Wed, Mar 23, 2022 at 04:15:42AM +0300, Evgeny Vereshchagin wrote:
> > I think that is a good idea. I really believe all the issues reported
> > by MSAN are bogus.
> 
> They are but all those issues should be gone once
> https://github.com/google/oss-fuzz/pull/7422 and
> https://github.com/google/oss-fuzz/pull/7401 are merged. I ran the
> fuzzers with those patches applied for a few hours and MSan didn't
> complain. I'll flip the flag there a bit later today anyway and
> maybe bring it back in a month or so if it isn't noisy.

That makes sense. You do indeed have to "rebuild the world" for MSAN
to work. I am slightly surprised it doesn't work with
-D_FORTIFY_SOURCE (which we indeed try to enable by default).

> > While the UBSAN and ASAN issues seem reasonable. At least I have a fix
> > for the last one (45952 Misaligned-address in elf_cvt_gnuhash):
> > https://sourceware.org/pipermail/elfutils-devel/2022q1/004782.html
> > https://code.wildebeest.org/git/user/mjw/elfutils/commit/?h=fuzz
> 
> I rebased it on top my "fuzzing" branch and the fuzzers, static analyzers, the unit tests
> on various architectures and so on confirmed that the issue is gone. Thanks!

Thanks for testing. Pushed.

Cheers,

Mark

Re: Some fuzzer workarounds

[Mark Wielaard]

Hi,

On Thu, Mar 17, 2022 at 02:30:49PM +0100, Mark Wielaard wrote:
> The following fixes should fix reading of some broken ar archives and
> misaligned access of the section zero Shdr for mmaped ELF files where
> the start of the Elf image is at some offset from the start of the
> map.
> 
> [PATCH 1/2] libelf: Take map offset into account for Shdr alignment
> [PATCH 2/2] libelf: Make sure ar_size starts with a digit before
> 
> https://code.wildebeest.org/git/user/mjw/elfutils/log/?h=fuzz
> 
> I haven't been able to replicate any other issues locally.

So I did eventually replicate some other issues and ran afl-fuzz
locally over the new fuzz targets during the weekend to look for any
others.

I pushed the above two patches, plus the other fixes I posted:

      libelf: Check alignment of Verdef, Verdaux, Verneed and Vernaux offsets
      libdwfl: Close ar members when they cannot be processed.
      libdwfl: Use memcpy to assign image header field values
      libelf: Don't overflow offsets in elf_cvt_Verneed and elf_cvt_Verdef

That should hopefully shutup the monorail reports. Except for those
using MSAN, which look bogus to me.

Cheers,

Mark