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

[Jakub Jelinek <jakub@redhat.com>]

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?
If you want to be sure EI_CLASS and e_version are correct, the test should
be:
	if (elf_ex.e_ident[EI_CLASS] != ELF_CLASS)
		goto out;
resp.
	if (elf_ex.e_version != EV_CURRENT)
		goto out;
Furthermore, there is load_elf_interp, which needs similar checks, otherwise
they are useless, because you can create a proper ELF binary loading
incorrect ELF interpreter.
Why not to check EI_DATA and EI_VERSION as well though?
glibc loader does:
#ifndef VALID_ELF_HEADER
# define VALID_ELF_HEADER(hdr,exp,size) (memcmp (hdr, exp, size) == 0)
# define VALID_ELF_OSABI(osabi)         (osabi == ELFOSABI_SYSV)
# define VALID_ELF_ABIVERSION(ver)      (ver == 0)
#endif
  static const unsigned char expected[EI_PAD] =
  {
    [EI_MAG0] = ELFMAG0,
    [EI_MAG1] = ELFMAG1,
    [EI_MAG2] = ELFMAG2,
    [EI_MAG3] = ELFMAG3,
    [EI_CLASS] = ELFW(CLASS),
    [EI_DATA] = byteorder,
    [EI_VERSION] = EV_CURRENT,
    [EI_OSABI] = ELFOSABI_SYSV,
    [EI_ABIVERSION] = 0
  };
      if (__builtin_expect (! VALID_ELF_HEADER (ehdr->e_ident, expected,
                                                EI_PAD), 0))
        {
...
	}
      if (__builtin_expect (ehdr->e_version, EV_CURRENT) != EV_CURRENT)
        {
          errstring = N_("ELF file version does not match current one");
          goto call_lose;
        }
      if (! __builtin_expect (elf_machine_matches_host (ehdr), 1))
        goto close_and_out;
...

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.

	Jakub