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=720383c53b435de6647edd78060dd7d38ade25a5 > > [...] > > 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
>From 43c9c2d0d8422cb584e3c97df5edde5d7be53173 Mon Sep 17 00:00:00 2001 From: Mark Wielaard <[email protected]> Date: Fri, 7 Feb 2014 14:23:24 +0100 Subject: [PATCH] readelf: Robustify print_phdr program interpreter printing. Check phdr->p_filesz and make sure interpreter string is zero terminated before calling printf. Reported-by: Florian Weimer <[email protected]> Signed-off-by: Mark Wielaard <[email protected]> --- src/ChangeLog | 5 +++++ src/readelf.c | 5 ++++- 2 files changed, 9 insertions(+), 1 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 134ad90..ad3b2b1 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,8 @@ +2014-02-07 Mark Wielaard <[email protected]> + + * readelf.c (print_phdr): Check phdr->p_filesz and make sure + interpreter string is zero terminated before calling printf. + 2014-01-22 Mark Wielaard <[email protected]> * Makefile.am (nm_no_Wformat): Removed. diff --git a/src/readelf.c b/src/readelf.c index 5c5ad3d..fb95463 100644 --- a/src/readelf.c +++ b/src/readelf.c @@ -1191,7 +1191,10 @@ print_phdr (Ebl *ebl, GElf_Ehdr *ehdr) size_t maxsize; char *filedata = elf_rawfile (ebl->elf, &maxsize); - if (filedata != NULL && phdr->p_offset < maxsize) + if (filedata != NULL && phdr->p_offset < maxsize + && phdr->p_filesz <= maxsize - phdr->p_offset + && memchr (filedata + phdr->p_offset, '\0', + phdr->p_filesz) != NULL) printf (gettext ("\t[Requesting program interpreter: %s]\n"), filedata + phdr->p_offset); } -- 1.7.1
