On Sat, Jun 27, 2015 at 12:45:13PM +0200, Hanno Böck wrote: > One reason you might want to fix such issues is that they could be used > to cause memory exhaustion. E.g. you have a server that processes files > and you send them specially crafted small files that will use up a lot > of memory, but not that much that malloc failes. > > Therefore imho it makes sense to add some sanity checks. Parsers should > never accept any field sizes that are larger than the file itself. > > This is probably not so much an issue in self-containing tools like > elfutils. Honestly the biggest reason I report these is that asan > complains about them and it makes fuzzing easier if they get fixed. But > it's up to you. (Most other apps where I reported similar things fixed > them)
The fix is indeed simple. We just have to switch the getting of data (and detecing it is bogus) before allocating the memory. With that your example gives: src/nm: bogus.elf: entry size in section 2 `(null)' is not what we expect src/nm: bogus.elf: INTERNAL ERROR 1207 (0.163): invalid data And then exists before trying to allocate any memory. Attached patch pushed to master. Hope that helps. Looking forward to more fuzzing results :) Thanks, Mark
>From c08079a076420f67742be98d060500965eb22340 Mon Sep 17 00:00:00 2001 From: Mark Wielaard <[email protected]> Date: Sat, 27 Jun 2015 22:07:01 +0200 Subject: [PATCH] nm: First call elf_getdata, then allocate memory. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This catches bogus data early before we might try to allocate giant amounts of memory. Reported-by: Hanno Böck <[email protected]> Signed-off-by: Mark Wielaard <[email protected]> --- src/ChangeLog | 4 ++++ src/nm.c | 12 ++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 7d5e001..50223a4 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2015-06-27 Mark Wielaard <[email protected]> + + * nm.c (show_symbols): First call elf_getdata, then allocate memory. + 2015-06-18 Mark Wielaard <[email protected]> * findtextrel.c (process_file): Free segments after use. diff --git a/src/nm.c b/src/nm.c index 7339506..15d9da4 100644 --- a/src/nm.c +++ b/src/nm.c @@ -1200,6 +1200,12 @@ show_symbols (Ebl *ebl, GElf_Ehdr *ehdr, Elf_Scn *scn, Elf_Scn *xndxscn, } } + /* Get the data of the section. */ + Elf_Data *data = elf_getdata (scn, NULL); + Elf_Data *xndxdata = elf_getdata (xndxscn, NULL); + if (data == NULL || (xndxscn != NULL && xndxdata == NULL)) + INTERNAL_ERROR (fullname); + /* Allocate the memory. XXX We can use a dirty trick here. Since GElf_Sym == Elf64_Sym we @@ -1211,12 +1217,6 @@ show_symbols (Ebl *ebl, GElf_Ehdr *ehdr, Elf_Scn *scn, Elf_Scn *xndxscn, else sym_mem = (GElf_SymX *) xmalloc (nentries * sizeof (GElf_SymX)); - /* Get the data of the section. */ - Elf_Data *data = elf_getdata (scn, NULL); - Elf_Data *xndxdata = elf_getdata (xndxscn, NULL); - if (data == NULL || (xndxscn != NULL && xndxdata == NULL)) - INTERNAL_ERROR (fullname); - /* Iterate over all symbols. */ #ifdef USE_DEMANGLE size_t demangle_buffer_len = 0; -- 2.4.3
