[PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

For a 64-bit ldconfig, running on a 32-bit library, if the p_vaddr field
of the segment containing the dynamic strings is less than it's
p_offset, then using ElfW(Off) for the arithmetic leads to a truncated
unsigned value for pointer arithmetic.

Instead, use intptr_t for loadoff, and cast the p_vaddr and p_offset
fields to same.

Also, given negative values are possible, use INTPTR_MAX instead of -1
as a better sentinel to indicate the value is unset.

Expected behaviour: 64-bit `ldconfig` runs silently, updating cache

Observed behaviour: `ldconfig` reports
```
ldconfig: file <filename> is truncated
```
... for any 32-bit ELF libs with dynamic strings in a segment with
p_vaddr > p_offset

Signed-off-by: Peter Edwards <peadar@arista.com>
---
 elf/readelflib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/elf/readelflib.c b/elf/readelflib.c
index f5b8c80e38..efab08ce3c 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -203,7 +203,7 @@ done:
 	{
 	  /* Find the file offset of the segment containing the dynamic
 	     string table.  */
-	  ElfW(Off) loadoff = -1;
+	  intptr_t loadoff = INTPTR_MAX;
 	  for (i = 0, segment = elf_pheader;
 	       i < elf_header->e_phnum; i++, segment++)
 	    {
@@ -212,11 +212,15 @@ done:
 		  && (dyn_entry->d_un.d_val - segment->p_vaddr
 		      < segment->p_filesz))
 		{
-		  loadoff = segment->p_vaddr - segment->p_offset;
+		  /* Note loadoff may be negative - the ELF headers may not be
+		     in a loadable segment, and the first loadable segment
+		     may be at a p_offset > 0, but p_vaddr == 0 */
+		  loadoff = (intptr_t)segment->p_vaddr -
+		      (intptr_t)segment->p_offset;
 		  break;
 		}
 	    }
-	  if (loadoff == (ElfW(Off)) -1)
+	  if (loadoff == INTPTR_MAX)
 	    {
 	      /* Very strange. */
 	      loadoff = 0;
-- 
2.42.0.111.gd814540bb7

Re: [PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[Paul Eggert]

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

On 2023-09-04 10:03, Peter Edwards via Libc-alpha wrote:
> +		  loadoff = (intptr_t)segment->p_vaddr -
> +		      (intptr_t)segment->p_offset;

This could suffer from intptr_t overflow, couldn't it? Signed integer 
overflow has undefined behavior.

> Also, given negative values are possible, use INTPTR_MAX instead of -1
> as a better sentinel 

When scanning arbitrary data even INTPTR_MAX is problematic at least in 
theory. Let's redo things to not make assumptions about data values.


Looking into the code a bit, it appears that elf/readelflib.c has a 
distressingly large number of pointer- and integer-overflow bugs with 
unusual or corrupt ELF data. And there's even a switch case with a 
missing 'break' at the end; ouch.

Attached is a proposed patch to fix the bugs I saw, including the bug 
you noticed. I'd appreciate your looking over it since you're familiar 
with this code.

[-- Attachment #2: 0001-elf-fix-arithmetic-bugs-in-process_elf_file.patch --]
[-- Type: text/x-patch, Size: 8619 bytes --]

From 9fb9575dfab623d210d5d78711a6fa69dc51d899 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 4 Sep 2023 16:45:13 -0700
Subject: [PATCH] elf: fix arithmetic bugs in process_elf_file

* elf/readelflib.c (check_ptr): Remove this macro, replacing with ...
(bad_offset): ... this new function.  All uses changed.
(process_elf_file): Don't assume dynamic_addr fits in int.
Check for offsets out of range, rather than bad pointers,
since pointer arithmetic might have undefined behavior
if a pointer goes outside its object (e.g., it might wrap around).
Do not calculate program_interpreter, which is unused, and anyway
the 'case PT_INTERP:' code to calculate it
didn't have 'break' at the end (!).
Check that offsets are properly aligned too.
Ignore trailing bytes in dynamic_size that might cause integer
arithmetic overflow.  Similarly, ignore trailing non-NUL bytes
in the string table, since xstrdup won't work with them.
Check sizes and alignments when scanning for NT_GNU_PROPERTY_TYPE_0
and for dynamic strings.
Remove no-longer-needed 'if (dynamic_size == 0)' test.
Recode so that there is no longer any need for a (loadoff ==
(ElfW(Off)) -1) test.
---
 elf/readelflib.c | 120 +++++++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 50 deletions(-)

diff --git a/elf/readelflib.c b/elf/readelflib.c
index f5b8c80e38..3109bfc86d 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -22,20 +22,32 @@
    which need to handle both 32bit and 64bit ELF libraries,  this file is
    included twice for each arch size.  */
 
-/* check_ptr checks that a pointer is in the mmaped file and doesn't
-   point outside it.  */
-#undef check_ptr
-#define check_ptr(ptr)						\
-do								\
-  {								\
-    if ((void *)(ptr) < file_contents				\
-	|| (void *)(ptr) > (file_contents+file_length))		\
-      {								\
-	error (0, 0, _("file %s is truncated\n"), file_name);	\
-	return 1;						\
-      }								\
-  }								\
- while (0);
+/* A single bad_offset implementation is valid for both 32- and 64-bit code,
+   so define it just once.  */
+#ifndef bad_offset_defined
+# define bad_offset_defined
+
+/* Check that OFFSET, ..., OFFSET + NITEMS * ITEMSIZE all fit within
+   (or just at the end of) a file of length FILE_LENGTH.  Also check
+   that OFFSET is a multiple of ALIGNMENT.  Return true (diagnosing
+   with FILE_NAME) if the offsets are invalid.  */
+static bool
+bad_offset (ElfW(Xword) offset, ElfW(Xword) nitems, size_t itemsize,
+	    size_t alignment, size_t file_length, char const *file_name)
+{
+  size_t nbytes;
+  if (!__builtin_mul_overflow (nitems, itemsize, &nbytes)
+      && (offset & (alignment - 1)) == 0
+      && nbytes <= file_length
+      && offset <= file_length - nbytes)
+    return false;
+  else
+    {
+      error (0, 0, _("file %s is truncated\n"), file_name);
+      return true;
+    }
+}
+#endif
 
 /* Returns 0 if everything is ok, != 0 in case of error.  */
 int
@@ -44,9 +56,8 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 		  size_t file_length)
 {
   int i;
-  unsigned int dynamic_addr;
+  ElfW(Off) dynamic_addr;
   size_t dynamic_size;
-  char *program_interpreter;
 
   ElfW(Ehdr) *elf_header;
   ElfW(Phdr) *elf_pheader, *segment;
@@ -77,8 +88,11 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
     }
 
   /* Get information from elf program header.  */
+  if (bad_offset (elf_header->e_phoff, elf_header->e_phnum,
+		  sizeof *elf_pheader, _Alignof (*elf_pheader),
+		  file_length, file_name))
+    return 1;
   elf_pheader = (ElfW(Phdr) *) (elf_header->e_phoff + file_contents);
-  check_ptr (elf_pheader);
 
   /* The library is an elf library.  */
   *flag = FLAG_ELF_LIBC6;
@@ -88,12 +102,9 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 
   dynamic_addr = 0;
   dynamic_size = 0;
-  program_interpreter = NULL;
   for (i = 0, segment = elf_pheader;
        i < elf_header->e_phnum; i++, segment++)
     {
-      check_ptr (segment);
-
       switch (segment->p_type)
 	{
 	case PT_DYNAMIC:
@@ -101,29 +112,30 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 	    error (0, 0, _("more than one dynamic segment\n"));
 
 	  dynamic_addr = segment->p_offset;
-	  dynamic_size = segment->p_filesz;
+	  if (bad_offset (dynamic_addr, segment->p_filesz, 1,
+			  _Alignof (*dyn_entry), file_length, file_name))
+	    return 1;
+	  dynamic_size = (segment->p_filesz
+			  - segment->p_filesz % sizeof *dyn_entry);
 	  break;
 
-	case PT_INTERP:
-	  program_interpreter = (char *) (file_contents + segment->p_offset);
-	  check_ptr (program_interpreter);
-
 	case PT_GNU_PROPERTY:
 	  /* The NT_GNU_PROPERTY_TYPE_0 note must be aligned to 4 bytes
 	     in 32-bit objects and to 8 bytes in 64-bit objects.  Skip
 	     notes with incorrect alignment.  */
 	  if (segment->p_align == (__ELF_NATIVE_CLASS / 8))
 	    {
-	      const ElfW(Nhdr) *note = (const void *) (file_contents
-						       + segment->p_offset);
-	      const ElfW(Addr) size = segment->p_filesz;
+	      if (bad_offset (segment->p_offset, segment->p_filesz, 1,
+			      _Alignof (ElfW(Nhdr)), file_length, file_name))
+		return 1;
+	      size_t note_offset = segment->p_offset;
+	      size_t note_offset_lim = note_offset + segment->p_filesz;
 	      const ElfW(Addr) align = segment->p_align;
-
-	      const ElfW(Addr) start = (ElfW(Addr)) (uintptr_t) note;
 	      unsigned int last_type = 0;
 
-	      while ((ElfW(Addr)) (uintptr_t) (note + 1) - start < size)
+	      while (sizeof (ElfW(Nhdr)) + 4 < note_offset_lim - note_offset)
 		{
+		  const ElfW(Nhdr) *note = file_contents + note_offset;
 		  /* Find the NT_GNU_PROPERTY_TYPE_0 note.  */
 		  if (note->n_namesz == 4
 		      && note->n_type == NT_GNU_PROPERTY_TYPE_0
@@ -131,6 +143,9 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 		    {
 		      /* Check for invalid property.  */
 		      if (note->n_descsz < 8
+			  || ((note_offset_lim - note_offset
+			       - (sizeof (ElfW(Nhdr)) + 4))
+			      < note->n_descsz)
 			  || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
 			goto done;
 
@@ -148,7 +163,7 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 			    goto done;
 
 			  ptr += 8;
-			  if ((ptr + datasz) > ptr_end)
+			  if (ptr_end - ptr < datasz)
 			    goto done;
 
 			  last_type = type;
@@ -187,23 +202,25 @@ done:
     }
 
   /* Now we can read the dynamic sections.  */
-  if (dynamic_size == 0)
-    return 1;
 
   dynamic_segment = (ElfW(Dyn) *) (file_contents + dynamic_addr);
-  check_ptr (dynamic_segment);
 
   /* Find the string table.  */
   dynamic_strings = NULL;
-  for (dyn_entry = dynamic_segment; dyn_entry->d_tag != DT_NULL;
-       ++dyn_entry)
+  size_t stringbytes;
+
+  for (size_t dynoff = 0; dynoff < dynamic_size; dynoff += sizeof *dyn_entry)
     {
-      check_ptr (dyn_entry);
+      if (bad_offset (dynoff, 1, sizeof *dyn_entry, _Alignof (*dyn_entry),
+		      dynamic_size, file_name))
+	return 1;
+      dyn_entry = (ElfW(Dyn) *) (file_contents + dynamic_addr + dynoff);
+      if (dyn_entry->d_tag == DT_NULL)
+	break;
       if (dyn_entry->d_tag == DT_STRTAB)
 	{
 	  /* Find the file offset of the segment containing the dynamic
 	     string table.  */
-	  ElfW(Off) loadoff = -1;
 	  for (i = 0, segment = elf_pheader;
 	       i < elf_header->e_phnum; i++, segment++)
 	    {
@@ -212,19 +229,20 @@ done:
 		  && (dyn_entry->d_un.d_val - segment->p_vaddr
 		      < segment->p_filesz))
 		{
-		  loadoff = segment->p_vaddr - segment->p_offset;
+		  dynamic_strings = (char *) (file_contents
+					      + (dyn_entry->d_un.d_val
+						 - segment->p_vaddr)
+					      + segment->p_offset);
+
+		  /* Ignore any trailing non-NUL bytes at string table end,
+		     since xstrdup wouldn't work with them.  */
+		  for (stringbytes = segment->p_filesz;
+		       0 < stringbytes && dynamic_strings[stringbytes - 1];
+		       stringbytes--)
+		    continue;
 		  break;
 		}
 	    }
-	  if (loadoff == (ElfW(Off)) -1)
-	    {
-	      /* Very strange. */
-	      loadoff = 0;
-	    }
-
-	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val
-				      - loadoff);
-	  check_ptr (dynamic_strings);
 	  break;
 	}
     }
@@ -238,8 +256,10 @@ done:
     {
       if (dyn_entry->d_tag == DT_SONAME)
 	{
+	  if (bad_offset (dyn_entry->d_un.d_val, 1, 1, 1,
+			  stringbytes, file_name))
+	    return 1;
 	  char *name = dynamic_strings + dyn_entry->d_un.d_val;
-	  check_ptr (name);
           *soname = xstrdup (name);
           return 0;
 	}
-- 
2.41.0

Re: [PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[Andreas Schwab]

On Sep 04 2023, Paul Eggert wrote:

> +/* A single bad_offset implementation is valid for both 32- and 64-bit code,
> +   so define it just once.  */
> +#ifndef bad_offset_defined
> +# define bad_offset_defined
> +
> +/* Check that OFFSET, ..., OFFSET + NITEMS * ITEMSIZE all fit within
> +   (or just at the end of) a file of length FILE_LENGTH.  Also check
> +   that OFFSET is a multiple of ALIGNMENT.  Return true (diagnosing
> +   with FILE_NAME) if the offsets are invalid.  */
> +static bool
> +bad_offset (ElfW(Xword) offset, ElfW(Xword) nitems, size_t itemsize,

Please don't make it look like it depends on ElfW.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[Peter Edwards]

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

On Tue, 5 Sept 2023 at 00:51, Paul Eggert <eggert@cs.ucla.edu> wrote:

> Attached is a proposed patch to fix the bugs I saw, including the bug
> you noticed. I'd appreciate your looking over it since you're familiar
> with this code.

I am a novice here - I first saw this code the other day, but for what my
opinion is worth, the patch looks fine to me, modulo the same issue Andreas
has - if we're doing pointer arithmetic in the host environment, I think
it's more appropriate to use offset types natural to the host environment,
rather than the ElfXX_ types for the elf class we're looking at. The unused
PT_INTERP and I assume accidental lack of a break was a nice catch.

Re: [PATCH v2] elf: Avoid pointer-arithmetic underflow in ldconfig

[Andreas Schwab]

On Sep 04 2023, Paul Eggert wrote:

> +	      while (sizeof (ElfW(Nhdr)) + 4 < note_offset_lim - note_offset)

	      while (note_offset_lim - note_offset > sizeof (ElfW(Nhdr)) + 4)

> +		       0 < stringbytes && dynamic_strings[stringbytes - 1];

		       stringbytes > 0 && dynamic_strings[stringbytes - 1];

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."