[Fedora 09/19] binfmt_elf: Elf executable signature verification

[Josh Boyer]

On Wed, Sep 4, 2013 at 5:24 PM, Vivek Goyal <vgoyal at redhat.com> wrote:
> Do elf executable signature verification (if one is present). If signature
> is present, it should be valid. Validly signed executables are locked in
> memory and a flag cred->proc_signed gets set to signify this process
> executable contents are signed.
>
> If file is unsigned, it can execute but it does not have the cred->proc_signed
> set.
>
> Signed-off-by: Vivek Goyal <vgoyal at redhat.com>
> ---
>  fs/Kconfig.binfmt    | 13 +++++++++++++
>  fs/binfmt_elf.c      | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/cred.h |  2 ++
>  kernel/cred.c        |  2 ++
>  4 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 370b24c..08ce598 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -23,6 +23,19 @@ config BINFMT_ELF
>           ld.so (check the file <file:Documentation/Changes> for location and
>           latest version).
>
> +config BINFMT_ELF_SIG
> +       bool "ELF binary signature verification"
> +       depends on BINFMT_ELF
> +       select INTEGRITY
> +       select INTEGRITY_SIGNATURE
> +       select INTEGRITY_ASYMMETRIC_KEYS
> +       select IMA
> +       select IMA_APPRAISE
> +       select SYSTEM_TRUSTED_KEYRING
> +       default n
> +       ---help---
> +         Check ELF binary signature verfication.

Please don't do this.  Yes, it's technically viable to select all the
things you need, but this turns on entire subsystems we don't have
enabled.  In months when the maintainers have long forgotten about
this, we have to go figure out what turned on INTEGRITY and IMA
because they aren't explicitly set in the config-* fragments.  It's
really frustrating.

Instead, please make BINFMT_ELF_SIG depend on
INTEGRITY_ASYMMETRIC_KEYS and IMA_APPRAISE, then explicitly enable the
options you need in config-x86-generic.  Lump them together and
include a comment at the top about what piece of functionality needs
them.

> +
>  config COMPAT_BINFMT_ELF
>         bool
>         depends on COMPAT && BINFMT_ELF
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 100edcc..55aea2b 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -34,6 +34,8 @@
>  #include <linux/utsname.h>
>  #include <linux/coredump.h>
>  #include <linux/sched.h>
> +#include <linux/ima.h>
> +#include <keys/system_keyring.h>
>  #include <asm/uaccess.h>
>  #include <asm/param.h>
>  #include <asm/page.h>
> @@ -584,6 +586,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         int executable_stack = EXSTACK_DEFAULT;
>         unsigned long def_flags = 0;
>         struct pt_regs *regs = current_pt_regs();
> +       char *signature = NULL;
> +#ifdef CONFIG_BINFMT_ELF_SIG
> +       unsigned int siglen = 0;
> +       bool mlock_mappings = false;
> +#endif
>         struct {
>                 struct elfhdr elf_ex;
>                 struct elfhdr interp_elf_ex;
> @@ -725,6 +732,36 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         /* OK, This is the point of no return */
>         current->mm->def_flags = def_flags;
>
> +#ifdef CONFIG_BINFMT_ELF_SIG
> +       /* If executable is digitally signed. Lock down in memory */
> +       /* Get file signature, if any */
> +       retval = ima_file_signature_alloc(bprm->file, &signature);
> +
> +       /*
> +        * If there is an error getting signature, bail out. Having
> +        * no signature is fine though.
> +        */
> +       if (retval < 0 && retval != -ENODATA && retval != -EOPNOTSUPP)
> +               goto out_free_dentry;
> +
> +       if (signature != NULL) {
> +               siglen = retval;
> +               retval = ima_signature_type(signature);
> +               if (retval == EVM_IMA_XATTR_DIGSIG &&
> +                   ima_memlock_file(signature, siglen)) {
> +                       retval = ima_appraise_file_digsig(system_trusted_keyring,
> +                                       bprm->file, signature, siglen);
> +                       if (retval) {
> +                               send_sig(SIGKILL, current, 0);
> +                               goto out_free_dentry;
> +                       }
> +
> +                       mlock_mappings = true;
> +                       current->mm->def_flags |= VM_LOCKED;
> +                       set_bit(MMF_VM_LOCKED, &current->mm->flags);
> +               }
> +       }
> +#endif
>         /* Do this immediately, since STACK_TOP as used in setup_arg_pages
>            may depend on the personality.  */
>         SET_PERSONALITY(loc->elf_ex);
> @@ -895,6 +932,19 @@ static int load_elf_binary(struct linux_binprm *bprm)
>                 goto out_free_dentry;
>         }
>
> +#ifdef CONFIG_BINFMT_ELF_SIG
> +       if (mlock_mappings) {
> +               retval = ima_appraise_file_digsig(system_trusted_keyring,
> +                                       bprm->file, signature, siglen);
> +               if (retval) {
> +                       send_sig(SIGKILL, current, 0);
> +                       goto out_free_dentry;
> +               }
> +               /* Signature verification successful */
> +               bprm->cred->proc_signed = true;
> +       }
> +#endif

If I'm understanding this correctly, it reads the file signature and
marks the _process_ as signed, right?  Is there anything that prevents
a process marked in this way from exec'ing a new, unsigned, unverified
executable?

josh