On Thu, 2014-02-06 at 14:39 +0100, Florian Weimer wrote:
On 02/06/2014 02:20 PM, Mark Wielaard wrote:
>> The check against maxsize is insufficient, it's also required to check
>> that phdr->p_filesz <= maxsize - phdr->p_offset. Would it make sense
to
>> do both checks inside gelf_getphdr?
>
> I think this case is actually caught now by default by the integrated
> robustify patches from last month, specifically:
>
https://git.fedorahosted.org/cgit/elfutils.git/commit/?id=720383c53b435de...
> [...]
> So that would make the maxsize check in readelf.c redundant.
Looks like this. I was testing the version in Fedora 20,
elfutils-0.158-1.fc20.x86_64.
The fedora elfutils package already does contain the robustify patches.
And I was wrong. What that patch does is check when getting the phdr
against the ehdr whether the offset to the phdr itself is sane. But what
we want in eu-readelf is to check the phdr offset data itself is sane
before using what it points to. That cannot be pushed into getphdr. But
you are right that we should make the checks in readelf itself a bit
tighter.
> Did you catch this by code inspection or did you stumble upon a
bad ELF
> file that made it crash?
I noticed this issue in the source code when I ivestigated why my own
code complained about about a malformed interp header in a debuginfo
package. I have built a crafted ELF image derived from /usr/bin/gs that
crashes eu-readelf (running under valgrind), but it's nothing that I
found in the wild.
Could you test against current git with the attached patch? Or could you
give me access to your crafted ELF file?
Now that we integrated the robustify patches we really could use some
fuzzing on ELF and DWARF data to check our code is robust enough. I
dunno if any of the existing fuzzing tools are already good enough for
this or whether we should write something ourselves that knows something
about ELF and DWARF datastructures to create really mean things.
Thanks,
Mark