From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 12/22] libelf: Check pointer references in elf_is_elfbinary Date: Tue, 11 Jun 2013 17:00:51 +0100 Message-ID: <51B749B3.5040202@citrix.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> <20919.15933.555614.734706@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20919.15933.555614.734706@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org 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