[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLsrTZV_xDR_MPZdkT44P1MpbDiF3Kk2HNWBXKsdY+fzg@mail.gmail.com>
Date: Thu, 8 Jun 2017 19:38:51 -0700
From: Kees Cook <keescook@...omium.org>
To: Matt Brown <matt@...tt.com>
Cc: James Morris <james.l.morris@...cle.com>, "Serge E. Hallyn" <serge@...lyn.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-security-module <linux-security-module@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v2 1/1] Add Trusted Path Execution as a stackable LSM
On Wed, Jun 7, 2017 at 8:43 PM, Matt Brown <matt@...tt.com> wrote:
> This patch was modified from Brad Spengler's Trusted Path Execution (TPE)
> feature. It also adds features and config options that were found in Corey
> Henderson's tpe-lkm project.
>
> Modifications from Brad Spengler's implementation of TPE were made to
> turn it into a stackable LSM using the existing LSM hook bprm_set_creds.
> Also, a new denial logging function was used to simplify printing messages
> to the kernel log. Additionally, mmap and mprotect restrictions were
> taken from Corey Henderson's tpe-lkm project and implemented using the
> LSM hooks mmap_file and file_mprotect.
>
> Trusted Path Execution is not a new idea:
>
> http://phrack.org/issues/52/6.html#article
>
> | A trusted path is one that is inside a root owned directory that
> | is not group or world writable. /bin, /usr/bin, /usr/local/bin, are
> | (under normal circumstances) considered trusted. Any non-root
> | users home directory is not trusted, nor is /tmp.
>
> To be clear, Trusted Path Execution is no replacement for a MAC system
> like SELinux, SMACK, or AppArmor. This LSM is designed to be good enough
> without requiring a userland utility to configure policies. The fact
> that TPE only requires the user to turn on a few sysctl options lowers
> the barrier to implementing a security framework substantially.
>
> Threat Models:
>
> 1. Attacker on system executing exploit on system vulnerability
>
> * If attacker uses a binary as a part of their system exploit, TPE can
> frustrate their efforts
>
> * This protection can be more effective when an attacker does not yet
> have an interactive shell on a system
>
> * Issues:
> * Can be bypassed by interpreted languages such as python. You can run
> malicious code by doing: python -c 'evil code'
What's the recommendation for people interested in using TPE but
having interpreters installed?
>
> 2. Attacker on system replaces binary used by a privileged user with a
> malicious one
>
> * This situation arises when the administrator of a system leaves a
> binary as world writable.
>
> * TPE is very effective against this threat model
>
> This Trusted Path Execution implementation introduces the following
> Kconfig options and sysctls. The Kconfig text and sysctl options are
> taken heavily from Brad Spengler's implementation. Some extra sysctl
> options were taken from Corey Henderson's implementation.
>
> CONFIG_SECURITY_TPE (sysctl=kernel.tpe.enabled)
>
> default: n
>
> This option enables Trusted Path Execution. TPE blocks *untrusted*
> users from executing files that meet the following conditions:
>
> * file is not in a root-owned directory
> * file is writable by a user other than root
>
> NOTE: By default root is not restricted by TPE.
>
> CONFIG_SECURITY_TPE_GID (sysctl=kernel.tpe.gid)
>
> default: 0
>
> This option defines a group id that, by default, is the trusted group.
> If a user is not trusted then it has the checks described in
> CONFIG_SECURITY_TPE applied. Otherwise, the user is trusted and the
> checks are not applied. You can disable the trusted gid option by
> setting it to 0. This makes all non-root users untrusted.
>
> CONFIG_SECURITY_TPE_STRICT (sysctl=kernel.tpe.strict)
>
> default: n
>
> This option applies another set of restrictions to all non-root users
> even if they are trusted. This only allows execution under the
> following conditions:
>
> * file is in a directory owned by the user that is not group or
> world-writable
> * file is in a directory owned by root and writable only by root
>
> CONFIG_SECURITY_TPE_RESTRICT_ROOT (sysctl=kernel.tpe.restrict_root)
>
> default: n
>
> This option applies all enabled TPE restrictions to root.
>
> CONFIG_SECURITY_TPE_INVERT_GID (sysctl=kernel.tpe.invert_gid)
>
> default: n
>
> This option reverses the trust logic of the gid option and makes
> kernel.tpe.gid into the untrusted group. This means that all other groups
> become trusted. This sysctl is helpful when you want TPE restrictions
> to be limited to a small subset of users.
>
> Signed-off-by: Matt Brown <matt@...tt.com>
> ---
> Documentation/security/tpe.txt | 59 +++++++++++
> MAINTAINERS | 5 +
> include/linux/lsm_hooks.h | 5 +
> security/Kconfig | 1 +
> security/Makefile | 2 +
> security/security.c | 1 +
> security/tpe/Kconfig | 64 ++++++++++++
> security/tpe/Makefile | 3 +
> security/tpe/tpe_lsm.c | 218 +++++++++++++++++++++++++++++++++++++++++
> 9 files changed, 358 insertions(+)
> create mode 100644 Documentation/security/tpe.txt
> create mode 100644 security/tpe/Kconfig
> create mode 100644 security/tpe/Makefile
> create mode 100644 security/tpe/tpe_lsm.c
>
> diff --git a/Documentation/security/tpe.txt b/Documentation/security/tpe.txt
> new file mode 100644
> index 0000000..226afcc
> --- /dev/null
> +++ b/Documentation/security/tpe.txt
> @@ -0,0 +1,59 @@
> [...]
Yes, docs! I love it. :) One suggestion, though, all of the per-LSM
docs were moved in -next from .txt to .rst files, and had index
entries added, etc:
https://patchwork.kernel.org/patch/9725165/
Namely, move to Documentation/admin-guide/LSM/, add an entry to
index.rst and format it using ReST:
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
> diff --git a/security/tpe/Kconfig b/security/tpe/Kconfig
> new file mode 100644
> index 0000000..a1b8f17
> --- /dev/null
> +++ b/security/tpe/Kconfig
> @@ -0,0 +1,64 @@
> +config SECURITY_TPE
> + bool "Trusted Path Execution (TPE)"
> + default n
> + help
> + If you say Y here, you will be able to choose a gid to add to the
> + supplementary groups of users you want to mark as "trusted."
> + Untrusted users will not be able to execute any files that are not in
> + root-owned directories writable only by root. If the sysctl option
> + is enabled, a sysctl option with name "tpe.enabled" is created.
> +
> +config SECURITY_TPE_GID
> + int
> + default SECURITY_TPE_TRUSTED_GID if (SECURITY_TPE && !SECURITY_TPE_INVERT_GID)
> + default SECURITY_TPE_UNTRUSTED_GID if (SECURITY_TPE && SECURITY_TPE_INVERT_GID)
I think this should have "depends on SECURITY_TPE" (like all the others).
> [...]
> diff --git a/security/tpe/tpe_lsm.c b/security/tpe/tpe_lsm.c
> new file mode 100644
> index 0000000..fda811a
> --- /dev/null
> +++ b/security/tpe/tpe_lsm.c
> @@ -0,0 +1,218 @@
> +/*
> + * Trusted Path Execution Security Module
> + *
> + * Copyright (C) 2017 Matt Brown
> + * Copyright (C) 2001-2014 Bradley Spengler, Open Source Security, Inc.
> + * http://www.grsecurity.net spender@...ecurity.net
> + * Copyright (C) 2011 Corey Henderson
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/uidgid.h>
> +#include <linux/ratelimit.h>
> +#include <linux/limits.h>
> +#include <linux/cred.h>
> +#include <linux/slab.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/sysctl.h>
> +#include <linux/binfmts.h>
> +#include <linux/string_helpers.h>
> +#include <linux/dcache.h>
> +#include <uapi/asm-generic/mman-common.h>
> +
> +#define global_root(x) uid_eq((x), GLOBAL_ROOT_UID)
> +#define global_nonroot(x) (!uid_eq((x), GLOBAL_ROOT_UID))
> +#define global_root_gid(x) (gid_eq((x), GLOBAL_ROOT_GID))
> +#define global_nonroot_gid(x) (!gid_eq((x), GLOBAL_ROOT_GID))
> +
> +static int tpe_enabled __read_mostly = IS_ENABLED(CONFIG_SECURITY_TPE);
> +static kgid_t tpe_gid __read_mostly = KGIDT_INIT(CONFIG_SECURITY_TPE_GID);
> +static int tpe_invert_gid __read_mostly =
> + IS_ENABLED(CONFIG_SECURITY_TPE_INVERT_GID);
> +static int tpe_strict __read_mostly = IS_ENABLED(CONFIG_SECURITY_TPE_STRICT);
> +static int tpe_restrict_root __read_mostly =
> + IS_ENABLED(CONFIG_SECURITY_TPE_RESTRICT_ROOT);
> +
> +int print_tpe_error(struct file *file, char *reason1, char *reason2,
> + char *method)
I think these char * can all be const char *.
> +{
> + char *filepath;
> +
> + filepath = kstrdup_quotable_file(file, GFP_KERNEL);
> +
> + if (!filepath)
> + return -ENOMEM;
> +
> + pr_warn_ratelimited("TPE: Denied %s of %s Reason: %s%s%s\n", method,
> + (IS_ERR(filepath) ? "failed fetching file path" : filepath),
> + reason1, reason2 ? " and " : "", reason2 ?: "");
> + kfree(filepath);
> + return -EPERM;
> +}
> +
> +static int tpe_check(struct file *file, char *method)
This char * can be const char *.
> +{
> + struct inode *inode;
> + struct inode *file_inode;
> + struct dentry *dir;
> + const struct cred *cred = current_cred();
> + char *reason1 = NULL;
> + char *reason2 = NULL;
Perhaps check file argument for NULL here instead of each caller?
> +
> + dir = dget_parent(file->f_path.dentry);
> + inode = d_backing_inode(dir);
> + file_inode = d_backing_inode(file->f_path.dentry);
> +
> + if (!tpe_enabled)
> + return 0;
> +
> + /* never restrict root unless restrict_root sysctl is 1*/
> + if (global_root(cred->uid) && !tpe_restrict_root)
> + return 0;
> +
> + if (!tpe_strict)
> + goto general_tpe_check;
This kind of use of goto tells me the code sections need to be
separate functions. i.e. tpe_check_strict() for the bit below before
general_tpe_check:
> +
> + /* TPE_STRICT: restrictions enforced even if the gid is trusted */
> + if (global_nonroot(inode->i_uid) && !uid_eq(inode->i_uid, cred->uid))
> + reason1 = "directory not owned by user";
> + else if (inode->i_mode & 0002)
> + reason1 = "file in world-writable directory";
> + else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> + reason1 = "file in group-writable directory";
> + else if (file_inode->i_mode & 0002)
> + reason1 = "file is world-writable";
> +
> + if (reason1)
> + goto end;
struct tpe_rationale {
const char *reason1;
const char *reason2;
};
bool tpe_check_strict(...)
{
if (!tpe_strict);
return false;
...
return rationale->reason1 != NULL;
}
static int tpe_check(...)
{
...
struct tpe_rationale rationale= { };
if (tpe_check_strict(inode, cred, &rationale))
goto end;
...
> +general_tpe_check:
> + /* determine if group is trusted */
> + if (global_root_gid(tpe_gid))
> + goto next_check;
> + if (!tpe_invert_gid && !in_group_p(tpe_gid))
> + reason2 = "not in trusted group";
> + else if (tpe_invert_gid && in_group_p(tpe_gid))
> + reason2 = "in untrusted group";
> + else
> + return 0;
(This return 0 currently leaks the dget that is put at the end label.
Ah, yes, already pointed out I see now in later reply in thread.)
if (tpe_check_general(inode, cred, &rationale))
goto end;
bool tpe_check_general(...)
{
if (!global_root_gid(tpe_gid)) {
rationale->reason1 = NULL;
return true;
}
...
}
> +
> +next_check:
> + /* main TPE checks */
> + if (global_nonroot(inode->i_uid))
> + reason1 = "file in non-root-owned directory";
> + else if (inode->i_mode & 0002)
> + reason1 = "file in world-writable directory";
> + else if ((inode->i_mode & 0020) && global_nonroot_gid(inode->i_gid))
> + reason1 = "file in group-writable directory";
> + else if (file_inode->i_mode & 0002)
> + reason1 = "file is world-writable";
tpe_check_main(inode, cred, &rationale);
> +
> +end:
> + dput(dir);
> + if (reason1)
> + return print_tpe_error(file, reason1, reason2, method);
> + else
> + return 0;
And the end part above stays.
> +}
> +
> +int tpe_mmap_file(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags)
> +{
> + if (!file || !(prot & PROT_EXEC))
> + return 0;
> +
> + return tpe_check(file, "mmap");
> +}
> +
> +int tpe_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> + unsigned long prot)
> +{
> + if (!vma->vm_file)
No examination of reqprot vs prot here?
> + return 0;
> + return tpe_check(vma->vm_file, "mprotect");
> +}
> +
> +static int tpe_bprm_set_creds(struct linux_binprm *bprm)
> +{
> + if (!bprm->file)
> + return 0;
> + return tpe_check(bprm->file, "exec");
> +
> +}
> +
> +static struct security_hook_list tpe_hooks[] = {
> + LSM_HOOK_INIT(mmap_file, tpe_mmap_file),
> + LSM_HOOK_INIT(file_mprotect, tpe_file_mprotect),
> + LSM_HOOK_INIT(bprm_set_creds, tpe_bprm_set_creds),
> +};
> +
> +#ifdef CONFIG_SYSCTL
> +struct ctl_path tpe_sysctl_path[] = {
> + { .procname = "kernel", },
> + { .procname = "tpe", },
> + { }
> +};
> +
> +static struct ctl_table tpe_sysctl_table[] = {
> + {
> + .procname = "enabled",
> + .data = &tpe_enabled,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "gid",
> + .data = &tpe_gid,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "invert_gid",
> + .data = &tpe_invert_gid,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "strict",
> + .data = &tpe_strict,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "restrict_root",
> + .data = &tpe_restrict_root,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec,
> + },
> + { }
> +};
> +static void __init tpe_init_sysctl(void)
> +{
> + if (!register_sysctl_paths(tpe_sysctl_path, tpe_sysctl_table))
> + panic("TPE: sysctl registration failed.\n");
> +}
> +#else
> +static inline void tpe_init_sysctl(void) { }
> +#endif /* CONFIG_SYSCTL */
> +
> +void __init tpe_add_hooks(void)
> +{
> + pr_info("TPE: securing systems like it's 1998\n");
:)
> + security_add_hooks(tpe_hooks, ARRAY_SIZE(tpe_hooks), "tpe");
> + tpe_init_sysctl();
> +}
> --
> 2.10.2
>
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
