Re: Make gelf_getphdr more robust?

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2014-02-05 at 18:52 +0100, Florian Weimer wrote:
> readelf does this:
> 
>        GElf_Phdr *phdr = gelf_getphdr (ebl->elf, cnt, &mem);
> …
>        if (phdr->p_type == PT_INTERP)
> 	{
> 	  /* We can show the user the name of the interpreter.  */
> 	  size_t maxsize;
> 	  char *filedata = elf_rawfile (ebl->elf, &maxsize);
> 
> 	  if (filedata != NULL && phdr->p_offset < maxsize)
> 	    printf (gettext ("\t[Requesting program interpreter: %s]\n"),
> 		    filedata + phdr->p_offset);
> 	}
> 
> 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

+ /* First see whether the information in the ELF header is
+ valid and it does not ask for too much. */
+ if (unlikely (ehdr->e_phoff >= elf->maximum_size)
+     || unlikely (elf->maximum_size - ehdr->e_phoff < size))
+ {
+   /* Something is wrong. */
+   __libelf_seterrno (ELF_E_INVALID_PHDR);
+   goto out;
+ }

So that would make the maxsize check in readelf.c redundant.

> And the printf call can leak data or crash if filedata + phdr->p_offset 
> is not NUL terminated (which obviously needs a crafted ELF file).  This 
> can't be fixed in gelf_getphdr, alas.

Yes, it would probably be good to catch this in eu-readelf.

Did you catch this by code inspection or did you stumble upon a bad ELF
file that made it crash?

Thanks,

Mark