Re: [PATCH][RFC] invalid ELF binaries can execute - better sanitychecking

[Jesper Juhl]

On Fri, 9 Jan 2004, Jakub Jelinek wrote:

> On Fri, Jan 09, 2004 at 03:19:12AM +0100, Jesper Juhl wrote:
> > --- linux-2.6.1-rc1-mm2-orig/fs/binfmt_elf.c    2003-12-31 05:47:13.000000000 +0100
> > +++ linux-2.6.1-rc1-mm2/fs/binfmt_elf.c 2004-01-09 01:41:05.000000000 +0100
> > @@ -482,11 +482,14 @@ static int load_elf_binary(struct linux_
> >         /* First of all, some simple consistency checks */
> >         if (memcmp(elf_ex.e_ident, ELFMAG, SELFMAG) != 0)
> >                 goto out;
> > -
> > +       if (elf_ex.e_ident[EI_CLASS] == ELFCLASSNONE)
> > +               goto out;
> >         if (elf_ex.e_type != ET_EXEC && elf_ex.e_type != ET_DYN)
> >                 goto out;
> >         if (!elf_check_arch(&elf_ex))
> >                 goto out;
> > +       if (elf_ex.e_version == EV_NONE)
> > +               goto out;
> >         if (!bprm->file->f_op||!bprm->file->f_op->mmap)
> >                 goto out;
>
> These checks look useless for me.
> If you want to check EI_CLASS or e_version, why is 0 so special and not say
> 157?

You are absolutely correct. Those tests are very weak, and (as I tried to
explain in my original mail), I did not intend the patch to be merged as
it was. I only did those (admittedly very weak) checks initially to be
able to easily verify with a test program that my checks where indeed
checking the right fields of the ELF header, and that introducing
aditional checking did not make the kernel "blow up".
I'm working on a proper patch that'll do the real checks of these ELF
header fields, as well as add additional checks.
I'll be submitting that patch as soon as it's done. I'm aiming to have
something ready during the weekend.

> Why not to check EI_DATA and EI_VERSION as well though?
> glibc loader does:

I plan to.
I just wanted to get some feedback initially. The patch was a very minor
bit of the email I send, and probably the least important bit.
I wanted to get peoples reactions to the thought of adding stronger sanity
checks. The patch was just a minor thing - the discussion about "do we
want additional checks?" was the important bit.


> Perhaps binfmt_elf.c wants to be able to load different OSABI ELF objects,
> if so, it could just memcmp the first EI_OSABI bytes of e_ident and check
> e_version and other fields outside of e_ident.
>
I'll keep that in mind. Thank you.


-- Jesper Juhl

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/