diff mbox series

[v2,1/1] libdw: check that DWARF strings are null-terminated

Message ID 77e629c6d4992af3701c2e4bd79dae2df99a1737.1676406334.git.vvvvvv@google.com
State Committed
Headers
Series libdw: check that DWARF strings are null-terminated |

Commit Message

Aleksei Vetrov Feb. 14, 2023, 8:30 p.m. UTC 
  It is expected from libdw to return strings that are null-terminated to
avoid overflowing ELF data.

* Add calculation of a safe prefix inside string sections, where any
  string will be null-terminated.

* Check if offset overflows the safe prefix in dwarf_formstring.

Signed-off-by: Aleksei Vetrov <vvvvvv@google.com>
---
 libdw/dwarf_begin_elf.c  | 37 +++++++++++++++++++++++++++++++++++++
 libdw/dwarf_formstring.c |  5 ++++-
 libdw/libdwP.h           | 11 +++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

Comments

Mark Wielaard Feb. 16, 2023, 11:45 p.m. UTC | #1 
Hi Aleksei,

On Tue, Feb 14, 2023 at 08:30:02PM +0000, Aleksei Vetrov via Elfutils-devel wrote:
> It is expected from libdw to return strings that are null-terminated to
> avoid overflowing ELF data.
> 
> * Add calculation of a safe prefix inside string sections, where any
>   string will be null-terminated.
> 
> * Check if offset overflows the safe prefix in dwarf_formstring.

This is a very nice sanity/hardening fix.

> +  /* If the section contains string data, we want to know a size of a prefix
> +     where any string will be null-terminated. */
> +  enum string_section_index string_section_idx = scn_to_string_section_idx[cnt];
> +  if (string_section_idx < STR_SCN_IDX_last)
> +    {
> +      size_t size = data->d_size;
> +      /* Reduce the size by the number of non-zero bytes at the end of the
> +	 section.  */
> +      while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0')
> +	--size;
> +      result->string_section_size[string_section_idx] = size;
> +    }

Checking from the end is smart. In the normal case the debug string
section will end with a zero terminator (or zero padding), so this
should be really quick.

> @@ -171,7 +174,7 @@ dwarf_formstring (Dwarf_Attribute *attrp)
>        else
>  	off = read_8ubyte_unaligned (dbg, datap);
>  
> -      if (off > dbg->sectiondata[IDX_debug_str]->d_size)
> +      if (off >= data_size)
>  	goto invalid_offset;
>      }

O, the original check was actually one off.

Looks good. Pushed as is.

Thanks,

Mark
diff mbox series

Patch

diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 8fcef335..1d4bb333 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -70,6 +70,30 @@  static const char dwarf_scnnames[IDX_last][19] =
 };
 #define ndwarf_scnnames (sizeof (dwarf_scnnames) / sizeof (dwarf_scnnames[0]))
 
+/* Map from section index to string section index.
+   Non-string sections should have STR_SCN_IDX_last.  */
+static const enum string_section_index scn_to_string_section_idx[IDX_last] =
+{
+  [IDX_debug_info] = STR_SCN_IDX_last,
+  [IDX_debug_types] = STR_SCN_IDX_last,
+  [IDX_debug_abbrev] = STR_SCN_IDX_last,
+  [IDX_debug_addr] = STR_SCN_IDX_last,
+  [IDX_debug_aranges] = STR_SCN_IDX_last,
+  [IDX_debug_line] = STR_SCN_IDX_last,
+  [IDX_debug_line_str] = STR_SCN_IDX_debug_line_str,
+  [IDX_debug_frame] = STR_SCN_IDX_last,
+  [IDX_debug_loc] = STR_SCN_IDX_last,
+  [IDX_debug_loclists] = STR_SCN_IDX_last,
+  [IDX_debug_pubnames] = STR_SCN_IDX_last,
+  [IDX_debug_str] = STR_SCN_IDX_debug_str,
+  [IDX_debug_str_offsets] = STR_SCN_IDX_last,
+  [IDX_debug_macinfo] = STR_SCN_IDX_last,
+  [IDX_debug_macro] = STR_SCN_IDX_last,
+  [IDX_debug_ranges] = STR_SCN_IDX_last,
+  [IDX_debug_rnglists] = STR_SCN_IDX_last,
+  [IDX_gnu_debugaltlink] = STR_SCN_IDX_last
+};
+
 static enum dwarf_type
 scn_dwarf_type (Dwarf *result, size_t shstrndx, Elf_Scn *scn)
 {
@@ -230,6 +254,19 @@  check_section (Dwarf *result, size_t shstrndx, Elf_Scn *scn, bool inscngrp)
   /* We can now read the section data into results. */
   result->sectiondata[cnt] = data;
 
+  /* If the section contains string data, we want to know a size of a prefix
+     where any string will be null-terminated. */
+  enum string_section_index string_section_idx = scn_to_string_section_idx[cnt];
+  if (string_section_idx < STR_SCN_IDX_last)
+    {
+      size_t size = data->d_size;
+      /* Reduce the size by the number of non-zero bytes at the end of the
+	 section.  */
+      while (size > 0 && *((const char *) data->d_buf + size - 1) != '\0')
+	--size;
+      result->string_section_size[string_section_idx] = size;
+    }
+
   return result;
 }
 
diff --git a/libdw/dwarf_formstring.c b/libdw/dwarf_formstring.c
index c3e892a8..0ee42411 100644
--- a/libdw/dwarf_formstring.c
+++ b/libdw/dwarf_formstring.c
@@ -61,6 +61,9 @@  dwarf_formstring (Dwarf_Attribute *attrp)
   Elf_Data *data = ((attrp->form == DW_FORM_line_strp)
 		    ? dbg_ret->sectiondata[IDX_debug_line_str]
 		    : dbg_ret->sectiondata[IDX_debug_str]);
+  size_t data_size = ((attrp->form == DW_FORM_line_strp)
+		      ? dbg_ret->string_section_size[STR_SCN_IDX_debug_line_str]
+		      : dbg_ret->string_section_size[STR_SCN_IDX_debug_str]);
   if (data == NULL)
     {
       __libdw_seterrno ((attrp->form == DW_FORM_line_strp)
@@ -171,7 +174,7 @@  dwarf_formstring (Dwarf_Attribute *attrp)
       else
 	off = read_8ubyte_unaligned (dbg, datap);
 
-      if (off > dbg->sectiondata[IDX_debug_str]->d_size)
+      if (off >= data_size)
 	goto invalid_offset;
     }
 
diff --git a/libdw/libdwP.h b/libdw/libdwP.h
index 961fa4e7..5cbdc279 100644
--- a/libdw/libdwP.h
+++ b/libdw/libdwP.h
@@ -86,6 +86,13 @@  enum
     IDX_last
   };
 
+/* Valid indices for the string section's information.  */
+enum string_section_index
+  {
+    STR_SCN_IDX_debug_line_str,
+    STR_SCN_IDX_debug_str,
+    STR_SCN_IDX_last
+  };
 
 /* Error values.  */
 enum
@@ -169,6 +176,10 @@  struct Dwarf
   /* The section data.  */
   Elf_Data *sectiondata[IDX_last];
 
+  /* Size of a prefix of string sections, where any string will be
+     null-terminated. */
+  size_t string_section_size[STR_SCN_IDX_last];
+
   /* True if the file has a byte order different from the host.  */
   bool other_byte_order;