Re: Make gelf_getphdr more robust?

[Mark Wielaard <mjw at redhat dot com>]

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 <mjw@redhat.com>
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 <fweimer@redhat.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 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  <mjw@redhat.com>
+
+	* readelf.c (print_phdr): Check phdr->p_filesz and make sure
+	interpreter string is zero terminated before calling printf.
+
 2014-01-22  Mark Wielaard  <mjw@redhat.com>
 
 	* 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