diff mbox series

[v5,5/5] sidechannel: Linux Security Module for sidechannel

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

Commit Message

Schaufler, Casey Sept. 26, 2018, 8:34 p.m. UTC
From: Casey Schaufler <casey@schaufler-ca.com>

This is a new Linux Security Module (LSM) that checks for
potential sidechannel issues that are not covered in the
ptrace PTRACE_MODE_SCHED option. Namespace differences are
checked in this intitial version. Additional checks should
be added when they are determined to be useful.

Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
 include/linux/lsm_hooks.h          |  5 ++
 security/Kconfig                   |  1 +
 security/Makefile                  |  2 +
 security/security.c                |  1 +
 security/sidechannel/Kconfig       | 13 +++++
 security/sidechannel/Makefile      |  1 +
 security/sidechannel/sidechannel.c | 88 ++++++++++++++++++++++++++++++
 7 files changed, 111 insertions(+)
 create mode 100644 security/sidechannel/Kconfig
 create mode 100644 security/sidechannel/Makefile
 create mode 100644 security/sidechannel/sidechannel.c

Comments

James Morris Sept. 27, 2018, 9:45 p.m. UTC | #1
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.
Casey Schaufler Sept. 27, 2018, 10:39 p.m. UTC | #2
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.
James Morris Sept. 27, 2018, 10:47 p.m. UTC | #3
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?
Schaufler, Casey Sept. 27, 2018, 11:19 p.m. UTC | #4
> -----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.
James Morris Sept. 27, 2018, 11:43 p.m. UTC | #5
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.
Jann Horn Sept. 27, 2018, 11:47 p.m. UTC | #6
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().
James Morris Sept. 28, 2018, 4:33 p.m. UTC | #7
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.
Schaufler, Casey Sept. 28, 2018, 5:40 p.m. UTC | #8
> -----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 mbox series

Patch

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");
+}