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

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

[Jesper Juhl]

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

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

[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@dif.dk 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

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

[Andrew Morton]

Jesper Juhl <juhl@dif.dk> wrote:
>
> The current Linux kernel does only very basic sanity checking on ELF
>  binaries.

I've always had little confidence in the elf loader.  The problem is
complex, the code quality is not high and the consequences of an error are
severe.

I guess others realise this, and the bad guys have probably already
"audited" the code for us, but still.

I'll merge your additional checks for testing and would encourage you to
keep looking at the problem, thanks.

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

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Thu, 08 Jan 2004 19:20:21 PST, Andrew Morton said:
> I've always had little confidence in the elf loader.  The problem is
> complex, the code quality is not high and the consequences of an error are
> severe.

You might want to read this very interesting dissection of the ELF format
for fun and non-profit.
 
http://www.muppetlabs.com/~breadbox/software/tiny/teensy.html

All I can say is - although it's insanely creative, this is *NOT* how I
want my ELF loader to work. ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

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

[Jesper Juhl]

On Fri, 9 Jan 2004 Valdis.Kletnieks@vt.edu wrote:

> On Thu, 08 Jan 2004 19:20:21 PST, Andrew Morton said:
> > I've always had little confidence in the elf loader.  The problem is
> > complex, the code quality is not high and the consequences of an error
> are
> > severe.
>
> You might want to read this very interesting dissection of the ELF
> format
> for fun and non-profit.
>
> http://www.muppetlabs.com/~breadbox/software/tiny/teensy.html
>
> All I can say is - although it's insanely creative, this is *NOT* how I
> want my ELF loader to work. ;)
>
I know of the document, but thank you for pointing it out, it's quite an
interresting read. Actually, reading that exact document ages ago was what
initially caused me to start reading the ELF loading code (thinking
"there's got to be something wrong here").
I've actually been planning to use some of the crazy stunts he pulls
with that code as validity checks of the code I want to implement (in
adition to specially tailored test-cases ofcourse).


-- Jesper Juhl

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

[Maciej Zenczykowski]

> I know of the document, but thank you for pointing it out, it's quite an
> interresting read. Actually, reading that exact document ages ago was what
> initially caused me to start reading the ELF loading code (thinking
> "there's got to be something wrong here").
> I've actually been planning to use some of the crazy stunts he pulls
> with that code as validity checks of the code I want to implement (in
> adition to specially tailored test-cases ofcourse).

I think this points to an 'issue', if we're going to increase the checks
in the ELF-loader (and thus increase the size of the minimal valid ELF
file we can load, thus effectively 'bloating' (lol) some programs) we
should probably allow some sort of direct binary executable files [i.e.
header 'XBIN386\0' followed by Read/Execute binary code to execute by
mapping as RX at any offset and jumping to offset 8] to allow writing
minimal executables.  Minimalizing executables is useful for embedded
systems, portable devices, floppy distributions and ramdisk/initrd
situations.  Sure many of these solve this problem by UPX compressing
busybox/crunchbox one-file-many-executables files, but it would still be
nice to be able to dump all the extra crud in some cases.  Some of these
distributions already contain non-standards conforming ELF files. I have a
933 byte less and a 305 byte strings command on my initrd (taken from some
floppy distribution) which report "ERROR: Corrupted section header size"
via 'file *' and there is probably many many more out there.  Is this 
worth it?  I don't know, in many ways this would be the COM to the ELF 
EXE... the DOS analogy proves little though [note: I have no idea about 
the a.out or COFF or whatever it's called format].

Cheers,
MaZe.

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

[Christoph Hellwig]

On Fri, Jan 09, 2004 at 09:20:53PM +0100, Maciej Zenczykowski wrote:
> I think this points to an 'issue', if we're going to increase the checks
> in the ELF-loader (and thus increase the size of the minimal valid ELF
> file we can load, thus effectively 'bloating' (lol) some programs) we
> should probably allow some sort of direct binary executable files [i.e.
> header 'XBIN386\0' followed by Read/Execute binary code to execute by

Like binfmt_flat? :)

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

[Xavier Bestel]

Le ven 09/01/2004 à 21:41, Christoph Hellwig a écrit :
> On Fri, Jan 09, 2004 at 09:20:53PM +0100, Maciej Zenczykowski wrote:
> > I think this points to an 'issue', if we're going to increase the checks
> > in the ELF-loader (and thus increase the size of the minimal valid ELF
> > file we can load, thus effectively 'bloating' (lol) some programs) we
> > should probably allow some sort of direct binary executable files [i.e.
> > header 'XBIN386\0' followed by Read/Execute binary code to execute by
> 
> Like binfmt_flat? :)

.. or even zflat. Not that I'm proud of it, but it can effectively
manage to produce rather compact executables :)

	Xav

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

[Maciej Zenczykowski]

> > Like binfmt_flat? :)
> 
> .. or even zflat. Not that I'm proud of it, but it can effectively
> manage to produce rather compact executables :)

Probably one of those two, is this something new?  Never heard of 
either... :) Specs? Min bin file size?  If this (one of these) is 
guaranteed to be in any new kernel (i.e. can't be configed out like a.out) 
then that would be enough (assuming this flat is slim file size wise).

Cheers,
MaZe.

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

[Xavier Bestel]

Le sam 10/01/2004 à 03:27, Maciej Zenczykowski a écrit :
> > > Like binfmt_flat? :)
> > 
> > .. or even zflat. Not that I'm proud of it, but it can effectively
> > manage to produce rather compact executables :)
> 
> Probably one of those two, is this something new?  Never heard of 
> either... :) Specs? Min bin file size?  If this (one of these) is 
> guaranteed to be in any new kernel (i.e. can't be configed out like a.out) 
> then that would be enough (assuming this flat is slim file size wise).

It can be configured out, of course, and is generally not configured for
you regular desktop. It comes from the embedded world (uCLinux) where
space on primary storage (generally flash) is one of the big costs. The
tools to generate/manipulate it are in the uCLinux distro.

	Xav

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

[Jesper Juhl]

On Fri, 9 Jan 2004, Maciej Zenczykowski wrote:

> > I know of the document, but thank you for pointing it out, it's quite an
> > interresting read. Actually, reading that exact document ages ago was what
> > initially caused me to start reading the ELF loading code (thinking
> > "there's got to be something wrong here").
> > I've actually been planning to use some of the crazy stunts he pulls
> > with that code as validity checks of the code I want to implement (in
> > adition to specially tailored test-cases ofcourse).
>
> I think this points to an 'issue', if we're going to increase the checks
> in the ELF-loader (and thus increase the size of the minimal valid ELF
> file we can load, thus effectively 'bloating' (lol) some programs) we

Do you need smaller than this?  :

                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:

                mov     bl, 0
                xor     eax, eax
                inc     eax
                int     0x80

  filesize      equ     $ - $$


That's a 100% valid ELF executable, and the entire program is 91 bytes..
Sure, it doesn't do much useful, and the ELF header and program header
table is huge overhead compared to the actual program, but that overhead
is minimal in any program that does any actual work.

Also, I'm not planning to add anything that disallows anything the ELF
spec allows, so you can still pull funny tricks like have sections overlap
and in the above program put _start inside the unused padding bytes in
e_ident[EI_PAD] if you want.. still a valid program, and not something
that the checks I'm adding will prevent.

It you want *really* tiny files then, as some have suggested, anothe
format could be used.
In my oppinion, if you claim to be an ELF executable, then you should be a
*valid* ELF executable.. If you are not a valid elf file but claim to be
so, then either something corrupted you or the tools that generated you
are buggy - and you should not be allowed to even attempt to execute - for
all the reasons I gave in my original mail.


-- Jesper Juhl

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

[Maciej Zenczykowski]

> Do you need smaller than this?  :
...
> That's a 100% valid ELF executable, and the entire program is 91 bytes..
> Sure, it doesn't do much useful, and the ELF header and program header
> table is huge overhead compared to the actual program, but that overhead
> is minimal in any program that does any actual work.
> 
> Also, I'm not planning to add anything that disallows anything the ELF
> spec allows, so you can still pull funny tricks like have sections overlap
> and in the above program put _start inside the unused padding bytes in
> e_ident[EI_PAD] if you want.. still a valid program, and not something
> that the checks I'm adding will prevent.
> 
> It you want *really* tiny files then, as some have suggested, anothe
> format could be used.
> In my oppinion, if you claim to be an ELF executable, then you should be a
> *valid* ELF executable.. If you are not a valid elf file but claim to be
> so, then either something corrupted you or the tools that generated you
> are buggy - and you should not be allowed to even attempt to execute - for
> all the reasons I gave in my original mail.

OK, if that 91 is OK, then no problem, I was thinking the minimum would be 
around 1-2 KB (now that I think about it, not really sure why I assumed 
that).  I'm not mad enough to require/want shrinking from 90 to 45 
bytes :) especially since most useful programs have a little more meat to 
them than the 80 bytes worth of header :)

Cheers,
MaZe

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

[Jesper Juhl]

On Sat, 10 Jan 2004, Maciej Zenczykowski wrote:

> > Do you need smaller than this?  :
> ...
> > That's a 100% valid ELF executable, and the entire program is 91 bytes..
> > Sure, it doesn't do much useful, and the ELF header and program header
> > table is huge overhead compared to the actual program, but that overhead
> > is minimal in any program that does any actual work.
> >
> > Also, I'm not planning to add anything that disallows anything the ELF
> > spec allows, so you can still pull funny tricks like have sections overlap
> > and in the above program put _start inside the unused padding bytes in
> > e_ident[EI_PAD] if you want.. still a valid program, and not something
> > that the checks I'm adding will prevent.
> >
> ...
>
> OK, if that 91 is OK, then no problem, I was thinking the minimum would be
> around 1-2 KB (now that I think about it, not really sure why I assumed
> that).  I'm not mad enough to require/want shrinking from 90 to 45
> bytes :) especially since most useful programs have a little more meat to
> them than the 80 bytes worth of header :)
>

If you have any small programs that you worry about and/or some programs
that try to pull unusual (but valid) stunts, then I'd appreciate it if
you'd help test out the patches I'm creating and verify that they don't
cause any trouble - I already posted the first version of the patch to the
list today and more will follow.


-- Jesper Juhl

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

[Willy Tarreau]

On Fri, Jan 09, 2004 at 09:20:53PM +0100, Maciej Zenczykowski wrote:
> I have a 933 byte less and a 305 byte strings command on my initrd
> (taken from some floppy distribution) which report "ERROR: Corrupted
> section header size" via 'file *' and there is probably many many
> more out there.

Then upgrade to file-4.07 which fixes this bad identification. I was very
annoyed by this problem because I too sometimes generate rather tiny
executables, and my build scripts automatically mark them as executables
when 'file' tells me they are ELF executables. When I discovered it, it
was because the victim was init...

Cheers,
Willy

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

[Adrian Bunk]

On Fri, Jan 09, 2004 at 09:20:53PM +0100, Maciej Zenczykowski wrote:
> > I know of the document, but thank you for pointing it out, it's quite an
> > interresting read. Actually, reading that exact document ages ago was what
> > initially caused me to start reading the ELF loading code (thinking
> > "there's got to be something wrong here").
> > I've actually been planning to use some of the crazy stunts he pulls
> > with that code as validity checks of the code I want to implement (in
> > adition to specially tailored test-cases ofcourse).
> 
> I think this points to an 'issue', if we're going to increase the checks
> in the ELF-loader (and thus increase the size of the minimal valid ELF
> file we can load, thus effectively 'bloating' (lol) some programs) we
> should probably allow some sort of direct binary executable files [i.e.
> header 'XBIN386\0' followed by Read/Execute binary code to execute by
> mapping as RX at any offset and jumping to offset 8] to allow writing
> minimal executables.  Minimalizing executables is useful for embedded
> systems, portable devices, floppy distributions and ramdisk/initrd
> situations.  Sure many of these solve this problem by UPX compressing
> busybox/crunchbox one-file-many-executables files, but it would still be
> nice to be able to dump all the extra crud in some cases.  Some of these
> distributions already contain non-standards conforming ELF files. I have a
> 933 byte less and a 305 byte strings command on my initrd (taken from some
>...

The best non-standards conforming ELF program I know is e3 [1] - a
10 kB Editor that supports Emacs-, Vi-, Pico-, Nedit- and Wordstar-like 
key bindings. Additionally, it includes a numeric calculator.

> Cheers,
> MaZe.

cu
Adrian

[1] http://www.sax.de/~adlibit/

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

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

[Jesper Juhl]

On Thu, 8 Jan 2004, Andrew Morton wrote:

> Jesper Juhl <juhl@dif.dk> wrote:
> >
> > The current Linux kernel does only very basic sanity checking on ELF
> >  binaries.
>
> I've always had little confidence in the elf loader.  The problem is
> complex, the code quality is not high and the consequences of an error are
> severe.
>
Ahh, so I'm not crazy ;)  I've been looking at that code trying to
convince myself that I should try and deal with it for quite a while.


> I guess others realise this, and the bad guys have probably already
> "audited" the code for us, but still.
>
> I'll merge your additional checks for testing and would encourage you to
> keep looking at the problem, thanks.
>

Thank you. I'll keep working on this. I'll see if I can get a patch done over
the weekend that adds a few more checks and re-do the ones you just merged
to be stronger - it may take longer as I probably won't have too much time
the next 2-3 days, but I'll se what I can do.


-- Jesper Juhl

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

[Anton Blanchard]

> I've always had little confidence in the elf loader.  The problem is
> complex, the code quality is not high and the consequences of an error are
> severe.

One thing I noticed is that we only obey execute permission on load
sections. On ppc32 the PLT is in the bss area and must be executable:

[27] .sbss             PROGBITS        100ba10c 0aa10c 000a14 00  WA 0   0  8
[28] .plt              PROGBITS        100bab20 0aab20 000834 00 WAX 0   0  4  
[29] .bss              NOBITS          100bb358 0ab354 003f90 00  WA 0   0  8

When I did per page execute for ppc64 we fell apart because the current
elf loader just creates a single region of non executable memory
regardless of what the binary asks for.

Anton

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

[Jakub Jelinek]

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

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

[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

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

[Mike Fedyk]

On Fri, Jan 09, 2004 at 11:50:53AM +0100, Jesper Juhl wrote:
> 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.

There are some patches for the elf loader from one of the big names in LKML,
but I forgot who it was.  Maybe a search through google will bring something
up...

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

[Jesper Juhl]

On Fri, 9 Jan 2004, Mike Fedyk wrote:

> On Fri, Jan 09, 2004 at 11:50:53AM +0100, Jesper Juhl wrote:
> > 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.
>
> There are some patches for the elf loader from one of the big names in LKML,
> but I forgot who it was.  Maybe a search through google will bring something
> up...
>
I have been digging for patches, and I've found some that clean up various
bits in various archs, and I'll try to take all that into account in my
own modifications.


-- Jesper Juhl