Message ID | 20180926203446.2004-6-casey.schaufler@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | LSM: Support ptrace sidechannel access checks | expand |
On Wed, 26 Sep 2018, Casey Schaufler wrote: > + /* > + * Namespace checks. Considered safe if: > + * cgroup namespace is the same > + * User namespace is the same > + * PID namespace is the same > + */ > + if (current->nsproxy) > + ccgn = current->nsproxy->cgroup_ns; > + if (p->nsproxy) > + pcgn = p->nsproxy->cgroup_ns; > + if (ccgn != pcgn) > + return -EACCES; > + if (current->cred->user_ns != p->cred->user_ns) > + return -EACCES; > + if (task_active_pid_ns(current) != task_active_pid_ns(p)) > + return -EACCES; > + return 0; I really don't like the idea of hard-coding namespace security semantics in an LSM. Also, I'm not sure if these semantics make any sense. It least make it user configurable.
On 9/27/2018 2:45 PM, James Morris wrote: > On Wed, 26 Sep 2018, Casey Schaufler wrote: > >> + /* >> + * Namespace checks. Considered safe if: >> + * cgroup namespace is the same >> + * User namespace is the same >> + * PID namespace is the same >> + */ >> + if (current->nsproxy) >> + ccgn = current->nsproxy->cgroup_ns; >> + if (p->nsproxy) >> + pcgn = p->nsproxy->cgroup_ns; >> + if (ccgn != pcgn) >> + return -EACCES; >> + if (current->cred->user_ns != p->cred->user_ns) >> + return -EACCES; >> + if (task_active_pid_ns(current) != task_active_pid_ns(p)) >> + return -EACCES; >> + return 0; > I really don't like the idea of hard-coding namespace security semantics > in an LSM. Also, I'm not sure if these semantics make any sense. Checks on namespaces where explicitly requested. I think these are the most sensible, but I'm willing to be educated. I was also requested to check on potential issues between containers, but as there is no kernel concept of containers this is the best I see we can do. > It least make it user configurable. Would you have a suggested granularity? I could have a configuration option for each of cgroups, user and pid namespaces but that's getting to be a lot of knobs to twist.
On Thu, 27 Sep 2018, Casey Schaufler wrote: > On 9/27/2018 2:45 PM, James Morris wrote: > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > >> + /* > >> + * Namespace checks. Considered safe if: > >> + * cgroup namespace is the same > >> + * User namespace is the same > >> + * PID namespace is the same > >> + */ > >> + if (current->nsproxy) > >> + ccgn = current->nsproxy->cgroup_ns; > >> + if (p->nsproxy) > >> + pcgn = p->nsproxy->cgroup_ns; > >> + if (ccgn != pcgn) > >> + return -EACCES; > >> + if (current->cred->user_ns != p->cred->user_ns) > >> + return -EACCES; > >> + if (task_active_pid_ns(current) != task_active_pid_ns(p)) > >> + return -EACCES; > >> + return 0; > > I really don't like the idea of hard-coding namespace security semantics > > in an LSM. Also, I'm not sure if these semantics make any sense. > > Checks on namespaces where explicitly requested. By whom and what is the rationale?
> -----Original Message----- > From: James Morris [mailto:jmorris@namei.org] > Sent: Thursday, September 27, 2018 3:47 PM > To: Casey Schaufler <casey@schaufler-ca.com> > Cc: Schaufler, Casey <casey.schaufler@intel.com>; kristen@linux.intel.com; > kernel-hardening@lists.openwall.com; Dock, Deneen T > <deneen.t.dock@intel.com>; linux-kernel@vger.kernel.org; Hansen, Dave > <dave.hansen@intel.com>; linux-security-module@vger.kernel.org; > selinux@tycho.nsa.gov; arjan@linux.intel.com > Subject: Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel > > On Thu, 27 Sep 2018, Casey Schaufler wrote: > > > On 9/27/2018 2:45 PM, James Morris wrote: > > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > > > >> + /* > > >> + * Namespace checks. Considered safe if: > > >> + * cgroup namespace is the same > > >> + * User namespace is the same > > >> + * PID namespace is the same > > >> + */ > > >> + if (current->nsproxy) > > >> + ccgn = current->nsproxy->cgroup_ns; > > >> + if (p->nsproxy) > > >> + pcgn = p->nsproxy->cgroup_ns; > > >> + if (ccgn != pcgn) > > >> + return -EACCES; > > >> + if (current->cred->user_ns != p->cred->user_ns) > > >> + return -EACCES; > > >> + if (task_active_pid_ns(current) != task_active_pid_ns(p)) > > >> + return -EACCES; > > >> + return 0; > > > I really don't like the idea of hard-coding namespace security semantics > > > in an LSM. Also, I'm not sure if these semantics make any sense. > > > > Checks on namespaces where explicitly requested. > > By whom and what is the rationale? The rationale is to protect containers. Since those closest thing there is to a definition of containers is "uses namespaces" that becomes the focus. Separating them out does not make too much sense as I would expect someone concerned with one to be concerned with all.
On Thu, 27 Sep 2018, Schaufler, Casey wrote: > > > On 9/27/2018 2:45 PM, James Morris wrote: > > > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > > > > > >> + /* > > > >> + * Namespace checks. Considered safe if: > > > >> + * cgroup namespace is the same > > > >> + * User namespace is the same > > > >> + * PID namespace is the same > > > >> + */ > > > >> + if (current->nsproxy) > > > >> + ccgn = current->nsproxy->cgroup_ns; > > > >> + if (p->nsproxy) > > > >> + pcgn = p->nsproxy->cgroup_ns; > > > >> + if (ccgn != pcgn) > > > >> + return -EACCES; > > > >> + if (current->cred->user_ns != p->cred->user_ns) > > > >> + return -EACCES; > > > >> + if (task_active_pid_ns(current) != task_active_pid_ns(p)) > > > >> + return -EACCES; > > > >> + return 0; > > > > I really don't like the idea of hard-coding namespace security semantics > > > > in an LSM. Also, I'm not sure if these semantics make any sense. > > > > > > Checks on namespaces where explicitly requested. > > > > By whom and what is the rationale? > > The rationale is to protect containers. Since those closest thing > there is to a definition of containers is "uses namespaces" that > becomes the focus. Separating them out does not make too much > sense as I would expect someone concerned with one to be concerned > with all. A lot of people will not be using user namespaces due to security concerns, so with this hard-coded logic, you are saying this case is 'safe' in a sidechannel context. Which hints at the deeper issue that containers are a userland abstraction. Protection of containers needs to be defined by userland policy.
On Fri, Sep 28, 2018 at 1:43 AM James Morris <jmorris@namei.org> wrote: > On Thu, 27 Sep 2018, Schaufler, Casey wrote: > > > > On 9/27/2018 2:45 PM, James Morris wrote: > > > > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > > > > > > > >> + /* > > > > >> + * Namespace checks. Considered safe if: > > > > >> + * cgroup namespace is the same > > > > >> + * User namespace is the same > > > > >> + * PID namespace is the same > > > > >> + */ > > > > >> + if (current->nsproxy) > > > > >> + ccgn = current->nsproxy->cgroup_ns; > > > > >> + if (p->nsproxy) > > > > >> + pcgn = p->nsproxy->cgroup_ns; > > > > >> + if (ccgn != pcgn) > > > > >> + return -EACCES; > > > > >> + if (current->cred->user_ns != p->cred->user_ns) > > > > >> + return -EACCES; > > > > >> + if (task_active_pid_ns(current) != task_active_pid_ns(p)) > > > > >> + return -EACCES; > > > > >> + return 0; > > > > > I really don't like the idea of hard-coding namespace security semantics > > > > > in an LSM. Also, I'm not sure if these semantics make any sense. > > > > > > > > Checks on namespaces where explicitly requested. > > > > > > By whom and what is the rationale? > > > > The rationale is to protect containers. Since those closest thing > > there is to a definition of containers is "uses namespaces" that > > becomes the focus. Separating them out does not make too much > > sense as I would expect someone concerned with one to be concerned > > with all. > > A lot of people will not be using user namespaces due to security > concerns, Ugh. > so with this hard-coded logic, you are saying this case is > 'safe' in a sidechannel context. > > Which hints at the deeper issue that containers are a userland > abstraction. Protection of containers needs to be defined by userland > policy. Or just compare mount namespaces additionally/instead. I think that containers will always use those, because AFAIK nobody uses chroot() for containers, given that the kernel makes absolutely no security guarantees about chroot().
On Fri, 28 Sep 2018, Jann Horn wrote: > > so with this hard-coded logic, you are saying this case is > > 'safe' in a sidechannel context. > > > > Which hints at the deeper issue that containers are a userland > > abstraction. Protection of containers needs to be defined by userland > > policy. > > Or just compare mount namespaces additionally/instead. I think that > containers will always use those, because AFAIK nobody uses chroot() > for containers, given that the kernel makes absolutely no security > guarantees about chroot(). We can't define this in the kernel. It has no concept of containers. People utilize some combination of namespaces and cgroups and call them containers, but we can't make assumptions from the kernel on what any of this means from a security point of view, and hard-code kernel policy based on those assumptions. This is violating the principal of separating mechanism and policy, and also imposing semantics across the kernel/user boundary. The latter creates an ABI which we can then never break.
> -----Original Message----- > From: James Morris [mailto:jmorris@namei.org] > Sent: Friday, September 28, 2018 9:33 AM > To: Jann Horn <jannh@google.com> > Cc: Schaufler, Casey <casey.schaufler@intel.com>; Casey Schaufler > <casey@schaufler-ca.com>; kristen@linux.intel.com; Kernel Hardening > <kernel-hardening@lists.openwall.com>; Dock, Deneen T > <deneen.t.dock@intel.com>; kernel list <linux-kernel@vger.kernel.org>; > Hansen, Dave <dave.hansen@intel.com>; linux-security-module <linux-security- > module@vger.kernel.org>; selinux@tycho.nsa.gov; Arjan van de Ven > <arjan@linux.intel.com> > Subject: Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel > > On Fri, 28 Sep 2018, Jann Horn wrote: > > > > so with this hard-coded logic, you are saying this case is > > > 'safe' in a sidechannel context. > > > > > > Which hints at the deeper issue that containers are a userland > > > abstraction. Protection of containers needs to be defined by userland > > > policy. > > > > Or just compare mount namespaces additionally/instead. I think that > > containers will always use those, because AFAIK nobody uses chroot() > > for containers, given that the kernel makes absolutely no security > > guarantees about chroot(). > > We can't define this in the kernel. It has no concept of containers. > > People utilize some combination of namespaces and cgroups and call them > containers, There is an amazing variety of things called containers out there. I cite them as a use case, not a requirement. > but we can't make assumptions from the kernel on what any of > this means from a security point of view, and hard-code kernel policy > based on those assumptions. We can assume that namespaces are being used as a separation mechanism. That makes processes in different namespaces potentially vulnerable to side-channel attacks. That's true regardless of whether or not someone is using namespaces to implement containers. > This is violating the principal of separating mechanism and policy, and > also imposing semantics across the kernel/user boundary. The latter > creates an ABI which we can then never break. The effects of the sidechannel security module are not API visible. The potential impact is on performance. This implementation of PTRACE_MODE_SCHED does not change what happens, but may affect when it happens. It is intended to aid in optimizing the use of expensive anti-side-channel countermeasures.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 97a020c616ad..3cb6516dba3c 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -2081,5 +2081,10 @@ void __init loadpin_add_hooks(void); #else static inline void loadpin_add_hooks(void) { }; #endif +#ifdef CONFIG_SECURITY_SIDECHANNEL +void __init sidechannel_add_hooks(void); +#else +static inline void sidechannel_add_hooks(void) { }; +#endif #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/Kconfig b/security/Kconfig index d9aa521b5206..6b814a3f93ea 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -236,6 +236,7 @@ source security/tomoyo/Kconfig source security/apparmor/Kconfig source security/loadpin/Kconfig source security/yama/Kconfig +source security/sidechannel/Kconfig source security/integrity/Kconfig diff --git a/security/Makefile b/security/Makefile index 4d2d3782ddef..d0c9e1b227f9 100644 --- a/security/Makefile +++ b/security/Makefile @@ -10,6 +10,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor subdir-$(CONFIG_SECURITY_YAMA) += yama subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin +subdir-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel # always enable default capabilities obj-y += commoncap.o @@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/ obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/ obj-$(CONFIG_SECURITY_YAMA) += yama/ obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/ +obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel/ obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o # Object integrity file lists diff --git a/security/security.c b/security/security.c index 736e78da1ab9..2129b0e31d7b 100644 --- a/security/security.c +++ b/security/security.c @@ -83,6 +83,7 @@ int __init security_init(void) capability_add_hooks(); yama_add_hooks(); loadpin_add_hooks(); + sidechannel_add_hooks(); /* * Load all the remaining security modules. diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig new file mode 100644 index 000000000000..653033027415 --- /dev/null +++ b/security/sidechannel/Kconfig @@ -0,0 +1,13 @@ +config SECURITY_SIDECHANNEL + bool "Sidechannel attack safety extra checks" + depends on SECURITY + default n + help + Look for a variety of cases where a side-channel attack + could potentially be exploited. Instruct the switching + code to use the indirect_branch_prediction_barrier in + cases where the passed task and the current task may be + at risk. + + If you are unsure how to answer this question, answer N. + diff --git a/security/sidechannel/Makefile b/security/sidechannel/Makefile new file mode 100644 index 000000000000..f61d83f28035 --- /dev/null +++ b/security/sidechannel/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel.o diff --git a/security/sidechannel/sidechannel.c b/security/sidechannel/sidechannel.c new file mode 100644 index 000000000000..18a67d19c020 --- /dev/null +++ b/security/sidechannel/sidechannel.c @@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Side Channel Safety Security Module + * + * Copyright (C) 2018 Intel Corporation. + * + */ + +#define pr_fmt(fmt) "SideChannel: " fmt + +#include <linux/types.h> +#include <linux/lsm_hooks.h> +#include <linux/capability.h> +#include <linux/cred.h> +#include <linux/sched.h> +#include <linux/string_helpers.h> +#include <linux/nsproxy.h> +#include <linux/pid_namespace.h> +#include <linux/ptrace.h> + +#ifdef CONFIG_NAMESPACES +/** + * safe_by_namespace - Are task and current sidechannel safe? + * @p: task to check on + * + * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise. + */ +static int safe_by_namespace(struct task_struct *p) +{ + struct cgroup_namespace *ccgn = NULL; + struct cgroup_namespace *pcgn = NULL; + + /* + * Namespace checks. Considered safe if: + * cgroup namespace is the same + * User namespace is the same + * PID namespace is the same + */ + if (current->nsproxy) + ccgn = current->nsproxy->cgroup_ns; + if (p->nsproxy) + pcgn = p->nsproxy->cgroup_ns; + if (ccgn != pcgn) + return -EACCES; + if (current->cred->user_ns != p->cred->user_ns) + return -EACCES; + if (task_active_pid_ns(current) != task_active_pid_ns(p)) + return -EACCES; + return 0; +} +#else +static int safe_by_namespace(struct task_struct *p) +{ + return 0; +} +#endif + +/** + * sidechannel_ptrace_access_check - Are task and current sidechannel safe? + * @p: task to check on + * @mode: ptrace access mode + * + * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise. + */ +static int sidechannel_ptrace_access_check(struct task_struct *p, + unsigned int mode) +{ + int rc; + + if ((mode & PTRACE_MODE_SCHED) == 0) + return 0; + + rc = safe_by_namespace(p); + if (rc) + return rc; + return 0; +} + +static struct security_hook_list sidechannel_hooks[] __lsm_ro_after_init = { + LSM_HOOK_INIT(ptrace_access_check, sidechannel_ptrace_access_check), +}; + +void __init sidechannel_add_hooks(void) +{ + pr_info("Extra sidechannel checks enabled\n"); + security_add_hooks(sidechannel_hooks, ARRAY_SIZE(sidechannel_hooks), + "sidechannel"); +}