Re: [Xen-devel] [PATCH 15/21] libelf: check loops for running away

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jun 12, 2013 at 5:00 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> Ensure that libelf does not have any loops which can run away
> indefinitely even if the input is bogus.  (Grepped for \bfor, \bwhile
> and \bgoto in libelf and xc_dom_*loader*.c.)
>
> Changes needed:
>  * elf_note_next uses the note's unchecked alleged length, which might
>    wrap round.  If it does, return ELF_MAX_PTRVAL (0xfff..fff) instead,
>    which will be beyond the end of the section and so terminate the
>    caller's loop.
>  * In various loops over section and program headers, check that the
>    calculated header pointer is still within the image, and quit the
>    loop if it isn't.
>
> We have not changed loops which might, in principle, iterate over the
> whole image - even if they might do so one byte at a time with a
> nontrivial access check function in the middle.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
>  tools/libxc/xc_dom_elfloader.c     |   33 ++++++++++++++++++------
>  xen/common/libelf/libelf-dominfo.c |   48 
> ++++++++++++++++++++++++------------
>  xen/common/libelf/libelf-loader.c  |   47 +++++++++++++++++++++++++++++++++-
>  xen/common/libelf/libelf-tools.c   |   28 +++++++++++++++++++-
>  xen/include/xen/libelf.h           |    3 ++
>  5 files changed, 130 insertions(+), 29 deletions(-)
>
[snip]
> diff --git a/xen/common/libelf/libelf-dominfo.c 
> b/xen/common/libelf/libelf-dominfo.c
> index 6054e40..8f6bdec 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
[snip]
> @@ -471,6 +479,13 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
>      for ( i = 0; i < count; i++ )
>      {
>          phdr = elf_phdr_by_index(elf, i);
> +        /*
> +         * This test also arranges for the loop to terminate if the
> +         * input file has a ridiculous value for the header count: The
> +         * first putative header outside the input image will appear
> +         * to have type 0 (since out-of-range accesses read as 0) and
> +         * PT_NOTE != 0.
> +         */
>          if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
>              continue;

Sorry, run that by me again?  It looks like it will call "continue" in
that case, not "break".

This looks inconsistent with the changes you made in libelf-loader.c
and xc_dom_elfloader.c: There we have similar types of reads with
continues, but you do an access check anyway.

Is there something special about phdr vs shdr?

>
> @@ -483,7 +498,8 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
>
>          more_notes = elf_xen_parse_notes(elf, parms,
>                                   elf_segment_start(elf, phdr),
> -                                 elf_segment_end(elf, phdr));
> +                                 elf_segment_end(elf, phdr),
> +                                 &total_note_count);
>          if ( more_notes == ELF_NOTE_INVALID )
>              return -1;
>
> @@ -501,12 +517,17 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
>          {
>              shdr = elf_shdr_by_index(elf, i);
>
> +            /*
> +             * See above re guarantee of loop termination.
> +             * SHT_NOTE != 0.
> +             */
>              if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
>                  continue;

Same as above.

> @@ -524,20 +545,15 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
>       */
>      if ( xen_elfnotes == 0 )
>      {
> -        count = elf_shdr_count(elf);
> -        for ( i = 0; i < count; i++ )
> +        shdr = elf_shdr_by_name(elf, "__xen_guest");
> +        if ( ELF_HANDLE_VALID(shdr) )
>          {
> -            shdr = elf_shdr_by_name(elf, "__xen_guest");
> -            if ( ELF_HANDLE_VALID(shdr) )
> -            {
> -                parms->guest_info = elf_section_start(elf, shdr);
> -                parms->elf_note_start = ELF_INVALID_PTRVAL;
> -                parms->elf_note_end   = ELF_INVALID_PTRVAL;
> -                elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> -                        elf_strfmt(elf, parms->guest_info));
> -                elf_xen_parse_guest_info(elf, parms);
> -                break;
> -            }
> +            parms->guest_info = elf_section_start(elf, shdr);
> +            parms->elf_note_start = ELF_INVALID_PTRVAL;
> +            parms->elf_note_end   = ELF_INVALID_PTRVAL;
> +            elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> +                    elf_strfmt(elf, parms->guest_info));
> +            elf_xen_parse_guest_info(elf, parms);
>          }
>      }

Wow -- nice loop.  Better check that one thing N times, just to be sure... :-)


> @@ -306,7 +323,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct 
> elf_binary *elf, ELF_HANDLE_DECL(
>      unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
>      unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
>
> -    return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, 
> note) + namesz + descsz);
> +    elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note)
> +        + elf_size(elf, note) + namesz + descsz;
> +
> +    if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */
> +           !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) )
> +        ptrval = ELF_MAX_PTRVAL; /* terminate caller's loop */

Is this an official part of the interface, or do you just "know" that
the two callers will happen to break out of their loop when this
happens?

If it's the second, this seems like a future bug waiting to happen...

Other than that, it looks like all the changes do what they say on the tin.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel