[PATCH] libdwfl: Check relocations don't overlap ELF ehdr, shdrs or phdrs.

[PATCH] libdwfl: Check relocations don't overlap ELF ehdr, shdrs or phdrs.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 4308 bytes --]

American Fuzzy Lop (afl-fuzz) has an habit of generating ELF files
with relocations that when applied (or removed/cleared) change one
of the in-memory ELF headers. There does not seem to be a valid reason
for having section data that contain relocations or to which relocations
are applied to overlap with one of the headers.

If either the section that needs the relocation applied, or the
section that the relocations come from overlap one of the ehdrs,
shdrs or phdrs data then refuse to do the relocations.  We update
both section data. It isn't illegal for ELF section data to overlap
the header data, but updating the (relocation) data might corrupt
the in-memory libelf headers causing strange corruptions or errors.

Also check offset + size of a relocation doesn't overflow.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog  |  6 ++++++
 libdwfl/relocate.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 5876fcc..03faecf 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,9 @@
+2014-11-29  Mark Wielaard  <mjw@redhat.com>
+
+	* relocate.c (relocate_section): Check relocation section and target
+	section data don't overlap any of the ELF headers.
+	(relocate): Check for offset + size overflow.
+
 2014-11-22  Mark Wielaard  <mjw@redhat.com>
 
 	* link_map.c (consider_executable): Use elf_getphdrnum.
diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 52b7b5e..4fc0956 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -297,6 +297,51 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
   if (tdata == NULL)
     return DWFL_E_LIBELF;
 
+  /* If either the section that needs the relocation applied, or the
+     section that the relocations come from overlap one of the ehdrs,
+     shdrs or phdrs data then we refuse to do the relocations.  It
+     isn't illegal for ELF section data to overlap the header data,
+     but updating the (relocation) data might corrupt the in-memory
+     libelf headers causing strange corruptions or errors.  */
+  if (unlikely (shdr->sh_offset < ehdr->e_ehsize
+		|| tshdr->sh_offset < ehdr->e_ehsize))
+    return DWFL_E_BADELF;
+
+  GElf_Off shdr_start = ehdr->e_shoff;
+  size_t shnums;
+  if (elf_getshdrnum (relocated, &shnums) < 0)
+    return DWFL_E_LIBELF;
+  /* Overflows will have been checked by elf_getshdrnum/get|rawdata.  */
+  GElf_Off shdr_end = shdr_start + shnums * ehdr->e_shentsize;
+  if (unlikely ((shdr->sh_offset >= shdr_start
+		 && shdr->sh_offset < shdr_end)
+		|| (shdr->sh_offset + shdr->sh_size >= shdr_start
+		    && shdr->sh_offset + shdr->sh_size < shdr_end)
+		|| (tshdr->sh_offset >= shdr_start
+		    && tshdr->sh_offset < shdr_end)
+		|| (tshdr->sh_offset + tshdr->sh_size >= shdr_start
+		    && tshdr->sh_offset + tshdr->sh_size < shdr_end)))
+    return DWFL_E_BADELF;
+
+  GElf_Off phdr_start = ehdr->e_phoff;
+  size_t phnums;
+  if (elf_getphdrnum (relocated, &phnums) < 0)
+    return DWFL_E_LIBELF;
+  if (phdr_start != 0 && phnums != 0)
+    {
+      /* Overflows will have been checked by elf_getphdrnum/get|rawdata.  */
+      GElf_Off phdr_end = phdr_start + phnums * ehdr->e_phentsize;
+      if (unlikely ((shdr->sh_offset >= phdr_start
+		     && shdr->sh_offset < phdr_end)
+		    || (shdr->sh_offset + shdr->sh_size >= phdr_start
+			&& shdr->sh_offset + shdr->sh_size < phdr_end)
+		    || (tshdr->sh_offset >= phdr_start
+			&& tshdr->sh_offset < phdr_end)
+		    || (tshdr->sh_offset + tshdr->sh_size >= phdr_start
+			&& tshdr->sh_offset + tshdr->sh_size < phdr_end)))
+	return DWFL_E_BADELF;
+    }
+
   /* Apply one relocation.  Returns true for any invalid data.  */
   Dwfl_Error relocate (GElf_Addr offset, const GElf_Sxword *addend,
 		       int rtype, int symndx)
@@ -365,7 +410,7 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
 	return DWFL_E_BADRELTYPE;
       }
 
-    if (offset + size > tdata->d_size)
+    if (offset > tdata->d_size || tdata->d_size - offset < size)
       return DWFL_E_BADRELOFF;
 
 #define DO_TYPE(NAME, Name) GElf_##Name Name;
-- 
1.9.3

Re: [PATCH] libdwfl: Check relocations don't overlap ELF ehdr, shdrs or phdrs.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Sun, 2014-11-30 at 21:02 +0100, Mark Wielaard wrote:
> If either the section that needs the relocation applied, or the
> section that the relocations come from overlap one of the ehdrs, shdrs
> or phdrs data then refuse to do the relocations. We update both
> section data. It isn't illegal for ELF section data to overlap the
> header data, but updating the (relocation) data might corrupt the
> in-memory libelf headers causing strange corruptions or errors. Also
> check offset + size of a relocation doesn't overflow.

I pushed this to master now. This really fixed a lot of mysterious
crashers.

Cheers,

Mark

Re: [PATCH] libdwfl: Check relocations don't overlap ELF ehdr, shdrs or phdrs.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Sat, Nov 29, 2014 at 08:40:58PM +0100, Mark Wielaard wrote:
> American Fuzzy Lop (afl-fuzz) has an habit of generating ELF files
> with relocations that when applied (or removed/cleared) change one
> of the in-memory ELF headers. There does not seem to be a valid reason
> for having section data that contain relocations or to which relocations
> are applied to overlap with one of the headers.
> [...]
> +  GElf_Off shdr_start = ehdr->e_shoff;
> +  size_t shnums;
> +  if (elf_getshdrnum (relocated, &shnums) < 0)
> +    return DWFL_E_LIBELF;
> +  /* Overflows will have been checked by elf_getshdrnum/get|rawdata.  */
> +  GElf_Off shdr_end = shdr_start + shnums * ehdr->e_shentsize;
> +  if (unlikely ((shdr->sh_offset >= shdr_start
> +		 && shdr->sh_offset < shdr_end)
> +		|| (shdr->sh_offset + shdr->sh_size >= shdr_start
> +		    && shdr->sh_offset + shdr->sh_size < shdr_end)
> +		|| (tshdr->sh_offset >= shdr_start
> +		    && tshdr->sh_offset < shdr_end)
> +		|| (tshdr->sh_offset + tshdr->sh_size >= shdr_start
> +		    && tshdr->sh_offset + tshdr->sh_size < shdr_end)))
> +    return DWFL_E_BADELF;

Some testing revealed this test is too complicated and wrong.  It missed
the header being completely inside the section. Fixed version attached.

Cheers,

Mark

[-- Attachment #2: 0001-libdwfl-Check-relocations-don-t-overlap-ELF-ehdr-shd.patch --]
[-- Type: text/plain, Size: 3742 bytes --]

>From c3cfdb056fe8ff2f29924c8d6c94e727a372fa51 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Sat, 29 Nov 2014 20:23:30 +0100
Subject: [PATCH] libdwfl: Check relocations don't overlap ELF ehdr, shdrs or
 phdrs.

If either the section that needs the relocation applied, or the
section that the relocations come from overlap one of the ehdrs,
shdrs or phdrs data then refuse to do the relocations.  We update
both section data. It isn't illegal for ELF section data to overlap
the header data, but updating the (relocation) data might corrupt
the in-memory libelf headers causing strange corruptions or errors.

Also check offset + size of a relocation doesn't overflow.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog  |  6 ++++++
 libdwfl/relocate.c | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 5876fcc..03faecf 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,9 @@
+2014-11-29  Mark Wielaard  <mjw@redhat.com>
+
+	* relocate.c (relocate_section): Check relocation section and target
+	section data don't overlap any of the ELF headers.
+	(relocate): Check for offset + size overflow.
+
 2014-11-22  Mark Wielaard  <mjw@redhat.com>
 
 	* link_map.c (consider_executable): Use elf_getphdrnum.
diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 52b7b5e..6862189 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -297,6 +297,43 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
   if (tdata == NULL)
     return DWFL_E_LIBELF;
 
+  /* If either the section that needs the relocation applied, or the
+     section that the relocations come from overlap one of the ehdrs,
+     shdrs or phdrs data then we refuse to do the relocations.  It
+     isn't illegal for ELF section data to overlap the header data,
+     but updating the (relocation) data might corrupt the in-memory
+     libelf headers causing strange corruptions or errors.  */
+  if (unlikely (shdr->sh_offset < ehdr->e_ehsize
+		|| tshdr->sh_offset < ehdr->e_ehsize))
+    return DWFL_E_BADELF;
+
+  GElf_Off shdrs_start = ehdr->e_shoff;
+  size_t shnums;
+  if (elf_getshdrnum (relocated, &shnums) < 0)
+    return DWFL_E_LIBELF;
+  /* Overflows will have been checked by elf_getshdrnum/get|rawdata.  */
+  GElf_Off shdrs_end = shdrs_start + shnums * ehdr->e_shentsize;
+  if (unlikely ((shdrs_start < shdr->sh_offset + shdr->sh_size
+		 && shdr->sh_offset < shdrs_end)
+		|| (shdrs_start < tshdr->sh_offset + tshdr->sh_size
+		    && tshdr->sh_offset < shdrs_end)))
+    return DWFL_E_BADELF;
+
+  GElf_Off phdrs_start = ehdr->e_phoff;
+  size_t phnums;
+  if (elf_getphdrnum (relocated, &phnums) < 0)
+    return DWFL_E_LIBELF;
+  if (phdrs_start != 0 && phnums != 0)
+    {
+      /* Overflows will have been checked by elf_getphdrnum/get|rawdata.  */
+      GElf_Off phdrs_end = phdrs_start + phnums * ehdr->e_phentsize;
+      if (unlikely ((phdrs_start < shdr->sh_offset + shdr->sh_size
+		     && shdr->sh_offset < phdrs_end)
+		    || (phdrs_start < tshdr->sh_offset + tshdr->sh_size
+			&& tshdr->sh_offset < phdrs_end)))
+	return DWFL_E_BADELF;
+    }
+
   /* Apply one relocation.  Returns true for any invalid data.  */
   Dwfl_Error relocate (GElf_Addr offset, const GElf_Sxword *addend,
 		       int rtype, int symndx)
@@ -365,7 +402,7 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
 	return DWFL_E_BADRELTYPE;
       }
 
-    if (offset + size > tdata->d_size)
+    if (offset > tdata->d_size || tdata->d_size - offset < size)
       return DWFL_E_BADRELOFF;
 
 #define DO_TYPE(NAME, Name) GElf_##Name Name;
-- 
1.9.3