Hi Sayed, On Sun, Jun 28, 2026 at 08:28:32PM +0530, Sayed Kaif wrote: > elf_getarsym parses the archive symbol table (the "/" or "/SYM64/" > member). After validating only the size of the index, it walks the > declared number of symbols and, for each one, computes a hash with > _dl_elf_hash and advances to the next name with rawmemchr (or an > open-coded loop on platforms without rawmemchr). Both of these scan > for a terminating NUL byte without any bound. > > Nothing guarantees that the string area following the offset table > actually contains that many NUL-terminated names, or any NUL at all. > A crafted archive whose symbol-table string area is not NUL terminated > makes the name scan run past the end of the heap buffer holding the > string data (or past the mapped file) -- an out-of-bounds read.
Yeah, usage of rawmemchr usually is a red flag unless there was some preliminary scan over the memory area to make sure the needed (NUL) character is already there. > Remember the end of the string area and use a bounded memchr to locate > each name terminator. If a name is not terminated within the string > area the archive is malformed, so fail with ELF_E_INVALID_ARCHIVE > instead of reading out of bounds. I was hoping we could search backwards because normally there always is a zero at the end, so that would be more efficient. But you have to scan the string area for each symbol because the strings are in order. So using memchr to "advance" to the next name is correct. > Reproduced under AddressSanitizer with a crafted archive whose string > area contains no NUL byte: > > ==ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 1 > #0 _dl_elf_hash dl-hash.h:45 > #1 elf_getarsym elf_getarsym.c:294 > 0 bytes after 116-byte region ... allocated by elf_getarsym.c:215 > > The function is reachable from eu-nm -s and eu-readelf, which call > elf_getarsym on untrusted .a files. Yeah, and elf_getarsym is a public libelf function, so anybody using it will want this sanity check. > Signed-off-by: Sayed Kaif <[email protected]> > --- > diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c > index 281f0c1..ec05c68 100644 > --- a/libelf/elf_getarsym.c > +++ b/libelf/elf_getarsym.c > @@ -199,6 +199,7 @@ elf_getarsym (Elf *elf, size_t *ptr) > { > void *file_data; /* unit32_t[n] or uint64_t[n] */ > char *str_data; > + char *str_end; > size_t sz = n * w; > > if (elf->map_address == NULL) OK. Define str_end. > @@ -238,6 +239,7 @@ elf_getarsym (Elf *elf, size_t *ptr) > } > > str_data = (char *) new_str; > + str_end = new_str + (index_size - sz); > } > else > { OK. pread case. > @@ -254,6 +256,7 @@ elf_getarsym (Elf *elf, size_t *ptr) > file_data = memcpy (temp_data, elf->map_address + off, sz); > } > str_data = (char *) (elf->map_address + off + sz); > + str_end = (char *) (elf->map_address + off + index_size - w); > } > > /* Now we can build the data structure. */ OK. mmap case. > @@ -291,16 +294,25 @@ elf_getarsym (Elf *elf, size_t *ptr) > else > arsym[cnt].as_off = (*u32)[cnt]; > > + /* The symbol name must be NUL terminated within the string > + table. Otherwise the archive symbol table is corrupt and > + hashing or scanning the name would read out of bounds. */ > + char *endp = (str_data < str_end > + ? memchr (str_data, '\0', str_end - str_data) > + : NULL); > + if (unlikely (endp == NULL)) > + { > + if (elf->map_address == NULL) > + { > + free (elf->state.ar.ar_sym); > + elf->state.ar.ar_sym = NULL; > + } > + __libelf_seterrno (ELF_E_INVALID_ARCHIVE); > + goto out; > + } > + OK. Extra check for str_data == str_end (which also would be bad). Correct failure cleanup. > arsym[cnt].as_hash = _dl_elf_hash (str_data); > -#if HAVE_DECL_RAWMEMCHR > - str_data = rawmemchr (str_data, '\0') + 1; > -#else > - char c; > - do { > - c = *str_data; > - str_data++; > - } while (c); > -#endif > + str_data = endp + 1; > } OK. remove rawmemchr no longer needed. And set str_data to next string start. Will push this. Thanks. Also this looks like the last usage of rawmemchr in the code base. So I'll remove the configure check in a followup commit. Thanks, Mark
