Re: [PATCH 12/22] libelf: Check pointer references in elf_is_elfbinary

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

On 07/06/13 19:27, Ian Jackson wrote:
> elf_is_elfbinary didn't take a length parameter and could potentially
> access out of range when provided with a very short image.
>
> 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>
>
> v2: Style fix.
>     Fix commit message subject.
> ---
>  tools/libxc/xc_dom_elfloader.c    |    2 +-
>  xen/arch/x86/bzimage.c            |    4 ++--
>  xen/common/libelf/libelf-loader.c |    2 +-
>  xen/common/libelf/libelf-tools.c  |    9 ++++++---
>  xen/include/xen/libelf.h          |    2 +-
>  5 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index c038d1c..f14b053 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -93,7 +93,7 @@ static int check_elf_kernel(struct xc_dom_image *dom, int verbose)
>          return -EINVAL;
>      }
>  
> -    if ( !elf_is_elfbinary(dom->kernel_blob) )
> +    if ( !elf_is_elfbinary(dom->kernel_blob, dom->kernel_size) )
>      {
>          if ( verbose )
>              xc_dom_panic(dom->xch,
> diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
> index c5519d8..58fda16 100644
> --- a/xen/arch/x86/bzimage.c
> +++ b/xen/arch/x86/bzimage.c
> @@ -220,7 +220,7 @@ unsigned long __init bzimage_headroom(char *image_start,
>          image_length = hdr->payload_length;
>      }
>  
> -    if ( elf_is_elfbinary(image_start) )
> +    if ( elf_is_elfbinary(image_start, image_length) )
>          return 0;
>  
>      orig_image_len = image_length;
> @@ -251,7 +251,7 @@ int __init bzimage_parse(char *image_base, char **image_start, unsigned long *im
>          *image_len = hdr->payload_length;
>      }
>  
> -    if ( elf_is_elfbinary(*image_start) )
> +    if ( elf_is_elfbinary(*image_start, *image_len) )
>          return 0;
>  
>      BUG_ON(!(image_base < *image_start));
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 878552e..6c43c34 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -29,7 +29,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size)
>      ELF_HANDLE_DECL(elf_shdr) shdr;
>      uint64_t i, count, section, offset;
>  
> -    if ( !elf_is_elfbinary(image_input) )
> +    if ( !elf_is_elfbinary(image_input, size) )
>      {
>          elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__);
>          return -1;
> diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
> index 08ab027..b613593 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -332,11 +332,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
>  
>  /* ------------------------------------------------------------------------ */
>  
> -int elf_is_elfbinary(const void *image)
> +int elf_is_elfbinary(const void *image_start, size_t image_size)
>  {
> -    const Elf32_Ehdr *ehdr = image;
> +    const Elf32_Ehdr *ehdr = image_start;
>  
> -    return IS_ELF(*ehdr); /* fixme unchecked */
> +    if ( image_size < sizeof(*ehdr) )
> +        return 0;
> +
> +    return IS_ELF(*ehdr);
>  }

sizeof(Elf32_Ehdr) == 52
sizeof(Elf64_Ehdr) == 64

As stated by a trivial C program containing 'printf("32: %zu, 64:
%zu\n", sizeof(Elf32_Ehdr), sizeof(Elf64_Ehdr));'

Therefore, the sizeof check is insufficient if given a 64bit elf.

It needs to check first against sizeof e_ident which is consistent
between the two, look up EI_CLASS in the ident, then choose the
appropriate structure.

~Andrew

>  
>  int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index 783f125..c54c90b 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -349,7 +349,7 @@ uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),
>                                  unsigned int unitsz, unsigned int idx);
>  ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
>  
> -int elf_is_elfbinary(const void *image);
> +int elf_is_elfbinary(const void *image_start, size_t image_size);
>  int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
>  
>  /* ------------------------------------------------------------------------ */