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

[Jesper Juhl]

Arrgh, sorry for the re-post/reply to self, people. I accidentilly send
the original mail from the wrong email addr. which I don't usually follow
up on, so please reply to this mail and the juhl-lkml@xxxxxx addr.

Sorry for the trouble...
Seems I have to screw /something/ up every time I post to lkml :(

-- Jesper Juhl


On Fri, 9 Jan 2004, Jesper Juhl wrote:

>
> The current Linux kernel does only very basic sanity checking on ELF
> binaries.
> In my oppinion, any attempt to load an invalid/corrupted binary should
> fail as early as possible. Currently Linux will assign a PID to a lot of
> different variants of broken ELF binaries, so I took it upon myself to fix
> that up a bit.
>
> Why bother checking validity of a binary too closely?
>
> Well, the reasons I can thing of include
>
> - Correctness. If it's invalid it /should/ fail, and as early as possible.
>
> - Stability. Who knows when it'll crash and what damage it may have
> done before it crashes?
>
> - Least amount of surprise for the user. If a binary has become corrupted
> the user is likely to want to be told it's bad when loading it (if
> possible), rather than being left wondering about strange crashes during
> runtime.
>
> - Security 1. Is it not plausible that someone may try to play tricks on
> the kernel with invalid binaries? Isn't it safer just rejecting them if we
> *know* they are bad?
>
> - Security 2. If a virus/worm/trojan/whatever attempts to infect a binary
> and does not do a perfect job of fixing up the ELF header, section table
> headers etc, then with the current code we would in some cases still run
> the binary. If we enforce as many sanity checks as possible such an
> infected binary willlikely fail to run.
>
>
> The patch below only implements two additional sanity checks, and they are
> very weak. This code is not intended to be merged in its current form, I
> only did it as proof that more valid sanity checks are possible (well, you
> probably already knew that), and to prove that I /do/ have a basic
> understanding of the ELF format.. well, consider it flame detergent, code
> talks, BS walks - tends to be the norm on LKML ;)
>
> The two checks I've implemented in this patch simply check that e_version
> is not EV_NONE (Invalid version), and that e_ident[EI_CLASS] is not
> ELFCLASSNONE (Invalid class). No binaries looking like that should ever
> exist, so they are valid (albeit not very strong) sanity checks.
>
>
> What I would like to know at this point is whether adding additional
> checks to load_elf_binary() in binfmt_elf is worthwhile and desirable, or
> if there's some (unknown to me) very good reason to only do the very basic
> checks that are currently done?
> If there's an interrest in seeing strong sanity checks done in ELF binary
> loading, then I'll attempt to expand my patch to implement whatever sanity
> checks the ELF spec allows for (and ofcourse re-do the checks below right
> so they check for the exact valid value and reject anything else instead
> of just test for a single 'known to be invalid' value).
> Initially I'd be dealing with i386 only, as that's all I can actually test
> with, but I can get access to x86-64 hardware as well and I would do my
> very best to do this for all archs.
>
> In order to test my current code I've done the following:
>
> Test if it compiles without errors/warnings
>    - it does.
>
> Test if a kernel with this patch applied boots and is able to run a basic
> Linux distribution
>    - it boots and currently runs my Slackware 9.1 install just fine.
>
> Create a minimal test program that is easily modifyable with a hex editor
> to create test-case binaries that /should/ fail the sanity check.
>    - I've been using the minimal program below and it does fail if
>      modified to contain the 'tested for, invalid' header fields.
>
>
> ; Test program start - original code from
> ; http://www.muppetlabs.com/~breadbox/software/tiny/teensy.html
>
>                org     0x08048000
>
>   ehdr:                                                 ; Elf32_Ehdr
>                 db      0x7F, "ELF", 1, 1, 1            ;   e_ident
>         times 9 db      0
>                 dw      2                               ;   e_type
>                 dw      3                               ;   e_machine
>                 dd      1                               ;   e_version
>                 dd      _start                          ;   e_entry
>                 dd      phdr - $$                       ;   e_phoff
>                 dd      0                               ;   e_shoff
>                 dd      0                               ;   e_flags
>                 dw      ehdrsize                        ;   e_ehsize
>                 dw      phdrsize                        ;   e_phentsize
>                 dw      1                               ;   e_phnum
>                 dw      0                               ;   e_shentsize
>                 dw      0                               ;   e_shnum
>                 dw      0                               ;   e_shstrndx
>
>   ehdrsize      equ     $ - ehdr
>
>   phdr:                                                 ; Elf32_Phdr
>                 dd      1                               ;   p_type
>                 dd      0                               ;   p_offset
>                 dd      $$                              ;   p_vaddr
>                 dd      $$                              ;   p_paddr
>                 dd      filesize                        ;   p_filesz
>                 dd      filesize                        ;   p_memsz
>                 dd      5                               ;   p_flags
>                 dd      0x1000                          ;   p_align
>
>   phdrsize      equ     $ - phdr
>
>   _start:
>                 xor     bl, bl
>                 xor     eax, eax
>                 inc     eax
>                 int     0x80
>
>   filesize      equ     $ - $$
>
> ; Test program end
>
>
> Here's the patch I've created to implement the two additional, weak,
> sanity checks - patch against 2.6.1-rc1-mm2 :
>
>
> --- 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;
>
>
> Any and all comments are welcome - what do you think, should we have safer
> binary loading in 2.6.x?
>
>
> -- 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/
>
-
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/