Re: [PATCH 10/22] libelf: check nul-terminated strings properly

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/06/13 19:20, Ian Jackson wrote:
> It is not safe to simply take pointers into the ELF and use them as C
> pointers.  They might not be properly nul-terminated (and the pointers
> might be wild).
>
> So we are going to introduce a new function elf_strval for safely
> getting strings.  This will check that the addresses are in range and
> that there is a proper nul-terminated string.  Of course it might
> discover that there isn't.  In that case, it will be made to fail.
> This means that elf_note_name might fail, too.
>
> For the benefit of call sites which are just going to pass the value
> to a printf-like function, we provide elf_strfmt which returns
> "(invalid)" on failure rather than NULL.
>
> In this patch we introduce dummy definitions of these functions.  We
> introduce calls to elf_strval and elf_strfmt everywhere, and update
> all the call sites with appropriate error checking.
>
> There is not yet any semantic change, since before this patch all the
> places where we introduce elf_strval dereferenced the value anyway, so
> it mustn't have been NULL.
>
> In future patches, when elf_strval is made able return NULL, when it
> does so it will mark the elf "broken" so that an appropriate
> diagnostic can be printed.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> v7: Change readnotes.c check to use two if statements rather than ||.
>
> v2: Fix coding style, in one "if" statement.
> ---
>  tools/xcutils/readnotes.c          |   11 ++++++++---
>  xen/common/libelf/libelf-dominfo.c |   13 ++++++++++---
>  xen/common/libelf/libelf-tools.c   |   10 +++++++---
>  xen/include/xen/libelf.h           |    7 +++++--
>  4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
> index 7ff2530..cfae994 100644
> --- a/tools/xcutils/readnotes.c
> +++ b/tools/xcutils/readnotes.c
> @@ -63,7 +63,7 @@ struct setup_header {
>  static void print_string_note(const char *prefix, struct elf_binary *elf,
>  			      ELF_HANDLE_DECL(elf_note) note)
>  {
> -	printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note));
> +	printf("%s: %s\n", prefix, elf_strfmt(elf, elf_note_desc(elf, note)));
>  }
>  
>  static void print_numeric_note(const char *prefix, struct elf_binary *elf,
> @@ -103,10 +103,14 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
>  {
>  	ELF_HANDLE_DECL(elf_note) note;
>  	int notes_found = 0;
> +	const char *this_note_name;
>  
>  	for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) )
>  	{
> -		if (0 != strcmp(elf_note_name(elf, note), "Xen"))
> +		this_note_name = elf_note_name(elf, note);
> +		if (NULL == this_note_name)
> +			continue;
> +		if (0 != strcmp(this_note_name, "Xen"))
>  			continue;
>  
>  		notes_found++;
> @@ -294,7 +298,8 @@ int main(int argc, char **argv)
>  
>  	shdr = elf_shdr_by_name(&elf, "__xen_guest");
>  	if (ELF_HANDLE_VALID(shdr))
> -		printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr));
> +		printf("__xen_guest: %s\n",
> +                       elf_strfmt(&elf, elf_section_start(&elf, shdr)));
>  
>  	return 0;
>  }
> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
> index 566f6f9..ba0dc83 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -137,7 +137,10 @@ int elf_xen_parse_note(struct elf_binary *elf,
>  
>      if ( note_desc[type].str )
>      {
> -        str = elf_note_desc(elf, note);
> +        str = elf_strval(elf, elf_note_desc(elf, note));
> +        if (str == NULL)
> +            /* elf_strval will mark elf broken if it fails so no need to log */
> +            return 0;
>          elf_msg(elf, "%s: %s = \"%s\"\n", __FUNCTION__,
>                  note_desc[type].name, str);
>          parms->elf_notes[type].type = XEN_ENT_STR;
> @@ -220,6 +223,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
>  {
>      int xen_elfnotes = 0;
>      ELF_HANDLE_DECL(elf_note) note;
> +    const char *note_name;
>  
>      parms->elf_note_start = start;
>      parms->elf_note_end   = end;
> @@ -227,7 +231,10 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
>            ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
>            note = elf_note_next(elf, note) )
>      {
> -        if ( strcmp(elf_note_name(elf, note), "Xen") )
> +        note_name = elf_note_name(elf, note);
> +        if ( note_name == NULL )
> +            continue;
> +        if ( strcmp(note_name, "Xen") )
>              continue;
>          if ( elf_xen_parse_note(elf, parms, note) )
>              return -1;
> @@ -541,7 +548,7 @@ int elf_xen_parse(struct elf_binary *elf,
>                  parms->elf_note_start = ELF_INVALID_PTRVAL;
>                  parms->elf_note_end   = ELF_INVALID_PTRVAL;
>                  elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> -                        parms->guest_info);
> +                        elf_strfmt(elf, parms->guest_info));
>                  elf_xen_parse_guest_info(elf, parms);
>                  break;
>              }
> diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
> index bf68bcd..fa7dedd 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -119,7 +119,7 @@ const char *elf_section_name(struct elf_binary *elf,
>      if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
>          return "unknown";
>  
> -    return elf->sec_strtab + elf_uval(elf, shdr, sh_name);
> +    return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name));
>  }
>  
>  ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
> @@ -151,6 +151,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
>      ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab);
>      ELF_HANDLE_DECL(elf_sym) sym;
>      uint64_t info, name;
> +    const char *sym_name;
>  
>      for ( ; ptr < end; ptr += elf_size(elf, sym) )
>      {
> @@ -159,7 +160,10 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
>          name = elf_uval(elf, sym, st_name);
>          if ( ELF32_ST_BIND(info) != STB_GLOBAL )
>              continue;
> -        if ( strcmp(elf->sym_strtab + name, symbol) )
> +        sym_name = elf_strval(elf, elf->sym_strtab + name);
> +        if ( sym_name == NULL ) /* out of range, oops */
> +            return ELF_INVALID_HANDLE(elf_sym);
> +        if ( strcmp(sym_name, symbol) )
>              continue;
>          return sym;
>      }
> @@ -177,7 +181,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index)
>  
>  const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
>  {
> -    return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note);
> +    return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note));
>  }
>  
>  ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index 7bd3bdb..28c7b11 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -252,6 +252,9 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr,
>  uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
>  
>  
> +#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */
> +#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */
> +
>  #define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz))
>  #define elf_memset_safe(elf, dst, c, sz)   memset((dst),(c),(sz))
>    /*
> @@ -279,7 +282,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
>  ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index);
>  ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index);
>  
> -const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
> +const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */
>  ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
>  ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
>  
> @@ -289,7 +292,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el
>  ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol);
>  ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index);
>  
> -const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
> +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */
>  ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
>  uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
>  uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),