Re: out-of-bounds read / crash in elfutils tools (readelf, nm, ...) with malformed file

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2014-11-06 at 16:11 +0100, Mark Wielaard wrote:
> I haven't figured out yet why we do use the data in the mmap case. It
> looks like we should catch that issue in elf_getdata:
> 
>       /* We can use the mapped or loaded data if available.  */
>       if (elf->map_address != NULL)
>         {
>           /* First see whether the information in the section header is
>              valid and it does not ask for too much.  */
>           if (unlikely (offset + size > elf->maximum_size))
>             {
>               /* Something is wrong.  */
>               __libelf_seterrno (ELF_E_INVALID_SECTION_HEADER);
>               return 1;
>             }
> 
> elf->maximum_size is setup correctly based on the actual file size. But
> it seems we don't actually hit this code path in this case. The rawdata
> is setup some other way than by calling __libelf_set_rawdata_wrlock. But
> I haven't figured out how yet.

I believe it comes from a reversed check in elf_begin when it sets up
the base addresses in case the file was mmaped.

diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index c3ad140..5525a3b 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -337,8 +337,8 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
              elf->state.elf32.scns.data[cnt].shdr.e32 =
                &elf->state.elf32.shdr[cnt];
              if (likely (elf->state.elf32.shdr[cnt].sh_offset < maxsize)
-                 && likely (maxsize - elf->state.elf32.shdr[cnt].sh_offset
-                            <= elf->state.elf32.shdr[cnt].sh_size))
+                 && likely (elf->state.elf32.shdr[cnt].sh_size
+                            <= maxsize - elf->state.elf32.shdr[cnt].sh_offset))
                elf->state.elf32.scns.data[cnt].rawdata_base =
                  elf->state.elf32.scns.data[cnt].data_base =
                  ((char *) map_address + offset
@@ -428,8 +428,8 @@ file_read_elf (int fildes, void *map_address, unsigned char *e_ident,
              elf->state.elf64.scns.data[cnt].shdr.e64 =
                &elf->state.elf64.shdr[cnt];
              if (likely (elf->state.elf64.shdr[cnt].sh_offset < maxsize)
-                 && likely (maxsize - elf->state.elf64.shdr[cnt].sh_offset
-                            <= elf->state.elf64.shdr[cnt].sh_size))
+                 && likely (elf->state.elf64.shdr[cnt].sh_size
+                            <= maxsize - elf->state.elf64.shdr[cnt].sh_offset))
                elf->state.elf64.scns.data[cnt].rawdata_base =
                  elf->state.elf64.scns.data[cnt].data_base =
                  ((char *) map_address + offset

With that the invalid sized sections aren't precached anymore and so
rawdata will remain NULL. __libelf_set_rawdata_wrlock will be called and
readelf will just report "cannot get data for section [28] '.strtab':
invalid section header" instead of trying to use the data.

Does the above look sane? All tests PASS with the above change.

Thanks,

Mark