The library function nlist(3) does not properly validate parsed ELF
binary files, which can lead to out of boundary accesses.
Also, nlist will return -1 for stripped binary files, because eventually
it will try to mmap 0 bytes. Instead of returning the amount of symbols
we tried to look up, -1 suggests that the file was not there or invalid.
I moved that check a bit up so it behaves as expected.
What my diff does to prevent out of boundary accesses:
- Prevent mmap to map memory from outside file boundaries. A broken
file could contain invalid values for ehdr.e_shoff and shdr_size.
Please note that shdr_size calculation is safe ((2^16 - 1)^2 < 2^32).
- Verify that shdr[i].sh_link contains a valid index.
- While iterating through symbols, make sure that symsize actually
references enough memory for an Elf_Sym. Imagine that only 1 byte is
left in file. Current code would read that 1 byte into an Elf_Sym
struct and processes its content. That's definitely undefined
behaviour.
- Verify that Elf symbol references a valid string offset.
- When comparing symbol names, make sure that strcmp() won't access
memory outside the allocated space. A symbol name must \0 terminate,
and that byte must be in the ELF file.
Index: lib/libc/gen/nlist.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/nlist.c,v
retrieving revision 1.59
diff -u -p -r1.59 nlist.c
--- lib/libc/gen/nlist.c 6 Feb 2015 23:21:58 -0000 1.59
+++ lib/libc/gen/nlist.c 15 Oct 2015 09:03:07 -0000
@@ -101,6 +101,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) ||
@@ -111,7 +112,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);
}
@@ -138,6 +140,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;
@@ -152,12 +156,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
@@ -177,41 +206,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;
@@ -221,13 +229,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;
