From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============8995579268150437672=="
MIME-Version: 1.0
From: Mark Wielaard <mjw@redhat.com>
To: elfutils-devel@lists.fedorahosted.org
Subject: Re: out-of-bounds read / crash in elfutils tools (readelf, nm,
 ...) with malformed file
Date: Thu, 06 Nov 2014 17:05:42 +0100
Message-ID: <1415289942.19702.22.camel@bordewijk.wildebeest.org>
In-Reply-To: 1415286703.19702.20.camel@bordewijk.wildebeest.org

--===============8995579268150437672==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

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 !=3D 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 =3D
                &elf->state.elf32.shdr[cnt];
              if (likely (elf->state.elf32.shdr[cnt].sh_offset < maxsize)
-                 && likely (maxsize - elf->state.elf32.shdr[cnt].sh_offset
-                            <=3D elf->state.elf32.shdr[cnt].sh_size))
+                 && likely (elf->state.elf32.shdr[cnt].sh_size
+                            <=3D maxsize - elf->state.elf32.shdr[cnt].sh_o=
ffset))
                elf->state.elf32.scns.data[cnt].rawdata_base =3D
                  elf->state.elf32.scns.data[cnt].data_base =3D
                  ((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 =3D
                &elf->state.elf64.shdr[cnt];
              if (likely (elf->state.elf64.shdr[cnt].sh_offset < maxsize)
-                 && likely (maxsize - elf->state.elf64.shdr[cnt].sh_offset
-                            <=3D elf->state.elf64.shdr[cnt].sh_size))
+                 && likely (elf->state.elf64.shdr[cnt].sh_size
+                            <=3D maxsize - elf->state.elf64.shdr[cnt].sh_o=
ffset))
                elf->state.elf64.scns.data[cnt].rawdata_base =3D
                  elf->state.elf64.scns.data[cnt].data_base =3D
                  ((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

--===============8995579268150437672==--