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

[Mark Wielaard <mark@klomp.org>]

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