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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/06/13 16:11, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 12/22] libelf: Check pointer references in 
> elf_is_elfbinary"):
>> On 07/06/13 19:27, Ian Jackson wrote:
>>> @@ -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.
> The purpose of the size check here is to ensure that IS_ELF does not
> compute or dereference a wild pointer.  It isn't intended to ensure
> that the ELF actually has a valid header length.
>
> I don't think that's necessary because this function's purpose is the
> magic number check.  It doesn't promise that the ELF isn't malformed
> somehow.  We need an explicit size check here because we call
> elf_is_elfbinary /before/ setting up the elf_image and our
> pointer-checking machinery; elf_is_elfbinary uses naked pointer
> arithmetic so needs explicit checking.
>
> Ian.

Ok on the argument regarding the validity of Elf32_Ehdr.

However, I would then suggest that const Elf32_Ehdr should really be an
unsigned char e_ident[], and the length check should be against
EI_NIDENT to avoid giving the false impression that it is validating the
entire Ehdr.

~Andrew

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