Validate parsed ELF values to prevent out of boundary accesses.

While at it, return proper return value when encountering a stripped
binary. Instead of -1 (illegal file), it should be the amount of symbols
that were tried to be resolved.

ok millert

diff --git a/lib/libc/gen/nlist.c b/lib/libc/gen/nlist.c
index 100fa94..f966626 100644 (file)
--- a/lib/libc/gen/nlist.c
+++ b/lib/libc/gen/nlist.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: nlist.c,v 1.62 2015/10/04 06:59:57 guenther Exp $ */
+/*     $OpenBSD: nlist.c,v 1.63 2015/10/16 07:40:12 tobias Exp $ */
 /*
  * Copyright (c) 1989, 1993
  *     The Regents of the University of California.  All rights reserved.
@@ -95,6 +95,7 @@ __fdnlist(int fd, struct nlist *list)
        Elf_Word shdr_size;
        struct stat st;
        int usemalloc = 0;
+       size_t left, len;
 
        /* Make sure obj is OK */
        if (pread(fd, &ehdr, sizeof(Elf_Ehdr), (off_t)0) != sizeof(Elf_Ehdr) ||
@@ -105,7 +106,8 @@ __fdnlist(int fd, struct nlist *list)
        shdr_size = ehdr.e_shentsize * ehdr.e_shnum;
 
        /* Make sure it's not too big to mmap */
-       if (shdr_size > SIZE_MAX) {
+       if (SIZE_MAX - ehdr.e_shoff < shdr_size ||
+           ehdr.e_shoff + shdr_size > st.st_size) {
                errno = EFBIG;
                return (-1);
        }
@@ -132,6 +134,8 @@ __fdnlist(int fd, struct nlist *list)
         */
        for (i = 0; i < ehdr.e_shnum; i++) {
                if (shdr[i].sh_type == SHT_SYMTAB) {
+                       if (shdr[i].sh_link >= ehdr.e_shnum)
+                               continue;
                        symoff = shdr[i].sh_offset;
                        symsize = shdr[i].sh_size;
                        symstroff = shdr[shdr[i].sh_link].sh_offset;
@@ -146,12 +150,37 @@ __fdnlist(int fd, struct nlist *list)
        else
                munmap((caddr_t)shdr, shdr_size);
 
+       /*
+        * clean out any left-over information for all valid entries.
+        * Type and value defined to be 0 if not found; historical
+        * versions cleared other and desc as well.  Also figure out
+        * the largest string length so don't read any more of the
+        * string table than we have to.
+        *
+        * XXX clearing anything other than n_type and n_value violates
+        * the semantics given in the man page.
+        */
+       nent = 0;
+       for (p = list; !ISLAST(p); ++p) {
+               p->n_type = 0;
+               p->n_other = 0;
+               p->n_desc = 0;
+               p->n_value = 0;
+               ++nent;
+       }
+
+       /* Don't process any further if object is stripped. */
+       /* ELFism - dunno if stripped by looking at header */
+       if (symoff == 0)
+               return nent;
+
        /* Check for files too large to mmap. */
-       /* XXX is this really possible? */
-       if (symstrsize > SIZE_MAX) {
+       if (SIZE_MAX - symstrsize < symstroff ||
+           symstrsize + symstroff > st.st_size) {
                errno = EFBIG;
                return (-1);
        }
+
        /*
         * Map string table into our address space.  This gives us
         * an easy way to randomly access all the strings, without
@@ -171,41 +200,20 @@ __fdnlist(int fd, struct nlist *list)
                if (strtab == MAP_FAILED)
                        return (-1);
        }
-       /*
-        * clean out any left-over information for all valid entries.
-        * Type and value defined to be 0 if not found; historical
-        * versions cleared other and desc as well.  Also figure out
-        * the largest string length so don't read any more of the
-        * string table than we have to.
-        *
-        * XXX clearing anything other than n_type and n_value violates
-        * the semantics given in the man page.
-        */
-       nent = 0;
-       for (p = list; !ISLAST(p); ++p) {
-               p->n_type = 0;
-               p->n_other = 0;
-               p->n_desc = 0;
-               p->n_value = 0;
-               ++nent;
-       }
 
-       /* Don't process any further if object is stripped. */
-       /* ELFism - dunno if stripped by looking at header */
-       if (symoff == 0)
-               goto elf_done;
-
-       while (symsize > 0) {
+       while (symsize >= sizeof(Elf_Sym)) {
                cc = MINIMUM(symsize, sizeof(sbuf));
                if (pread(fd, sbuf, cc, (off_t)symoff) != cc)
                        break;
                symsize -= cc;
                symoff += cc;
                for (s = sbuf; cc > 0; ++s, cc -= sizeof(*s)) {
-                       int soff = s->st_name;
+                       Elf_Word soff = s->st_name;
 
-                       if (soff == 0)
+                       if (soff == 0 || soff >= symstrsize)
                                continue;
+                       left = symstrsize - soff;
+
                        for (p = list; !ISLAST(p); p++) {
                                char *sym;
 
@@ -215,13 +223,16 @@ __fdnlist(int fd, struct nlist *list)
                                 * and the first char is an '_', skip over
                                 * the '_' and try again.
                                 * XXX - What do we do when the user really
-                                *       wants '_foo' and the are symbols
+                                *       wants '_foo' and there are symbols
                                 *       for both 'foo' and '_foo' in the
                                 *       table and 'foo' is first?
                                 */
                                sym = p->n_un.n_name;
-                               if (strcmp(&strtab[soff], sym) != 0 &&
-                                   (sym[0] != '_' ||
+                               len = strlen(sym);
+
+                               if ((len >= left ||
+                                   strcmp(&strtab[soff], sym) != 0) &&
+                                   (sym[0] != '_' || len - 1 >= left ||
                                     strcmp(&strtab[soff], sym + 1) != 0))
                                        continue;