Fuzzing fixes #251
Merged
Fuzzing fixes #251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parsing this file results in patchelf triggering an assertion: patchelf: patchelf.cc:431: ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, Elf64_Verneed, unsigned short>::ElfFile(FileContents) [Elf_Ehdr = Elf64_Ehdr, Elf_Phdr = Elf64_Phdr, Elf_Shdr = Elf64_Shdr, Elf_Addr = unsigned long, Elf_Off = unsigned long, Elf_Dyn = Elf64_Dyn, Elf_Sym = Elf64_Sym, Elf_Verneed = Elf64_Verneed, Elf_Versym = unsigned short]: Assertion `shstrtabIndex < shdrs.size()' failed. Program received signal SIGABRT, Aborted.
Parsing this file results in patchelf triggering an assertion: patchelf: patchelf.cc:384: void checkPointer(const FileContents &, void *, unsigned int): Assertion `q >= contents->data() && q + size <= contents->data() + contents->size()' failed. Aborted (core dumped)
Parsing this file results in patchelf triggering an assertion: patchelf: patchelf.cc:439: ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, Elf64_Verneed, unsigned short>::ElfFile(FileContents) [Elf_Ehdr = Elf64_Ehdr, Elf_Phdr = Elf64_Phdr, Elf_Shdr = Elf64_Shdr, Elf_Addr = unsigned long, Elf_Off = unsigned long, Elf_Dyn = Elf64_Dyn, Elf_Sym = Elf64_Sym, Elf_Verneed = Elf64_Verneed, Elf_Versym = unsigned short]: Assertion `shstrtabSize > 0' failed. Aborted (core dumped)
Parsing this file results in patchelf triggering an assertion: patchelf: patchelf.cc:442: ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, unsigned long, unsigned long, Elf64_Dyn, Elf64_Sym, Elf64_Verneed, unsigned short>::ElfFile(FileContents) [Elf_Ehdr = Elf64_Ehdr, Elf_Phdr = Elf64_Phdr, Elf_Shdr = Elf64_Shdr, Elf_Addr = unsigned long, Elf_Off = unsigned long, Elf_Dyn = Elf64_Dyn, Elf_Sym = Elf64_Sym, Elf_Verneed = Elf64_Verneed, Elf_Versym = unsigned short]: Assertion `shstrtab[shstrtabSize - 1] == 0' failed. Aborted (core dumped)
Parsing this file results in patchelf segfaulting at:
at /nix/store/h31cy7jm6g7cfqbhc5pm4rf9c53i3qfb-gcc-9.3.0/include/c++/9.3.0/bits/char_traits.h:335
at /nix/store/h31cy7jm6g7cfqbhc5pm4rf9c53i3qfb-gcc-9.3.0/include/c++/9.3.0/bits/basic_string.h:527
this=0x7fffffff8028, fileContents=...) at patchelf.cc:449
... otherwise strlen() (in the std::string constructor) will be called with an out-of-bounds pointer.
Parsing this file results in patchelf segfaulting at:
Program received signal SIGSEGV, Segmentation fault.
std::vector<Elf32_Phdr, std::allocator<Elf32_Phdr> >::_M_realloc_insert<Elf32_Phdr const&> (this=0x7fffffff80a8, __position=..., __args=...)
at /nix/store/h31cy7jm6g7cfqbhc5pm4rf9c53i3qfb-gcc-9.3.0/include/c++/9.3.0/bits/vector.tcc:449
449 _Alloc_traits::construct(this->_M_impl,
(gdb) bt
at /nix/store/h31cy7jm6g7cfqbhc5pm4rf9c53i3qfb-gcc-9.3.0/include/c++/9.3.0/bits/vector.tcc:449
at /nix/store/h31cy7jm6g7cfqbhc5pm4rf9c53i3qfb-gcc-9.3.0/include/c++/9.3.0/bits/stl_vector.h:1195
this=0x7fffffff8088, fileContents=...) at patchelf.cc:421
sectionsByOldIndex was resized to hdr->e_shnum instead of rdi(hdr->e_shnum). This omitted the endianness conversion and probably only worked by accident, because the 16-bit LE->BE conversion results in integers that are larger as long as there no more than 255 section headers. This would be a good usecase for std::transform, but I'm unsure whether we want to bump requirements to build patchelf to C++ 2017.
This is similar to the earlier fix for phdr offsets, but the fuzzer didn't find this.
|
With this patch of fixes, AFL now runs for 3h without finding anything. This is a big step up from before. :) |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been running patchelf against the afl fuzzer, because we've seen
patchelfcrash in our production environment. This MR contains test cases and fixes for the first round of issues.I've been operating with this assumption:
patchelfshould never trigger assertions or crash for malformed ELF files.I've structured this MR in this way:
There are a few trivial fixes that come from code reading that currently have no corresponding test case, because the fuzzer couldn't construct one. If this is an issue, I can probably manually construct an offending file.
@tfc @domenkozar