From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH 12/22] libelf: Check pointer references in
	elf_is_elfbinary
Date: Tue, 11 Jun 2013 16:11:57 +0100
Message-ID: <20919.15933.555614.734706@mariner.uk.xensource.com>
References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com>
	<1370629642-6990-13-git-send-email-ian.jackson@eu.citrix.com>
	<51B65B8D.9070009@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <51B65B8D.9070009@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, "mattjd@gmail.com" <mattjd@gmail.com>, "security@xen.org" <security@xen.org>
List-Id: xen-devel@lists.xenproject.org

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.