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

[Mark Wielaard <mjw@redhat.com>]

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

On Fri, Nov 07, 2014 at 04:32:49PM +0100, Hanno Böck wrote:
> Also see attachmend, output from american fuzzy lop with latest git
> code and your two patches. 9 crashes, 10 hangs.

Thanks. One of those pointed out that my overflow check for hash section
sizes was bogus. Fixed version attached. The others seem to be because
handle_versym didn't initialize its vernames and filenames. Then when
an ELF file didn't set them we did check they were not set (NULL), but
that check failed, because the elements still contained random data.
The second second patch fixes that.

I have pushed all three fuzz-robustify patches to master.

Note that the testcases you say are hanging are just really, realy slow.
Because of very large input values they try to process a lot of elements,
but eventually they will finish. We still might to sanity check some of
those excessively large input values, but they don't lead to hangs or
crashes. Just very long runtimes.

Cheers,

Mark

[-- Attachment #2: 0001-readelf-Sanity-check-hash-section-contents-before-pr.patch --]
[-- Type: text/plain, Size: 4229 bytes --]

>From 6b246e0620bdbaf8240f3bf391ec773eea3f7f48 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Fri, 7 Nov 2014 12:54:02 +0100
Subject: [PATCH 1/2] readelf: Sanity check hash section contents before
 processing.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reported by: Hanno Böck <hanno@hboeck.de>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 src/ChangeLog |  6 ++++++
 src/readelf.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index a252cdc..3ff3e31 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,9 @@
+2014-11-07  Mark Wielaard  <mjw@redhat.com>
+
+	* readelf.c (handle_sysv_hash): Sanity check section contents.
+	(handle_sysv_hash64): Likewise.
+	(handle_gnu_hash): Likewise.
+
 2014-09-14  Petr Machata  <pmachata@redhat.com>
 
 	* readelf.c (handle_relocs_rela): Typo fix, test DESTSHDR properly.
diff --git a/src/readelf.c b/src/readelf.c
index 4d3bb36..e03a771 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -2954,8 +2954,21 @@ handle_sysv_hash (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr, size_t shstrndx)
       return;
     }
 
+  if (unlikely (data->d_size < 2 * sizeof (Elf32_Word)))
+    {
+    invalid_data:
+      error (0, 0, gettext ("invalid data in sysv.hash section %d"),
+	     (int) elf_ndxscn (scn));
+      return;
+    }
+
   Elf32_Word nbucket = ((Elf32_Word *) data->d_buf)[0];
   Elf32_Word nchain = ((Elf32_Word *) data->d_buf)[1];
+
+  uint64_t used_buf = (2ULL + nchain + nbucket) * sizeof (Elf32_Word);
+  if (used_buf > data->d_size)
+    goto invalid_data;
+
   Elf32_Word *bucket = &((Elf32_Word *) data->d_buf)[2];
   Elf32_Word *chain = &((Elf32_Word *) data->d_buf)[2 + nbucket];
 
@@ -2996,8 +3009,21 @@ handle_sysv_hash64 (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr, size_t shstrndx)
       return;
     }
 
+  if (unlikely (data->d_size < 2 * sizeof (Elf64_Xword)))
+    {
+    invalid_data:
+      error (0, 0, gettext ("invalid data in sysv.hash64 section %d"),
+	     (int) elf_ndxscn (scn));
+      return;
+    }
+
   Elf64_Xword nbucket = ((Elf64_Xword *) data->d_buf)[0];
   Elf64_Xword nchain = ((Elf64_Xword *) data->d_buf)[1];
+
+  uint64_t used_buf = (2ULL + nchain + nbucket) * sizeof (Elf64_Xword);
+  if (used_buf > data->d_size)
+    goto invalid_data;
+
   Elf64_Xword *bucket = &((Elf64_Xword *) data->d_buf)[2];
   Elf64_Xword *chain = &((Elf64_Xword *) data->d_buf)[2 + nbucket];
 
@@ -3037,18 +3063,37 @@ handle_gnu_hash (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr, size_t shstrndx)
       return;
     }
 
+  if (unlikely (data->d_size < 4 * sizeof (Elf32_Word)))
+    {
+    invalid_data:
+      error (0, 0, gettext ("invalid data in gnu.hash section %d"),
+	     (int) elf_ndxscn (scn));
+      return;
+    }
+
   Elf32_Word nbucket = ((Elf32_Word *) data->d_buf)[0];
   Elf32_Word symbias = ((Elf32_Word *) data->d_buf)[1];
 
   /* Next comes the size of the bitmap.  It's measured in words for
      the architecture.  It's 32 bits for 32 bit archs, and 64 bits for
-     64 bit archs.  */
+     64 bit archs.  There is always a bloom filter present, so zero is
+     an invalid value.  */
   Elf32_Word bitmask_words = ((Elf32_Word *) data->d_buf)[2];
   if (gelf_getclass (ebl->elf) == ELFCLASS64)
     bitmask_words *= 2;
 
+  if (bitmask_words == 0)
+    goto invalid_data;
+
   Elf32_Word shift = ((Elf32_Word *) data->d_buf)[3];
 
+  /* Is there still room for the sym chain?
+     Use uint64_t calculation to prevent 32bit overlow.  */
+  uint64_t used_buf = (4ULL + bitmask_words + nbucket) * sizeof (Elf32_Word);
+  uint32_t max_nsyms = (data->d_size - used_buf) / sizeof (Elf32_Word);
+  if (used_buf > data->d_size)
+    goto invalid_data;
+
   uint32_t *lengths = (uint32_t *) xcalloc (nbucket, sizeof (uint32_t));
 
   Elf32_Word *bitmask = &((Elf32_Word *) data->d_buf)[4];
@@ -3068,6 +3113,8 @@ handle_gnu_hash (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr, size_t shstrndx)
 	    ++nsyms;
 	    if (maxlength < ++lengths[cnt])
 	      ++maxlength;
+	    if (inner > max_nsyms)
+	      goto invalid_data;
 	  }
 	while ((chain[inner++] & 1) == 0);
       }
-- 
1.9.3


[-- Attachment #3: 0002-readelf.c-handle_versym-Initialize-vername-and-filen.patch --]
[-- Type: text/plain, Size: 1614 bytes --]

>From d8b9682b1a5ff2746f172487eaf19ebd088bb7f4 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Sat, 8 Nov 2014 14:04:27 +0100
Subject: [PATCH 2/2] readelf.c (handle_versym): Initialize vername and
 filename array elements.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We check whether the elements are set before printing their contents,
but didn't make sure they were initialized.

Reported-by: Hanno Böck <hanno@hboeck.de>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 src/ChangeLog | 5 +++++
 src/readelf.c | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/src/ChangeLog b/src/ChangeLog
index 3ff3e31..6d3e951 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2014-11-08  Mark Wielaard  <mjw@redhat.com>
+
+	* readelf.c (handle_versym): Initialize vername and filename array
+	elements.
+
 2014-11-07  Mark Wielaard  <mjw@redhat.com>
 
 	* readelf.c (handle_sysv_hash): Sanity check section contents.
diff --git a/src/readelf.c b/src/readelf.c
index e03a771..01c644f 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -2716,7 +2716,9 @@ handle_versym (Ebl *ebl, Elf_Scn *scn, GElf_Shdr *shdr)
 
       /* Allocate the array.  */
       vername = (const char **) alloca (nvername * sizeof (const char *));
+      memset(vername, 0, nvername * sizeof (const char *));
       filename = (const char **) alloca (nvername * sizeof (const char *));
+      memset(filename, 0, nvername * sizeof (const char *));
 
       /* Run through the data structures again and collect the strings.  */
       if (defscn != NULL)
-- 
1.9.3