[PATCH v6 0/3] security: Yama LSM

[PATCH v6 0/3] security: Yama LSM

[Kees Cook]

As discussed at the Linux Security Summit, I'm resubmitting this
code. As an LSM, it has coherent policy around expanding specific DAC
behaviors. There is no need for it to be a full-blown MAC, since it is
not intended to be one, but rather to be a simplified expansion to DAC,
with system-wide knobs. See the specific patches for details...

Thanks,

-Kees

[PATCH 1/3] security: Yama LSM

[Kees Cook]

This adds the Yama Linux Security Module to collect several DAC security
improvements (symlink, hardlink, and ptrace restrictions) that have existed
in various forms over the years and have been carried outside the mainline
kernel by other Linux distributions like Openwall and grsecurity.

As discussed at the Linux Security Summit, I'm resending this patchset
since it has value to a few distros. It was observed that while Yama is
not a MAC, this isn't required for an LSM. It has a distinct policy,
one that enhances DAC security, and that will continue to be its role
as an LSM.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v6:
 - Fix interaction with overlayfs, thanks to Andy Whitcroft and Leann
   Ogasawara.
 - Clarify rationale for LSM.
 - Move documentation under security subdirectory.
v5:
 - resend, with ptrace relationship interface
v4:
 - drop accidentally included fs/exec.c chunk.
v3:
 - drop needless cap_ callbacks.
 - fix usage of get_task_comm.
 - drop CONFIG_ of sysctl defaults, as recommended by Andi Kleen.
 - require SYSCTL.
v2:
 - add rcu locking, thanks to Tetsuo Handa.
 - add Documentation/Yama.txt for summary of features.
---
 Documentation/security/Yama.txt |   91 ++++++++++++++
 security/Kconfig                |    6 +
 security/Makefile               |    2 +
 security/yama/Kconfig           |   14 ++
 security/yama/Makefile          |    3 +
 security/yama/yama_lsm.c        |  250 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 366 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/security/Yama.txt
 create mode 100644 security/yama/Kconfig
 create mode 100644 security/yama/Makefile
 create mode 100644 security/yama/yama_lsm.c

diff --git a/Documentation/security/Yama.txt b/Documentation/security/Yama.txt
new file mode 100644
index 0000000..1916742
--- /dev/null
+++ b/Documentation/security/Yama.txt
@@ -0,0 +1,91 @@
+Yama is a Linux Security Module that collects a number of system-wide DAC
+security protections that are not handled by the core kernel itself. To
+select it at boot time, specify "security=yama" (though this will disable
+any other LSM).
+
+Yama is controlled through sysctl in /proc/sys/kernel/yama:
+
+- protected_sticky_symlinks
+- protected_nonaccess_hardlinks
+- ptrace_scope
+
+==============================================================
+
+protected_sticky_symlinks:
+
+A long-standing class of security issues is the symlink-based
+time-of-check-time-of-use race, most commonly seen in world-writable
+directories like /tmp. The common method of exploitation of this flaw
+is to cross privilege boundaries when following a given symlink (i.e. a
+root process follows a symlink belonging to another user). For a likely
+incomplete list of hundreds of examples across the years, please see:
+http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp
+
+When set to "0", symlink following behavior is unrestricted.
+
+When set to "1" symlinks are permitted to be followed only when outside
+a sticky world-writable directory, or when the uid of the symlink and
+follower match, or when the directory owner matches the symlink's owner.
+
+This protection is based on the restrictions in Openwall and grsecurity.
+
+==============================================================
+
+protected_nonaccess_hardlinks:
+
+Hardlinks can be abused in a similar fashion to symlinks in sticky
+world-writable directories, but their weakness is not limited to
+just that scenario. For example, if /etc and /home are on the same
+partition, a regular user can create a hardlink to /etc/shadow in their
+home directory. While it retains the original owner and permissions,
+it is possible for privileged programs that are otherwise symlink-safe
+to mistakenly access the file through its hardlink. Additionally, a very
+minor untraceable quota-bypassing local denial of service is possible by
+an attacker exhausting disk space by filling a world-writable directory
+with hardlinks.
+
+When set to "0", hardlink creation behavior is unrestricted.
+
+When set to "1", hardlinks cannot be created to files that a given user
+would be unable to read and write originally, or are otherwise sensitive.
+
+This protection is based on the restrictions in Openwall and grsecurity.
+
+==============================================================
+
+ptrace_scope:
+
+As Linux grows in popularity, it will become a larger target for
+malware. One particularly troubling weakness of the Linux process
+interfaces is that a single user is able to examine the memory and
+running state of any of their processes. For example, if one application
+(e.g. Pidgin) was compromised, it would be possible for an attacker to
+attach to other running processes (e.g. Firefox, SSH sessions, GPG agent,
+etc) to extract additional credentials and continue to expand the scope
+of their attack without resorting to user-assisted phishing.
+
+This is not a theoretical problem. SSH session hijacking
+(http://www.storm.net.nz/projects/7) and arbitrary code injection
+(http://c-skills.blogspot.com/2007/05/injectso.html) attacks already
+exist and remain possible if ptrace is allowed to operate as before.
+Since ptrace is not commonly used by non-developers and non-admins, system
+builders should be allowed the option to disable this debugging system.
+
+For a solution, some applications use prctl(PR_SET_DUMPABLE, ...) to
+specifically disallow such ptrace attachment (e.g. ssh-agent), but many
+do not. A more general solution is to only allow ptrace directly from a
+parent to a child process (i.e. direct "gdb EXE" and "strace EXE" still
+work), or with CAP_SYS_PTRACE (i.e. "gdb --pid=PID", and "strace -p PID"
+still work as root).
+
+0 - classic ptrace permissions: a process can PTRACE_ATTACH to any other
+    process running under the same uid, as long as it is dumpable (i.e.
+    did not transition uids, start privileged, or have called
+    prctl(PR_SET_DUMPABLE...) already).
+
+1 - restricted ptrace: a process can only call PTRACE_ATTACH on its
+    descendants when the above classic criteria is also met.
+
+This protection is based on the restrictions in grsecurity.
+
+==============================================================
diff --git a/security/Kconfig b/security/Kconfig
index 51bd5a0..ccc61f8 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -187,6 +187,7 @@ source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
+source security/yama/Kconfig
 
 source security/integrity/Kconfig
 
@@ -196,6 +197,7 @@ choice
 	default DEFAULT_SECURITY_SMACK if SECURITY_SMACK
 	default DEFAULT_SECURITY_TOMOYO if SECURITY_TOMOYO
 	default DEFAULT_SECURITY_APPARMOR if SECURITY_APPARMOR
+	default DEFAULT_SECURITY_YAMA if SECURITY_YAMA
 	default DEFAULT_SECURITY_DAC
 
 	help
@@ -214,6 +216,9 @@ choice
 	config DEFAULT_SECURITY_APPARMOR
 		bool "AppArmor" if SECURITY_APPARMOR=y
 
+	config DEFAULT_SECURITY_YAMA
+		bool "Yama" if SECURITY_YAMA=y
+
 	config DEFAULT_SECURITY_DAC
 		bool "Unix Discretionary Access Controls"
 
@@ -225,6 +230,7 @@ config DEFAULT_SECURITY
 	default "smack" if DEFAULT_SECURITY_SMACK
 	default "tomoyo" if DEFAULT_SECURITY_TOMOYO
 	default "apparmor" if DEFAULT_SECURITY_APPARMOR
+	default "yama" if DEFAULT_SECURITY_YAMA
 	default "" if DEFAULT_SECURITY_DAC
 
 endmenu
diff --git a/security/Makefile b/security/Makefile
index a5e502f..c26c81e 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -7,6 +7,7 @@ subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 subdir-$(CONFIG_SECURITY_SMACK)		+= smack
 subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
+subdir-$(CONFIG_SECURITY_YAMA)		+= yama
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -21,6 +22,7 @@ obj-$(CONFIG_SECURITY_SMACK)		+= smack/built-in.o
 obj-$(CONFIG_AUDIT)			+= lsm_audit.o
 obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/built-in.o
 obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/built-in.o
+obj-$(CONFIG_SECURITY_YAMA)		+= yama/built-in.o
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/yama/Kconfig b/security/yama/Kconfig
new file mode 100644
index 0000000..1a5d1c1
--- /dev/null
+++ b/security/yama/Kconfig
@@ -0,0 +1,14 @@
+config SECURITY_YAMA
+	bool "Yama support"
+	depends on SECURITY
+	select SECURITYFS
+	select SECURITY_PATH
+	default n
+	help
+	  This selects Yama, which extends DAC support with additional
+	  system-wide security settings beyond regular Linux discretionary
+	  access controls. Currently available are symlink, hardlink, and
+	  ptrace scope restrictions. Further information can be found in
+	  Documentation/security/Yama.txt.
+
+	  If you are unsure how to answer this question, answer N.
diff --git a/security/yama/Makefile b/security/yama/Makefile
new file mode 100644
index 0000000..8b5e065
--- /dev/null
+++ b/security/yama/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_SECURITY_YAMA) := yama.o
+
+yama-y := yama_lsm.o
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
new file mode 100644
index 0000000..955e401
--- /dev/null
+++ b/security/yama/yama_lsm.c
@@ -0,0 +1,250 @@
+/*
+ * Yama Linux Security Module
+ *
+ * Author: Kees Cook <kees.cook@canonical.com>
+ *
+ * Copyright (C) 2010 Canonical, Ltd.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/security.h>
+#include <linux/sysctl.h>
+#include <linux/ptrace.h>
+#include <linux/ratelimit.h>
+
+static int ptrace_scope = 1;
+static int protected_sticky_symlinks = 1;
+static int protected_nonaccess_hardlinks = 1;
+
+/**
+ * yama_ptrace_access_check - validate PTRACE_ATTACH calls
+ * @child: child task pointer
+ * @mode: ptrace attach mode
+ *
+ * Returns 0 if following the ptrace is allowed, -ve on error.
+ */
+static int yama_ptrace_access_check(struct task_struct *child,
+				    unsigned int mode)
+{
+	int rc;
+
+	rc = cap_ptrace_access_check(child, mode);
+	if (rc != 0)
+		return rc;
+
+	/* require ptrace target be a child of ptracer on attach */
+	if (mode == PTRACE_MODE_ATTACH && ptrace_scope &&
+	    !capable(CAP_SYS_PTRACE)) {
+		struct task_struct *walker = child;
+
+		rcu_read_lock();
+		read_lock(&tasklist_lock);
+		while (walker->pid > 0) {
+			if (walker == current)
+				break;
+			walker = walker->real_parent;
+		}
+		if (walker->pid == 0)
+			rc = -EPERM;
+		read_unlock(&tasklist_lock);
+		rcu_read_unlock();
+	}
+
+	if (rc) {
+		char name[sizeof(current->comm)];
+		printk_ratelimited(KERN_INFO "ptrace of non-child"
+			" pid %d was attempted by: %s (pid %d)\n",
+			child->pid,
+			get_task_comm(name, current),
+			current->pid);
+	}
+
+	return rc;
+}
+
+/**
+ * yama_inode_follow_link - check for symlinks in sticky world-writeable dirs
+ * @dentry: The inode/dentry of the symlink
+ * @nameidata: The path data of the symlink
+ *
+ * In the case of the protected_sticky_symlinks sysctl being enabled,
+ * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
+ * in a sticky world-writable directory.  This is to protect privileged
+ * processes from failing races against path names that may change out
+ * from under them by way of other users creating malicious symlinks.
+ * It will permit symlinks to only be followed when outside a sticky
+ * world-writable directory, or when the uid of the symlink and follower
+ * match, or when the directory owner matches the symlink's owner.
+ *
+ * Returns 0 if following the symlink is allowed, -ve on error.
+ */
+static int yama_inode_follow_link(struct dentry *dentry,
+				  struct nameidata *nameidata)
+{
+	int rc = 0;
+	const struct inode *parent;
+	const struct inode *inode;
+	const struct cred *cred;
+
+	if (!protected_sticky_symlinks)
+		return 0;
+
+	/* if inode isn't a symlink, don't try to evaluate blocking it */
+	inode = dentry->d_inode;
+	if (!S_ISLNK(inode->i_mode))
+		return 0;
+
+	/* owner and follower match? */
+	cred = current_cred();
+	if (cred->fsuid == inode->i_uid)
+		return 0;
+
+	/* check parent directory mode and owner */
+	spin_lock(&dentry->d_lock);
+	parent = dentry->d_parent->d_inode;
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid) {
+		rc = -EACCES;
+	}
+	spin_unlock(&dentry->d_lock);
+
+	if (rc) {
+		char name[sizeof(current->comm)];
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky world-writable "
+			"directory by %s (fsuid %d != %d)\n",
+			get_task_comm(name, current),
+			cred->fsuid, inode->i_uid);
+	}
+
+	return rc;
+}
+
+static int yama_generic_permission(struct inode *inode, int mask)
+{
+	int retval;
+
+	if (inode->i_op->permission)
+		retval = inode->i_op->permission(inode, mask);
+	else
+		retval = generic_permission(inode, mask);
+	return retval;
+}
+
+/**
+ * yama_path_link - verify that hardlinking is allowed
+ * @old_dentry: the source inode/dentry to hardlink from
+ * @new_dir: target directory
+ * @new_dentry: the target inode/dentry to hardlink to
+ *
+ * Block hardlink when all of:
+ *  - fsuid does not match inode
+ *  - not CAP_FOWNER
+ *  - and at least one of:
+ *    - inode is not a regular file
+ *    - inode is setuid
+ *    - inode is setgid and group-exec
+ *    - access failure for read and write
+ *
+ * Returns 0 if successful, -ve on error.
+ */
+static int yama_path_link(struct dentry *old_dentry, struct path *new_dir,
+			  struct dentry *new_dentry)
+{
+	int rc = 0;
+	struct inode *inode = old_dentry->d_inode;
+	const int mode = inode->i_mode;
+	const struct cred *cred = current_cred();
+
+	if (!protected_nonaccess_hardlinks)
+		return 0;
+
+	if (cred->fsuid != inode->i_uid &&
+	    (!S_ISREG(mode) || (mode & S_ISUID) ||
+	     ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) ||
+	     (yama_generic_permission(inode, MAY_READ | MAY_WRITE))) &&
+	    !capable(CAP_FOWNER)) {
+		char name[sizeof(current->comm)];
+		printk_ratelimited(KERN_INFO "non-accessible hardlink"
+			" creation was attempted by: %s (fsuid %d)\n",
+			get_task_comm(name, current),
+			cred->fsuid);
+		rc = -EPERM;
+	}
+
+	return rc;
+}
+
+static struct security_operations yama_ops = {
+	.name =			"yama",
+
+	.ptrace_access_check =	yama_ptrace_access_check,
+	.inode_follow_link =	yama_inode_follow_link,
+	.path_link =		yama_path_link,
+};
+
+#ifdef CONFIG_SYSCTL
+static int zero;
+static int one = 1;
+
+struct ctl_path yama_sysctl_path[] = {
+	{ .procname = "kernel", },
+	{ .procname = "yama", },
+	{ }
+};
+
+static struct ctl_table yama_sysctl_table[] = {
+	{
+		.procname       = "protected_sticky_symlinks",
+		.data           = &protected_sticky_symlinks,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &one,
+	},
+	{
+		.procname       = "protected_nonaccess_hardlinks",
+		.data           = &protected_nonaccess_hardlinks,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &one,
+	},
+	{
+		.procname       = "ptrace_scope",
+		.data           = &ptrace_scope,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &one,
+	},
+	{ }
+};
+#endif /* CONFIG_SYSCTL */
+
+static __init int yama_init(void)
+{
+	if (!security_module_enable(&yama_ops))
+		return 0;
+
+	printk(KERN_INFO "Yama: becoming mindful.\n");
+
+	if (register_security(&yama_ops))
+		panic("Yama: kernel registration failed.\n");
+
+#ifdef CONFIG_SYSCTL
+	if (!register_sysctl_paths(yama_sysctl_path, yama_sysctl_table))
+		panic("Yama: sysctl registration failed.\n");
+#endif
+
+	return 0;
+}
+
+security_initcall(yama_init);
-- 
1.7.5.4

[PATCH 2/3] security: create task_free security callback

[Kees Cook]

The current LSM interface to cred_free is not sufficient for allowing
an LSM to track the life and death of a task. This patch adds the
task_free hook so that an LSM can clean up resources on task death.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/security.h |    9 +++++++++
 kernel/fork.c            |    1 +
 security/capability.c    |    4 ++++
 security/security.c      |    5 +++++
 4 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 19d8e04..256befc 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -650,6 +650,10 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	manual page for definitions of the @clone_flags.
  *	@clone_flags contains the flags indicating what should be shared.
  *	Return 0 if permission is granted.
+ * @task_free:
+ *	@task task being freed
+ *	Handle release of task-related resources. (Note that this can be called
+ *  from interrupt context.)
  * @cred_alloc_blank:
  *	@cred points to the credentials.
  *	@gfp indicates the atomicity of any memory allocations.
@@ -1500,6 +1504,7 @@ struct security_operations {
 	int (*dentry_open) (struct file *file, const struct cred *cred);
 
 	int (*task_create) (unsigned long clone_flags);
+	void (*task_free) (struct task_struct *task);
 	int (*cred_alloc_blank) (struct cred *cred, gfp_t gfp);
 	void (*cred_free) (struct cred *cred);
 	int (*cred_prepare)(struct cred *new, const struct cred *old,
@@ -1762,6 +1767,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
 int security_file_receive(struct file *file);
 int security_dentry_open(struct file *file, const struct cred *cred);
 int security_task_create(unsigned long clone_flags);
+void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
 int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
@@ -2273,6 +2279,9 @@ static inline int security_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static inline void security_task_free(struct task_struct *task)
+{ }
+
 static inline int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
 {
 	return 0;
diff --git a/kernel/fork.c b/kernel/fork.c
index 8e6b6f4..6371378 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -190,6 +190,7 @@ void __put_task_struct(struct task_struct *tsk)
 	WARN_ON(atomic_read(&tsk->usage));
 	WARN_ON(tsk == current);
 
+	security_task_free(tsk);
 	exit_creds(tsk);
 	delayacct_tsk_free(tsk);
 	put_signal_struct(tsk->signal);
diff --git a/security/capability.c b/security/capability.c
index 2984ea4..e4206b5 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -359,6 +359,9 @@ static int cap_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static void cap_task_free(struct task_struct *task)
+{ }
+
 static int cap_cred_alloc_blank(struct cred *cred, gfp_t gfp)
 {
 	return 0;
@@ -955,6 +958,7 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, file_receive);
 	set_to_cap_if_null(ops, dentry_open);
 	set_to_cap_if_null(ops, task_create);
+	set_to_cap_if_null(ops, task_free);
 	set_to_cap_if_null(ops, cred_alloc_blank);
 	set_to_cap_if_null(ops, cred_free);
 	set_to_cap_if_null(ops, cred_prepare);
diff --git a/security/security.c b/security/security.c
index 0c6cc69..49a7227 100644
--- a/security/security.c
+++ b/security/security.c
@@ -749,6 +749,11 @@ int security_task_create(unsigned long clone_flags)
 	return security_ops->task_create(clone_flags);
 }
 
+void security_task_free(struct task_struct *task)
+{
+	security_ops->task_free(task);
+}
+
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
 {
 	return security_ops->cred_alloc_blank(cred, gfp);
-- 
1.7.5.4

[PATCH 3/3] security: Yama: add ptrace relationship tracking interface

[Kees Cook]

Some application suites have external crash handlers that depend
on being able to use ptrace to generate crash reports (KDE, Wine,
Chromium, Firefox, etc). Since the inferior process has a defined
application-specific relationship with the debugger, allow the inferior
to express that relationship by declaring who can call PTRACE_ATTACH
against it. The inferior can use prctl() with PR_SET_PTRACER to allow a
specific PID and its descendants to perform the ptrace instead of only
a direct ancestor.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v5:
 - Use _bh spinlocks, thanks to Ming Lei.
v4:
 - make sure to use thread group leader when creating exceptions.
v3:
 - make sure to use thread group leader when searching for exceptions.
v2:
 - kmalloc, spinlock init, and doc typo corrections from Tetsuo Handa.
 - make sure to replace if possible on add, thanks to Eric Paris.
---
 Documentation/security/Yama.txt |   18 +++-
 include/linux/prctl.h           |    6 +
 security/yama/yama_lsm.c        |  240 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 243 insertions(+), 21 deletions(-)

diff --git a/Documentation/security/Yama.txt b/Documentation/security/Yama.txt
index 1916742..9a83aa5 100644
--- a/Documentation/security/Yama.txt
+++ b/Documentation/security/Yama.txt
@@ -78,14 +78,26 @@ parent to a child process (i.e. direct "gdb EXE" and "strace EXE" still
 work), or with CAP_SYS_PTRACE (i.e. "gdb --pid=PID", and "strace -p PID"
 still work as root).
 
+For software that has defined application-specific relationships
+between a debugging process and its inferior (crash handlers, etc),
+prctl(PR_SET_PTRACER, pid, ...) can be used. An inferior can declare which
+other process (and its descendents) are allowed to call PTRACE_ATTACH
+against it. For example, this is used by KDE, Chromium, and Firefox's
+crash handlers, and by Wine for allowing only Wine processes to ptrace
+each other.
+
 0 - classic ptrace permissions: a process can PTRACE_ATTACH to any other
     process running under the same uid, as long as it is dumpable (i.e.
     did not transition uids, start privileged, or have called
     prctl(PR_SET_DUMPABLE...) already).
 
-1 - restricted ptrace: a process can only call PTRACE_ATTACH on its
-    descendants when the above classic criteria is also met.
+1 - restricted ptrace: a process must have a predefined relationship
+    with the inferior it wants to call PTRACE_ATTACH on. By default,
+    this relationship is that of only its descendants when the above
+    classic criteria is also met. To change the relationship, an
+    inferior can call prctl(PR_SET_PTRACER, debugger, ...) to declare
+    an allowed debugger PID to call PTRACE_ATTACH on the inferior.
 
-This protection is based on the restrictions in grsecurity.
+The original children-only logic was based on the restrictions in grsecurity.
 
 ==============================================================
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index a3baeb2..da7837b 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -102,4 +102,10 @@
 
 #define PR_MCE_KILL_GET 34
 
+/*
+ * Set specific pid that is allowed to PTRACE the current task.
+ * A value of 0 mean "no process".
+ */
+#define PR_SET_PTRACER 0x59616d61
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 955e401..a92538c 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -14,15 +14,224 @@
 #include <linux/security.h>
 #include <linux/sysctl.h>
 #include <linux/ptrace.h>
+#include <linux/prctl.h>
 #include <linux/ratelimit.h>
 
 static int ptrace_scope = 1;
 static int protected_sticky_symlinks = 1;
 static int protected_nonaccess_hardlinks = 1;
 
+/* describe a PTRACE relationship for potential exception */
+struct ptrace_relation {
+	struct task_struct *tracer;
+	struct task_struct *tracee;
+	struct list_head node;
+};
+
+static LIST_HEAD(ptracer_relations);
+static DEFINE_SPINLOCK(ptracer_relations_lock);
+
+/**
+ * yama_ptracer_add - add/replace an exception for this tracer/tracee pair
+ * @tracer: the task_struct of the process doing the PTRACE
+ * @tracee: the task_struct of the process to be PTRACEd
+ *
+ * Returns 0 if relationship was added, -ve on error.
+ */
+static int yama_ptracer_add(struct task_struct *tracer,
+			    struct task_struct *tracee)
+{
+	int rc = 0;
+	struct ptrace_relation *added;
+	struct ptrace_relation *entry, *relation = NULL;
+
+	added = kmalloc(sizeof(*added), GFP_KERNEL);
+	spin_lock_bh(&ptracer_relations_lock);
+	list_for_each_entry(entry, &ptracer_relations, node)
+		if (entry->tracee == tracee) {
+			relation = entry;
+			break;
+		}
+	if (!relation) {
+		relation = added;
+		if (!relation) {
+			rc = -ENOMEM;
+			goto unlock_out;
+		}
+		relation->tracee = tracee;
+		list_add(&relation->node, &ptracer_relations);
+	}
+	relation->tracer = tracer;
+
+unlock_out:
+	spin_unlock_bh(&ptracer_relations_lock);
+	if (added && added != relation)
+		kfree(added);
+
+	return rc;
+}
+
+/**
+ * yama_ptracer_del - remove exceptions related to the given tasks
+ * @tracer: remove any relation where tracer task matches
+ * @tracee: remove any relation where tracee task matches
+ */
+static void yama_ptracer_del(struct task_struct *tracer,
+			     struct task_struct *tracee)
+{
+	struct ptrace_relation *relation;
+	struct list_head *list, *safe;
+
+	spin_lock_bh(&ptracer_relations_lock);
+	list_for_each_safe(list, safe, &ptracer_relations) {
+		relation = list_entry(list, struct ptrace_relation, node);
+		if (relation->tracee == tracee ||
+		    relation->tracer == tracer) {
+			list_del(&relation->node);
+			kfree(relation);
+		}
+	}
+	spin_unlock_bh(&ptracer_relations_lock);
+}
+
+/**
+ * yama_task_free - check for task_pid to remove from exception list
+ * @task: task being removed
+ */
+static void yama_task_free(struct task_struct *task)
+{
+	yama_ptracer_del(task, task);
+}
+
+/**
+ * yama_task_prctl - check for Yama-specific prctl operations
+ * @option: operation
+ * @arg2: argument
+ * @arg3: argument
+ * @arg4: argument
+ * @arg5: argument
+ *
+ * Return 0 on success, -ve on error.  -ENOSYS is returned when Yama
+ * does not handle the given option.
+ */
+static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+			   unsigned long arg4, unsigned long arg5)
+{
+	int rc;
+	struct task_struct *myself = current;
+
+	rc = cap_task_prctl(option, arg2, arg3, arg4, arg5);
+	if (rc != -ENOSYS)
+		return rc;
+
+	switch (option) {
+	case PR_SET_PTRACER:
+		rcu_read_lock();
+		if (!thread_group_leader(myself))
+			myself = myself->group_leader;
+		get_task_struct(myself);
+		rcu_read_unlock();
+
+		if (arg2 == 0) {
+			yama_ptracer_del(NULL, myself);
+			rc = 0;
+		} else {
+			struct task_struct *tracer;
+
+			rcu_read_lock();
+			tracer = find_task_by_vpid(arg2);
+			if (tracer)
+				get_task_struct(tracer);
+			else
+				rc = -EINVAL;
+			rcu_read_unlock();
+
+			if (tracer) {
+				rc = yama_ptracer_add(tracer, myself);
+				put_task_struct(tracer);
+			}
+		}
+
+		put_task_struct(myself);
+		break;
+	}
+
+	return rc;
+}
+
+/**
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+			      struct task_struct *child)
+{
+	int rc = 0;
+	struct task_struct *walker = child;
+
+	if (!parent || !child)
+		return 0;
+
+	rcu_read_lock();
+	read_lock(&tasklist_lock);
+	if (!thread_group_leader(parent))
+		parent = parent->group_leader;
+	while (walker->pid > 0) {
+		if (!thread_group_leader(walker))
+			walker = walker->group_leader;
+		if (walker == parent) {
+			rc = 1;
+			break;
+		}
+		walker = walker->real_parent;
+	}
+	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
+
+	return rc;
+}
+
+/**
+ * ptracer_exception_found - tracer registered as exception for this tracee
+ * @tracer: the task_struct of the process attempting PTRACE
+ * @tracee: the task_struct of the process to be PTRACEd
+ *
+ * Returns 1 if tracer has is ptracer exception ancestor for tracee.
+ */
+static int ptracer_exception_found(struct task_struct *tracer,
+				   struct task_struct *tracee)
+{
+	int rc = 0;
+	struct ptrace_relation *relation;
+	struct task_struct *parent = NULL;
+
+	spin_lock_bh(&ptracer_relations_lock);
+
+	rcu_read_lock();
+	read_lock(&tasklist_lock);
+	if (!thread_group_leader(tracee))
+		tracee = tracee->group_leader;
+	list_for_each_entry(relation, &ptracer_relations, node)
+		if (relation->tracee == tracee) {
+			parent = relation->tracer;
+			break;
+		}
+	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
+
+	if (task_is_descendant(parent, tracer))
+		rc = 1;
+	spin_unlock_bh(&ptracer_relations_lock);
+
+	return rc;
+}
+
 /**
  * yama_ptrace_access_check - validate PTRACE_ATTACH calls
- * @child: child task pointer
+ * @child: task that current task is attempting to PTRACE
  * @mode: ptrace attach mode
  *
  * Returns 0 if following the ptrace is allowed, -ve on error.
@@ -32,27 +241,20 @@ static int yama_ptrace_access_check(struct task_struct *child,
 {
 	int rc;
 
+	/* If standard caps disallows it, so does Yama.  We should
+	 * only tighten restrictions further.
+	 */
 	rc = cap_ptrace_access_check(child, mode);
-	if (rc != 0)
+	if (rc)
 		return rc;
 
 	/* require ptrace target be a child of ptracer on attach */
-	if (mode == PTRACE_MODE_ATTACH && ptrace_scope &&
-	    !capable(CAP_SYS_PTRACE)) {
-		struct task_struct *walker = child;
-
-		rcu_read_lock();
-		read_lock(&tasklist_lock);
-		while (walker->pid > 0) {
-			if (walker == current)
-				break;
-			walker = walker->real_parent;
-		}
-		if (walker->pid == 0)
-			rc = -EPERM;
-		read_unlock(&tasklist_lock);
-		rcu_read_unlock();
-	}
+	if (mode == PTRACE_MODE_ATTACH &&
+	    ptrace_scope &&
+	    !capable(CAP_SYS_PTRACE) &&
+	    !task_is_descendant(current, child) &&
+	    !ptracer_exception_found(current, child))
+		rc = -EPERM;
 
 	if (rc) {
 		char name[sizeof(current->comm)];
@@ -185,6 +387,8 @@ static struct security_operations yama_ops = {
 	.ptrace_access_check =	yama_ptrace_access_check,
 	.inode_follow_link =	yama_inode_follow_link,
 	.path_link =		yama_path_link,
+	.task_prctl =		yama_task_prctl,
+	.task_free =		yama_task_free,
 };
 
 #ifdef CONFIG_SYSCTL
-- 
1.7.5.4

Re: [PATCH 3/3] security: Yama: add ptrace relationship tracking interface

[Vasiliy Kulikov]

Hi Kees,

Thank you for working on this!  We'll use Yama in Openwall too :-)

On Wed, Oct 26, 2011 at 16:49 -0700, Kees Cook wrote:
> +	if (mode == PTRACE_MODE_ATTACH &&
> +	    ptrace_scope &&
> +	    !capable(CAP_SYS_PTRACE) &&
> +	    !task_is_descendant(current, child) &&
> +	    !ptracer_exception_found(current, child))
> +		rc = -EPERM;

capable() is better to put after all other tests as a failed capable()
might emit a false positive warning into logs or something.

Thanks,

-- 
Vasiliy

Re: [PATCH 3/3] security: Yama: add ptrace relationship tracking interface

[Solar Designer]

On Sat, Nov 19, 2011 at 08:30:58PM +0400, Vasiliy Kulikov wrote:
> On Wed, Oct 26, 2011 at 16:49 -0700, Kees Cook wrote:
> > +	if (mode == PTRACE_MODE_ATTACH &&
> > +	    ptrace_scope &&
> > +	    !capable(CAP_SYS_PTRACE) &&
> > +	    !task_is_descendant(current, child) &&
> > +	    !ptracer_exception_found(current, child))
> > +		rc = -EPERM;
> 
> capable() is better to put after all other tests

Right, but...

> as a failed capable()
> might emit a false positive warning into logs or something.

...primarily for another reason: a successful capable() sets
PF_SUPERPRIV, whereas the permission might have been granted without
capable() as well.  The PF_SUPERPRIV flag is visible via BSD process
accounting.

Alexander

Re: [PATCH 3/3] security: Yama: add ptrace relationship tracking interface

[Kees Cook]

On Sat, Nov 19, 2011 at 8:59 AM, Solar Designer <solar@openwall.com> wrote:
> On Sat, Nov 19, 2011 at 08:30:58PM +0400, Vasiliy Kulikov wrote:
>> On Wed, Oct 26, 2011 at 16:49 -0700, Kees Cook wrote:
>> > +   if (mode == PTRACE_MODE_ATTACH &&
>> > +       ptrace_scope &&
>> > +       !capable(CAP_SYS_PTRACE) &&
>> > +       !task_is_descendant(current, child) &&
>> > +       !ptracer_exception_found(current, child))
>> > +           rc = -EPERM;
>>
>> capable() is better to put after all other tests
>
> Right, but...
>
>> as a failed capable()
>> might emit a false positive warning into logs or something.
>
> ...primarily for another reason: a successful capable() sets
> PF_SUPERPRIV, whereas the permission might have been granted without
> capable() as well.  The PF_SUPERPRIV flag is visible via BSD process
> accounting.

Fair point, I'll move it. I had to go look up an earlier thread since
this was ringing a bell, but in there I was talking about moving it to
the end of the tests too. :)

https://lkml.org/lkml/2010/6/30/5

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v6 0/3] security: Yama LSM

[James Morris]

On Wed, 26 Oct 2011, Kees Cook wrote:

> As discussed at the Linux Security Summit, I'm resubmitting this
> code. As an LSM, it has coherent policy around expanding specific DAC
> behaviors. There is no need for it to be a full-blown MAC, since it is
> not intended to be one, but rather to be a simplified expansion to DAC,
> with system-wide knobs. See the specific patches for details...

Yes, there was a strong consensus (unanimous I believe) at the security 
summit on incorporating the Yama LSM as a container for generally useful 
miscellaneous security hardening features that don't belong elsewhere.

This means that LSM is no longer 'officially' only for access control 
mechanisms.

We probably need to flesh out criteria for inclusion in Yama and document 
it.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v6 0/3] security: Yama LSM

[Kees Cook]

On Tue, Nov 1, 2011 at 1:45 PM, James Morris <jmorris@namei.org> wrote:
> On Wed, 26 Oct 2011, Kees Cook wrote:
>
> > As discussed at the Linux Security Summit, I'm resubmitting this
> > code. As an LSM, it has coherent policy around expanding specific DAC
> > behaviors. There is no need for it to be a full-blown MAC, since it is
> > not intended to be one, but rather to be a simplified expansion to DAC,
> > with system-wide knobs. See the specific patches for details...
>
> Yes, there was a strong consensus (unanimous I believe) at the security
> summit on incorporating the Yama LSM as a container for generally useful
> miscellaneous security hardening features that don't belong elsewhere.
>
> This means that LSM is no longer 'officially' only for access control
> mechanisms.

That would be very welcome.

> We probably need to flesh out criteria for inclusion in Yama and document
> it.

What's the best way for me to help with that documentation?

Thanks,

-Kees

--
Kees Cook
ChromeOS Security

Re: [PATCH v6 0/3] security: Yama LSM

[James Morris]

On Tue, 1 Nov 2011, Kees Cook wrote:

> > We probably need to flesh out criteria for inclusion in Yama and document
> > it.
> 
> What's the best way for me to help with that documentation?

Perhaps post a basic document for review.  It doesn't have to be a novel.

One thing I'd like to see is making it clear that if a feature is nacked 
by a core maintainer, Yama is not a means to bypass that nack.  That 
maintainer will need to ack it as part of Yama, too.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v6 0/3] security: Yama LSM

[Kees Cook]

On Tue, Nov 1, 2011 at 2:37 PM, James Morris <jmorris@namei.org> wrote:
> On Tue, 1 Nov 2011, Kees Cook wrote:
>
>> > We probably need to flesh out criteria for inclusion in Yama and document
>> > it.
>>
>> What's the best way for me to help with that documentation?
>
> Perhaps post a basic document for review.  It doesn't have to be a novel.

I've sent this now. We can expand it further if needed.

> One thing I'd like to see is making it clear that if a feature is nacked
> by a core maintainer, Yama is not a means to bypass that nack.  That
> maintainer will need to ack it as part of Yama, too.

At the same time, Yama isn't intended to be an endless dumping ground
for all random things. It's intended to be a place to put DAC
extensions that make sense for general system hardening. I can clarify
that further in the Yama documentation, if that's not already clear.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v6 0/3] security: Yama LSM

[James Morris]

On Wed, 26 Oct 2011, Kees Cook wrote:

> As discussed at the Linux Security Summit, I'm resubmitting this
> code. As an LSM, it has coherent policy around expanding specific DAC
> behaviors. There is no need for it to be a full-blown MAC, since it is
> not intended to be one, but rather to be a simplified expansion to DAC,
> with system-wide knobs. See the specific patches for details...

In principle, Yama can be merged, however there are features with 
unresolved upstream naks:

- Handling symlinks in sticky directories
- The ptrace tracker

These still need to be resolved.




-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v6 0/3] security: Yama LSM

[Kees Cook]

On Thu, Nov 17, 2011 at 8:17 PM, James Morris <jmorris@namei.org> wrote:
> On Wed, 26 Oct 2011, Kees Cook wrote:
>
>> As discussed at the Linux Security Summit, I'm resubmitting this
>> code. As an LSM, it has coherent policy around expanding specific DAC
>> behaviors. There is no need for it to be a full-blown MAC, since it is
>> not intended to be one, but rather to be a simplified expansion to DAC,
>> with system-wide knobs. See the specific patches for details...
>
> In principle, Yama can be merged, however there are features with
> unresolved upstream naks:
>
> - Handling symlinks in sticky directories
> - The ptrace tracker
>
> These still need to be resolved.

Oleg, Roland,

Do you have any objection to my LSM performing ptrace restrictions?
It's entirely self-contained, and all the major upstream crash
handlers are already using the prctl() interface it uses to declare
ptrace attach relationships.

Thanks,

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v6 0/3] security: Yama LSM

[Roland McGrath]

> Do you have any objection to my LSM performing ptrace restrictions?
> It's entirely self-contained, and all the major upstream crash
> handlers are already using the prctl() interface it uses to declare
> ptrace attach relationships.

I don't have the context of what your LSM does.  But other LSMs apply their
own rules in security_ptrace().  That's what the hook is for.  I'm not sure
why we would object from the perspective of core ptrace functionality.
LSMs are LSMs.  Their behavior is between you and your users, as far as I
am concerned.  As experts on ptrace and aficionados of its users, we may
have thoughts on what constraints on ptrace users would find annoying.
But that doesn't mean we'd object per se to whatever bizarre constraints
users want to ask an LSM to put on them.


Thanks,
Roland

Re: [PATCH v6 0/3] security: Yama LSM

[Kees Cook]

On Fri, Nov 18, 2011 at 1:45 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> Do you have any objection to my LSM performing ptrace restrictions?
>> It's entirely self-contained, and all the major upstream crash
>> handlers are already using the prctl() interface it uses to declare
>> ptrace attach relationships.
>
> I don't have the context of what your LSM does.  But other LSMs apply their
> own rules in security_ptrace().  That's what the hook is for.  I'm not sure
> why we would object from the perspective of core ptrace functionality.
> LSMs are LSMs.  Their behavior is between you and your users, as far as I
> am concerned.  As experts on ptrace and aficionados of its users, we may
> have thoughts on what constraints on ptrace users would find annoying.
> But that doesn't mean we'd object per se to whatever bizarre constraints
> users want to ask an LSM to put on them.

Yup, that's precisely what Yama does as well. Sounds like you're not
naking an LSM that uses security_ptrace(), then.

Thanks!

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH v6 0/3] security: Yama LSM

[Kees Cook]

On Thu, Nov 17, 2011 at 8:17 PM, James Morris <jmorris@namei.org> wrote:
> On Wed, 26 Oct 2011, Kees Cook wrote:
>
>> As discussed at the Linux Security Summit, I'm resubmitting this
>> code. As an LSM, it has coherent policy around expanding specific DAC
>> behaviors. There is no need for it to be a full-blown MAC, since it is
>> not intended to be one, but rather to be a simplified expansion to DAC,
>> with system-wide knobs. See the specific patches for details...
>
> In principle, Yama can be merged, however there are features with
> unresolved upstream naks:
>
> - Handling symlinks in sticky directories

Also hardlink restrictions (disallow hardlinking to anything that a
user doesn't have read/write access to -- breaks no software, solves
actual vulnerabilities).

> - The ptrace tracker
>
> These still need to be resolved.

Hi Al,

I know you're not a fan of LSMs in general, but can you let Yama
extend DAC in these two cases (symlink and hardlink restrictions) for
the distros and admins that want these specific, decades-old,
well-established tweaks to how links work? Other LSMs handle policy
via these hooks, and Yama is just doing the same thing.

Thanks,

-Kees

-- 
Kees Cook
ChromeOS Security

[RFC] Make Yama pid_ns aware

[Vasiliy Kulikov]

Hi,

As Yama's sysctls are about defining a security policy for the system,
it is reasonable to define it per container in case of LXC containers
(or out-of-tree alternatives like OpenVZ).  In my opinion they belong
to pid namespace.  With per-pid_ns sysctls it is possible to create
multiple containers with different ptrace, /tmp, etc. policies.

As Yama is not merged yet, I post the patch in this thread.

The patch is straightforward:

1) all sysctl variables are moved from global vars to pid_namespace
fields.

2) each cloned pid ns gets the settings of the parent.

3) the variables of current pid ns are visible through sysctl interface.

proc_pid_dointvec_minmax() is stolen from its ipc_ns equivalent in
ipc/ipc_sysctl.c.

(The patch is not tested.)

diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 38d1032..46edaf8 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -30,6 +30,11 @@ struct pid_namespace {
 #ifdef CONFIG_BSD_PROCESS_ACCT
 	struct bsd_acct_struct *bacct;
 #endif
+#ifdef CONFIG_SECURITY_YAMA
+	int ptrace_scope;
+	int protected_sticky_symlinks;
+	int protected_nonaccess_hardlinks;
+#endif
 };
 
 extern struct pid_namespace init_pid_ns;
diff --git a/kernel/pid.c b/kernel/pid.c
index fa5f722..0cd8926 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -78,6 +78,11 @@ struct pid_namespace init_pid_ns = {
 	.last_pid = 0,
 	.level = 0,
 	.child_reaper = &init_task,
+#ifdef CONFIG_SECURITY_YAMA
+	.ptrace_scope = 1,
+	.protected_sticky_symlinks = 1,
+	.protected_nonaccess_hardlinks = 1,
+#endif
 };
 EXPORT_SYMBOL_GPL(init_pid_ns);
 
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index e9c9adc..73d47c4 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -101,6 +101,14 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_p
 	if (err)
 		goto out_put_parent_pid_ns;
 
+#ifdef CONFIG_SECURITY_YAMA
+	ns->ptrace_scope = parent_pid_ns->ptrace_scope;
+	ns->protected_sticky_symlinks =
+		parent_pid_ns->protected_sticky_symlinks;
+	ns->protected_nonaccess_hardlinks =
+		parent_pid_ns->protected_nonaccess_hardlinks;
+#endif
+
 	return ns;
 
 out_put_parent_pid_ns:
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index a92538c..cf99a8c 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -16,10 +16,7 @@
 #include <linux/ptrace.h>
 #include <linux/prctl.h>
 #include <linux/ratelimit.h>
-
-static int ptrace_scope = 1;
-static int protected_sticky_symlinks = 1;
-static int protected_nonaccess_hardlinks = 1;
+#include <linux/pid_namespace.h>
 
 /* describe a PTRACE relationship for potential exception */
 struct ptrace_relation {
@@ -250,7 +247,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
 
 	/* require ptrace target be a child of ptracer on attach */
 	if (mode == PTRACE_MODE_ATTACH &&
-	    ptrace_scope &&
+	    current->nsproxy->pid_ns->ptrace_scope &&
 	    !capable(CAP_SYS_PTRACE) &&
 	    !task_is_descendant(current, child) &&
 	    !ptracer_exception_found(current, child))
@@ -292,7 +289,7 @@ static int yama_inode_follow_link(struct dentry *dentry,
 	const struct inode *inode;
 	const struct cred *cred;
 
-	if (!protected_sticky_symlinks)
+	if (!current->nsproxy->pid_ns->protected_sticky_symlinks)
 		return 0;
 
 	/* if inode isn't a symlink, don't try to evaluate blocking it */
@@ -362,7 +359,7 @@ static int yama_path_link(struct dentry *old_dentry, struct path *new_dir,
 	const int mode = inode->i_mode;
 	const struct cred *cred = current_cred();
 
-	if (!protected_nonaccess_hardlinks)
+	if (!current->nsproxy->pid_ns->protected_nonaccess_hardlinks)
 		return 0;
 
 	if (cred->fsuid != inode->i_uid &&
@@ -395,6 +392,26 @@ static struct security_operations yama_ops = {
 static int zero;
 static int one = 1;
 
+static void *get_pid_data(ctl_table *table)
+{
+	char *which = table->data;
+	struct pid_namespace *pid_ns = current->nsproxy->pid_ns;
+	which = (which - (char *)&init_pid_ns) + (char *)pid_ns;
+	return which;
+}
+
+static int proc_pid_dointvec_minmax(ctl_table *table, int write,
+	void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table pid_table;
+
+	memcpy(&pid_table, table, sizeof(pid_table));
+	pid_table.data = get_pid_data(table);
+
+	return proc_dointvec_minmax(&pid_table, write, buffer, lenp, ppos);
+}
+
+
 struct ctl_path yama_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "yama", },
@@ -404,28 +421,28 @@ struct ctl_path yama_sysctl_path[] = {
 static struct ctl_table yama_sysctl_table[] = {
 	{
 		.procname       = "protected_sticky_symlinks",
-		.data           = &protected_sticky_symlinks,
+		.data           = &init_pid_ns.protected_sticky_symlinks,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
 	{
 		.procname       = "protected_nonaccess_hardlinks",
-		.data           = &protected_nonaccess_hardlinks,
+		.data           = &init_pid_ns.protected_nonaccess_hardlinks,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
 	{
 		.procname       = "ptrace_scope",
-		.data           = &ptrace_scope,
+		.data           = &init_pid_ns.ptrace_scope,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
---

Re: [RFC] Make Yama pid_ns aware

[Kees Cook]

On Mon, Nov 21, 2011 at 11:18 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> As Yama's sysctls are about defining a security policy for the system,
> it is reasonable to define it per container in case of LXC containers
> (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> to pid namespace.  With per-pid_ns sysctls it is possible to create
> multiple containers with different ptrace, /tmp, etc. policies.

Cool, this seems like a good idea. Thanks for the patch! If Serge (or
other folks thinking about per-pid_ns stuff) think this is a good
approach, I'm happy to include it in the next revision of the Yama
patchset.

> proc_pid_dointvec_minmax() is stolen from its ipc_ns equivalent in
> ipc/ipc_sysctl.c.

I wonder if this function should be moved into a common location, in
case other things will want to do similar things.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [RFC] Make Yama pid_ns aware

[Serge Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> Hi,
> 
> As Yama's sysctls are about defining a security policy for the system,
> it is reasonable to define it per container in case of LXC containers
> (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> to pid namespace.  With per-pid_ns sysctls it is possible to create
> multiple containers with different ptrace, /tmp, etc. policies.

tying the ptrace policy to pidns makes some sense, but is it definately
what we want?

Is the idea that the container should never be able to bypass the
restrictions, or should root in the container eventually be able to
bypass it as he can on the host?

Would a securebits interface be more or less suitable?  It would allow
per-process setting, inherited from parent on fork.  It would however
not allow a root shell on the desktop, after the fact, saying that
a running gdb should be allowed to access firefox.  But, it would be
able to say "I, from now on, am exempt, so that I can debug the
running firefox", without the rest of the system having its setting
changed.

> As Yama is not merged yet, I post the patch in this thread.
> 
> The patch is straightforward:
> 
> 1) all sysctl variables are moved from global vars to pid_namespace
> fields.
> 
> 2) each cloned pid ns gets the settings of the parent.
> 
> 3) the variables of current pid ns are visible through sysctl interface.
> 
> proc_pid_dointvec_minmax() is stolen from its ipc_ns equivalent in
> ipc/ipc_sysctl.c.
> 
> (The patch is not tested.)
> 
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 38d1032..46edaf8 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -30,6 +30,11 @@ struct pid_namespace {
>  #ifdef CONFIG_BSD_PROCESS_ACCT
>  	struct bsd_acct_struct *bacct;
>  #endif
> +#ifdef CONFIG_SECURITY_YAMA
> +	int ptrace_scope;
> +	int protected_sticky_symlinks;
> +	int protected_nonaccess_hardlinks;
> +#endif
>  };
>  
>  extern struct pid_namespace init_pid_ns;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index fa5f722..0cd8926 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -78,6 +78,11 @@ struct pid_namespace init_pid_ns = {
>  	.last_pid = 0,
>  	.level = 0,
>  	.child_reaper = &init_task,
> +#ifdef CONFIG_SECURITY_YAMA
> +	.ptrace_scope = 1,
> +	.protected_sticky_symlinks = 1,
> +	.protected_nonaccess_hardlinks = 1,
> +#endif
>  };
>  EXPORT_SYMBOL_GPL(init_pid_ns);
>  
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index e9c9adc..73d47c4 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -101,6 +101,14 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_p
>  	if (err)
>  		goto out_put_parent_pid_ns;
>  
> +#ifdef CONFIG_SECURITY_YAMA
> +	ns->ptrace_scope = parent_pid_ns->ptrace_scope;
> +	ns->protected_sticky_symlinks =
> +		parent_pid_ns->protected_sticky_symlinks;
> +	ns->protected_nonaccess_hardlinks =
> +		parent_pid_ns->protected_nonaccess_hardlinks;
> +#endif
> +
>  	return ns;
>  
>  out_put_parent_pid_ns:
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index a92538c..cf99a8c 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -16,10 +16,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/prctl.h>
>  #include <linux/ratelimit.h>
> -
> -static int ptrace_scope = 1;
> -static int protected_sticky_symlinks = 1;
> -static int protected_nonaccess_hardlinks = 1;
> +#include <linux/pid_namespace.h>
>  
>  /* describe a PTRACE relationship for potential exception */
>  struct ptrace_relation {
> @@ -250,7 +247,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
>  
>  	/* require ptrace target be a child of ptracer on attach */
>  	if (mode == PTRACE_MODE_ATTACH &&
> -	    ptrace_scope &&
> +	    current->nsproxy->pid_ns->ptrace_scope &&
>  	    !capable(CAP_SYS_PTRACE) &&
>  	    !task_is_descendant(current, child) &&
>  	    !ptracer_exception_found(current, child))
> @@ -292,7 +289,7 @@ static int yama_inode_follow_link(struct dentry *dentry,
>  	const struct inode *inode;
>  	const struct cred *cred;
>  
> -	if (!protected_sticky_symlinks)
> +	if (!current->nsproxy->pid_ns->protected_sticky_symlinks)
>  		return 0;
>  
>  	/* if inode isn't a symlink, don't try to evaluate blocking it */
> @@ -362,7 +359,7 @@ static int yama_path_link(struct dentry *old_dentry, struct path *new_dir,
>  	const int mode = inode->i_mode;
>  	const struct cred *cred = current_cred();
>  
> -	if (!protected_nonaccess_hardlinks)
> +	if (!current->nsproxy->pid_ns->protected_nonaccess_hardlinks)
>  		return 0;
>  
>  	if (cred->fsuid != inode->i_uid &&
> @@ -395,6 +392,26 @@ static struct security_operations yama_ops = {
>  static int zero;
>  static int one = 1;
>  
> +static void *get_pid_data(ctl_table *table)
> +{
> +	char *which = table->data;
> +	struct pid_namespace *pid_ns = current->nsproxy->pid_ns;
> +	which = (which - (char *)&init_pid_ns) + (char *)pid_ns;
> +	return which;
> +}
> +
> +static int proc_pid_dointvec_minmax(ctl_table *table, int write,
> +	void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct ctl_table pid_table;
> +
> +	memcpy(&pid_table, table, sizeof(pid_table));
> +	pid_table.data = get_pid_data(table);
> +
> +	return proc_dointvec_minmax(&pid_table, write, buffer, lenp, ppos);
> +}
> +
> +
>  struct ctl_path yama_sysctl_path[] = {
>  	{ .procname = "kernel", },
>  	{ .procname = "yama", },
> @@ -404,28 +421,28 @@ struct ctl_path yama_sysctl_path[] = {
>  static struct ctl_table yama_sysctl_table[] = {
>  	{
>  		.procname       = "protected_sticky_symlinks",
> -		.data           = &protected_sticky_symlinks,
> +		.data           = &init_pid_ns.protected_sticky_symlinks,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
>  	{
>  		.procname       = "protected_nonaccess_hardlinks",
> -		.data           = &protected_nonaccess_hardlinks,
> +		.data           = &init_pid_ns.protected_nonaccess_hardlinks,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
>  	{
>  		.procname       = "ptrace_scope",
> -		.data           = &ptrace_scope,
> +		.data           = &init_pid_ns.ptrace_scope,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
> ---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Make Yama pid_ns aware

[Vasiliy Kulikov]

Hi Serge,

On Tue, Nov 22, 2011 at 12:13 -0600, Serge Hallyn wrote:
> Quoting Vasiliy Kulikov (segoon@openwall.com):
> > As Yama's sysctls are about defining a security policy for the system,
> > it is reasonable to define it per container in case of LXC containers
> > (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> > to pid namespace.  With per-pid_ns sysctls it is possible to create
> > multiple containers with different ptrace, /tmp, etc. policies.
> 
> tying the ptrace policy to pidns makes some sense, but is it definately
> what we want?
> 
> Is the idea that the container should never be able to bypass the
> restrictions, or should root in the container eventually be able to
> bypass it as he can on the host?

In-container root already has CAP_SYS_PTRACE, so he can avoid the check
even if Yama's ptrace policy is enabled.


> Would a securebits interface be more or less suitable?  It would allow
> per-process setting, inherited from parent on fork.

IMHO securebits doesn't belong to such finegrained type of things.
I don't find anything dangerous for privileged process (i.e. with
CAP_SYS_ADMIN and CAP_SYS_PTRACE) to be able to fork from
ptrace-restricted pid namespace, unshare pid namespace and relax ptrace
policy.  Because (1) process which is able to do unshare is already able
to ptrace everybody and (2) unshared namespace cannot explicitly
interact with parent namespace.


> It would however
> not allow a root shell on the desktop, after the fact, saying that
> a running gdb should be allowed to access firefox.  But, it would be
> able to say "I, from now on, am exempt, so that I can debug the
> running firefox", without the rest of the system having its setting
> changed.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [RFC] Make Yama pid_ns aware

[Serge Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> Hi Serge,
> 
> On Tue, Nov 22, 2011 at 12:13 -0600, Serge Hallyn wrote:
> > Quoting Vasiliy Kulikov (segoon@openwall.com):
> > > As Yama's sysctls are about defining a security policy for the system,
> > > it is reasonable to define it per container in case of LXC containers
> > > (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> > > to pid namespace.  With per-pid_ns sysctls it is possible to create
> > > multiple containers with different ptrace, /tmp, etc. policies.
> > 
> > tying the ptrace policy to pidns makes some sense, but is it definately
> > what we want?
> > 
> > Is the idea that the container should never be able to bypass the
> > restrictions, or should root in the container eventually be able to
> > bypass it as he can on the host?
> 
> In-container root already has CAP_SYS_PTRACE, so he can avoid the check
> even if Yama's ptrace policy is enabled.

Well, not necessarily  :)  But in general.

But still, is turning this on and off per-container, and leaving it off
on the host, something people will reasonably want to do?  I'm just
wondering whether adding the extra data on the pidns is worth it.  It's
fine if it is, but I'm having a hard time imagining someone using it
like that.

thanks,
-serge

Re: [RFC] Make Yama pid_ns aware

[Vasiliy Kulikov]

On Tue, Nov 22, 2011 at 14:10 -0600, Serge Hallyn wrote:
> Quoting Vasiliy Kulikov (segoon@openwall.com):
> > Hi Serge,
> > 
> > On Tue, Nov 22, 2011 at 12:13 -0600, Serge Hallyn wrote:
> > > Quoting Vasiliy Kulikov (segoon@openwall.com):
> > > > As Yama's sysctls are about defining a security policy for the system,
> > > > it is reasonable to define it per container in case of LXC containers
> > > > (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> > > > to pid namespace.  With per-pid_ns sysctls it is possible to create
> > > > multiple containers with different ptrace, /tmp, etc. policies.
> > > 
> > > tying the ptrace policy to pidns makes some sense, but is it definately
> > > what we want?
> > > 
> > > Is the idea that the container should never be able to bypass the
> > > restrictions, or should root in the container eventually be able to
> > > bypass it as he can on the host?
> > 
> > In-container root already has CAP_SYS_PTRACE, so he can avoid the check
> > even if Yama's ptrace policy is enabled.
> 
> Well, not necessarily  :)  But in general.

This is the question is what in-container root is :-)  Until LXC root is
restricted up to the case where he is unable to get out of the container
in the mainline kernel, I assume we're talking about an abstract entity
which has full control over the container.  It includes stuff like
CAP_CHOWN, CAP_KILL, CAP_SYS_PTRACE, CAP_NET_ADMIN, etc. etc.  The
debatable thing is what parts of CAP_SYS_ADMIN belong to in-container
root, like ability to mount/umount.  But CAP_SYS_PTRACE is surely
belongs to the in-container root.


> But still, is turning this on and off per-container, and leaving it off
> on the host, something people will reasonably want to do?

Probably we need strict rules like ptrace is relaxed iff in both source
ns and dest ns ptrace is relaxed.


> I'm just
> wondering whether adding the extra data on the pidns is worth it.  It's
> fine if it is, but I'm having a hard time imagining someone using it
> like that.

We have already very big net_namespace with all kind of per-ns stuff.
Yama's variables don't significantly increase the size of container.


Actually, what concerns me is not ptrace, but symlink/hardling
protection.  There is no interaction between namespaces in case of
containers via symlinks in the basic case.  In case of ptrace I don't
think the child ns may weaken the parent ns - child ns may not access
processes of the parent namespace and everything it may ptrace is
already inside of this ns.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [RFC] Make Yama pid_ns aware

[Serge Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> > But still, is turning this on and off per-container, and leaving it off
> > on the host, something people will reasonably want to do?
> 
> Probably we need strict rules like ptrace is relaxed iff in both source
> ns and dest ns ptrace is relaxed.

But will people want that?

> > I'm just
> > wondering whether adding the extra data on the pidns is worth it.  It's
> > fine if it is, but I'm having a hard time imagining someone using it
> > like that.
> 
> We have already very big net_namespace with all kind of per-ns stuff.
> Yama's variables don't significantly increase the size of container.

Yes I'm not complaining about the extra host memory usage, just trying
to make sure we don't pollute the pidns struct with something noone
wants

> Actually, what concerns me is not ptrace, but symlink/hardling
> protection.  There is no interaction between namespaces in case of
> containers via symlinks in the basic case.  In case of ptrace I don't
> think the child ns may weaken the parent ns - child ns may not access
> processes of the parent namespace and everything it may ptrace is
> already inside of this ns.

User namespace being respected by VFS will help symlinking.

-serge

Re: [RFC] Make Yama pid_ns aware

[Serge E. Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> Actually, what concerns me is not ptrace, but symlink/hardling
> protection.  There is no interaction between namespaces in case of
> containers via symlinks in the basic case.  In case of ptrace I don't
> think the child ns may weaken the parent ns - child ns may not access
> processes of the parent namespace and everything it may ptrace is
> already inside of this ns.

Oh, yes.  If you're saying the symlink protection shouldn't be
per-pidns, I agree it seems an odd fit.

How about a version of this patch leaving symlink protection
out of pidns (maybe in user ns), and just putting ptrace
protection per-pidns?

-serge

Re: [RFC] Make Yama pid_ns aware

[Vasiliy Kulikov]

On Wed, Nov 23, 2011 at 14:49 +0000, Serge E. Hallyn wrote:
> Quoting Vasiliy Kulikov (segoon@openwall.com):
> > Actually, what concerns me is not ptrace, but symlink/hardling
> > protection.  There is no interaction between namespaces in case of
> > containers via symlinks in the basic case.  In case of ptrace I don't
> > think the child ns may weaken the parent ns - child ns may not access
> > processes of the parent namespace and everything it may ptrace is
> > already inside of this ns.
> 
> Oh, yes.  If you're saying the symlink protection shouldn't be
> per-pidns, I agree it seems an odd fit.
> 
> How about a version of this patch leaving symlink protection
> out of pidns (maybe in user ns), and just putting ptrace
> protection per-pidns?

I don't think moving symlink/hardling from pid ns to user ns is a good
idea as user ns is not matured yet.  Also we (Openwall) want to use Yama
almost as-is in our RHEL6 and RHEL5-based kernels with OpenVZ support
which don't have user namespaces yet at all (yes, it is not a cause for
mainline decisions :-) ).

While user ns is not yet ready, I don't clearly see what is the division
of security policies among namespaces including user namespace.  I had
a view that all stuff related to processes (i.e. distinct processes in
several ways) belongs to pid ns, all net stuff to net ns, etc.  If we
differentiate user ns and pid ns, only strictly things handling pids
(like kill(2), procfs, ptrace(2), etc.) belong to pid ns and all other
process-related stuff (like credentials handling, these symlink/hardling
things, etc.) belong to user namespaces.

Can we probably leave them in pid ns for now and when user ns is matured
just move it from pid ns to user ns?  Is it possible without breaking
Yama's ABI (I think so)?

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [RFC] Make Yama pid_ns aware

[Serge Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> On Wed, Nov 23, 2011 at 14:49 +0000, Serge E. Hallyn wrote:
> > Quoting Vasiliy Kulikov (segoon@openwall.com):
> > > Actually, what concerns me is not ptrace, but symlink/hardling
> > > protection.  There is no interaction between namespaces in case of
> > > containers via symlinks in the basic case.  In case of ptrace I don't
> > > think the child ns may weaken the parent ns - child ns may not access
> > > processes of the parent namespace and everything it may ptrace is
> > > already inside of this ns.
> > 
> > Oh, yes.  If you're saying the symlink protection shouldn't be
> > per-pidns, I agree it seems an odd fit.
> > 
> > How about a version of this patch leaving symlink protection
> > out of pidns (maybe in user ns), and just putting ptrace
> > protection per-pidns?
> 
> I don't think moving symlink/hardling from pid ns to user ns is a good
> idea as user ns is not matured yet.  Also we (Openwall) want to use Yama
> almost as-is in our RHEL6 and RHEL5-based kernels with OpenVZ support
> which don't have user namespaces yet at all (yes, it is not a cause for
> mainline decisions :-) ).
> 
> While user ns is not yet ready, I don't clearly see what is the division
> of security policies among namespaces including user namespace.  I had
> a view that all stuff related to processes (i.e. distinct processes in
> several ways) belongs to pid ns, all net stuff to net ns, etc.  If we
> differentiate user ns and pid ns, only strictly things handling pids
> (like kill(2), procfs, ptrace(2), etc.) belong to pid ns and all other
> process-related stuff (like credentials handling, these symlink/hardling
> things, etc.) belong to user namespaces.
> 
> Can we probably leave them in pid ns for now and when user ns is matured
> just move it from pid ns to user ns?  Is it possible without breaking
> Yama's ABI (I think so)?

That's fine with me.

thanks,
-serge

Re: [RFC] Make Yama pid_ns aware

[Kees Cook]

On Wed, Nov 23, 2011 at 8:55 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> On Wed, Nov 23, 2011 at 14:49 +0000, Serge E. Hallyn wrote:
>> Quoting Vasiliy Kulikov (segoon@openwall.com):
>> > Actually, what concerns me is not ptrace, but symlink/hardling
>> > protection.  There is no interaction between namespaces in case of
>> > containers via symlinks in the basic case.  In case of ptrace I don't
>> > think the child ns may weaken the parent ns - child ns may not access
>> > processes of the parent namespace and everything it may ptrace is
>> > already inside of this ns.
>>
>> Oh, yes.  If you're saying the symlink protection shouldn't be
>> per-pidns, I agree it seems an odd fit.
>>
>> How about a version of this patch leaving symlink protection
>> out of pidns (maybe in user ns), and just putting ptrace
>> protection per-pidns?
>
> I don't think moving symlink/hardling from pid ns to user ns is a good
> idea as user ns is not matured yet.  Also we (Openwall) want to use Yama
> almost as-is in our RHEL6 and RHEL5-based kernels with OpenVZ support
> which don't have user namespaces yet at all (yes, it is not a cause for
> mainline decisions :-) ).
>
> While user ns is not yet ready, I don't clearly see what is the division
> of security policies among namespaces including user namespace.  I had
> a view that all stuff related to processes (i.e. distinct processes in
> several ways) belongs to pid ns, all net stuff to net ns, etc.  If we
> differentiate user ns and pid ns, only strictly things handling pids
> (like kill(2), procfs, ptrace(2), etc.) belong to pid ns and all other
> process-related stuff (like credentials handling, these symlink/hardling
> things, etc.) belong to user namespaces.
>
> Can we probably leave them in pid ns for now and when user ns is matured
> just move it from pid ns to user ns?  Is it possible without breaking
> Yama's ABI (I think so)?

So it sounds like you'll send a new patch where only ptrace_scope is
tied to pidns?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [RFC] Make Yama pid_ns aware

[Vasiliy Kulikov]

On Mon, Nov 28, 2011 at 10:12 -0800, Kees Cook wrote:
> On Wed, Nov 23, 2011 at 8:55 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> > Can we probably leave them in pid ns for now and when user ns is matured
> > just move it from pid ns to user ns?  Is it possible without breaking
> > Yama's ABI (I think so)?
> 
> So it sounds like you'll send a new patch where only ptrace_scope is
> tied to pidns?

Em, no.  I've said we'll leave everything in pid ns for now.  When user
ns is ready, we'll move some options (all of them?) from pid ns to user
ns.  As user interface is kernel.yama.* sysctls and will not be changed
with the migration, ABI would not be broken by this migration.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[RFC -resend] Make Yama pid_ns aware

[Vasiliy Kulikov]

As Yama's sysctls are about defining a security policy for the system,
it is reasonable to define it per container in case of LXC containers
(or out-of-tree alternatives like OpenVZ).  In my opinion they belong
to pid namespace.  With per-pid_ns sysctls it is possible to create
multiple containers with different ptrace, /tmp, etc. policies.

The patch is straightforward:

1) all sysctl variables are moved from global vars to pid_namespace
fields.

2) each cloned pid ns gets the settings of the parent.

3) the variables of current pid ns are visible through sysctl interface.

proc_pid_dointvec_minmax() is stolen from its ipc_ns equivalent in
ipc/ipc_sysctl.c.

P.S.  As user namespaces are not merged yet and it's not clear when/whether
they are merged, these changes belong to pid namespace for now.  When
user namespaces are merged, per pid_ns variables should go to struct
user_namespace.  It will not break ABI as userspace sees the same
kernel.yama.* sysctls in both pid ns and user ns cases.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
--

diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 38d1032..46edaf8 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -30,6 +30,11 @@ struct pid_namespace {
 #ifdef CONFIG_BSD_PROCESS_ACCT
 	struct bsd_acct_struct *bacct;
 #endif
+#ifdef CONFIG_SECURITY_YAMA
+	int ptrace_scope;
+	int protected_sticky_symlinks;
+	int protected_nonaccess_hardlinks;
+#endif
 };
 
 extern struct pid_namespace init_pid_ns;
diff --git a/kernel/pid.c b/kernel/pid.c
index fa5f722..0cd8926 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -78,6 +78,11 @@ struct pid_namespace init_pid_ns = {
 	.last_pid = 0,
 	.level = 0,
 	.child_reaper = &init_task,
+#ifdef CONFIG_SECURITY_YAMA
+	.ptrace_scope = 1,
+	.protected_sticky_symlinks = 1,
+	.protected_nonaccess_hardlinks = 1,
+#endif
 };
 EXPORT_SYMBOL_GPL(init_pid_ns);
 
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index e9c9adc..73d47c4 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -101,6 +101,14 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_p
 	if (err)
 		goto out_put_parent_pid_ns;
 
+#ifdef CONFIG_SECURITY_YAMA
+	ns->ptrace_scope = parent_pid_ns->ptrace_scope;
+	ns->protected_sticky_symlinks =
+		parent_pid_ns->protected_sticky_symlinks;
+	ns->protected_nonaccess_hardlinks =
+		parent_pid_ns->protected_nonaccess_hardlinks;
+#endif
+
 	return ns;
 
 out_put_parent_pid_ns:
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index a92538c..cf99a8c 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -16,10 +16,7 @@
 #include <linux/ptrace.h>
 #include <linux/prctl.h>
 #include <linux/ratelimit.h>
-
-static int ptrace_scope = 1;
-static int protected_sticky_symlinks = 1;
-static int protected_nonaccess_hardlinks = 1;
+#include <linux/pid_namespace.h>
 
 /* describe a PTRACE relationship for potential exception */
 struct ptrace_relation {
@@ -250,7 +247,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
 
 	/* require ptrace target be a child of ptracer on attach */
 	if (mode == PTRACE_MODE_ATTACH &&
-	    ptrace_scope &&
+	    current->nsproxy->pid_ns->ptrace_scope &&
 	    !capable(CAP_SYS_PTRACE) &&
 	    !task_is_descendant(current, child) &&
 	    !ptracer_exception_found(current, child))
@@ -292,7 +289,7 @@ static int yama_inode_follow_link(struct dentry *dentry,
 	const struct inode *inode;
 	const struct cred *cred;
 
-	if (!protected_sticky_symlinks)
+	if (!current->nsproxy->pid_ns->protected_sticky_symlinks)
 		return 0;
 
 	/* if inode isn't a symlink, don't try to evaluate blocking it */
@@ -362,7 +359,7 @@ static int yama_path_link(struct dentry *old_dentry, struct path *new_dir,
 	const int mode = inode->i_mode;
 	const struct cred *cred = current_cred();
 
-	if (!protected_nonaccess_hardlinks)
+	if (!current->nsproxy->pid_ns->protected_nonaccess_hardlinks)
 		return 0;
 
 	if (cred->fsuid != inode->i_uid &&
@@ -395,6 +392,26 @@ static struct security_operations yama_ops = {
 static int zero;
 static int one = 1;
 
+static void *get_pid_data(ctl_table *table)
+{
+	char *which = table->data;
+	struct pid_namespace *pid_ns = current->nsproxy->pid_ns;
+	which = (which - (char *)&init_pid_ns) + (char *)pid_ns;
+	return which;
+}
+
+static int proc_pid_dointvec_minmax(ctl_table *table, int write,
+	void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table pid_table;
+
+	memcpy(&pid_table, table, sizeof(pid_table));
+	pid_table.data = get_pid_data(table);
+
+	return proc_dointvec_minmax(&pid_table, write, buffer, lenp, ppos);
+}
+
+
 struct ctl_path yama_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "yama", },
@@ -404,28 +421,28 @@ struct ctl_path yama_sysctl_path[] = {
 static struct ctl_table yama_sysctl_table[] = {
 	{
 		.procname       = "protected_sticky_symlinks",
-		.data           = &protected_sticky_symlinks,
+		.data           = &init_pid_ns.protected_sticky_symlinks,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
 	{
 		.procname       = "protected_nonaccess_hardlinks",
-		.data           = &protected_nonaccess_hardlinks,
+		.data           = &init_pid_ns.protected_nonaccess_hardlinks,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
 	{
 		.procname       = "ptrace_scope",
-		.data           = &ptrace_scope,
+		.data           = &init_pid_ns.ptrace_scope,
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
-		.proc_handler   = proc_dointvec_minmax,
+		.proc_handler   = proc_pid_dointvec_minmax,
 		.extra1         = &zero,
 		.extra2         = &one,
 	},
---

Re: [RFC -resend] Make Yama pid_ns aware

[Kees Cook]

On Mon, Nov 28, 2011 at 11:16 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> As Yama's sysctls are about defining a security policy for the system,
> it is reasonable to define it per container in case of LXC containers
> (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> to pid namespace.  With per-pid_ns sysctls it is possible to create
> multiple containers with different ptrace, /tmp, etc. policies.
> [...]
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>

Thanks! I'll include this in the next Yama patch set.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [RFC -resend] Make Yama pid_ns aware

[Kees Cook]

On Mon, Nov 28, 2011 at 11:35 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Nov 28, 2011 at 11:16 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
>> As Yama's sysctls are about defining a security policy for the system,
>> it is reasonable to define it per container in case of LXC containers
>> (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
>> to pid namespace.  With per-pid_ns sysctls it is possible to create
>> multiple containers with different ptrace, /tmp, etc. policies.
>> [...]
>> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
>
> Thanks! I'll include this in the next Yama patch set.
>
> Acked-by: Kees Cook <keescook@chromium.org>

I should mention, this lives here now:
http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/yama

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [RFC -resend] Make Yama pid_ns aware

[Serge E. Hallyn]

Quoting Vasiliy Kulikov (segoon@openwall.com):
> As Yama's sysctls are about defining a security policy for the system,
> it is reasonable to define it per container in case of LXC containers
> (or out-of-tree alternatives like OpenVZ).  In my opinion they belong
> to pid namespace.  With per-pid_ns sysctls it is possible to create
> multiple containers with different ptrace, /tmp, etc. policies.
> 
> The patch is straightforward:
> 
> 1) all sysctl variables are moved from global vars to pid_namespace
> fields.
> 
> 2) each cloned pid ns gets the settings of the parent.
> 
> 3) the variables of current pid ns are visible through sysctl interface.
> 
> proc_pid_dointvec_minmax() is stolen from its ipc_ns equivalent in
> ipc/ipc_sysctl.c.
> 
> P.S.  As user namespaces are not merged yet and it's not clear when/whether
> they are merged, these changes belong to pid namespace for now.  When
> user namespaces are merged, per pid_ns variables should go to struct
> user_namespace.  It will not break ABI as userspace sees the same
> kernel.yama.* sysctls in both pid ns and user ns cases.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>

Acked-by: Serge Hallyn <serge.hallyn@ubuntu.com>

thanks,
-serge

> --
> 
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 38d1032..46edaf8 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -30,6 +30,11 @@ struct pid_namespace {
>  #ifdef CONFIG_BSD_PROCESS_ACCT
>  	struct bsd_acct_struct *bacct;
>  #endif
> +#ifdef CONFIG_SECURITY_YAMA
> +	int ptrace_scope;
> +	int protected_sticky_symlinks;
> +	int protected_nonaccess_hardlinks;
> +#endif
>  };
>  
>  extern struct pid_namespace init_pid_ns;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index fa5f722..0cd8926 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -78,6 +78,11 @@ struct pid_namespace init_pid_ns = {
>  	.last_pid = 0,
>  	.level = 0,
>  	.child_reaper = &init_task,
> +#ifdef CONFIG_SECURITY_YAMA
> +	.ptrace_scope = 1,
> +	.protected_sticky_symlinks = 1,
> +	.protected_nonaccess_hardlinks = 1,
> +#endif
>  };
>  EXPORT_SYMBOL_GPL(init_pid_ns);
>  
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index e9c9adc..73d47c4 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -101,6 +101,14 @@ static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_p
>  	if (err)
>  		goto out_put_parent_pid_ns;
>  
> +#ifdef CONFIG_SECURITY_YAMA
> +	ns->ptrace_scope = parent_pid_ns->ptrace_scope;
> +	ns->protected_sticky_symlinks =
> +		parent_pid_ns->protected_sticky_symlinks;
> +	ns->protected_nonaccess_hardlinks =
> +		parent_pid_ns->protected_nonaccess_hardlinks;
> +#endif
> +
>  	return ns;
>  
>  out_put_parent_pid_ns:
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index a92538c..cf99a8c 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -16,10 +16,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/prctl.h>
>  #include <linux/ratelimit.h>
> -
> -static int ptrace_scope = 1;
> -static int protected_sticky_symlinks = 1;
> -static int protected_nonaccess_hardlinks = 1;
> +#include <linux/pid_namespace.h>
>  
>  /* describe a PTRACE relationship for potential exception */
>  struct ptrace_relation {
> @@ -250,7 +247,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
>  
>  	/* require ptrace target be a child of ptracer on attach */
>  	if (mode == PTRACE_MODE_ATTACH &&
> -	    ptrace_scope &&
> +	    current->nsproxy->pid_ns->ptrace_scope &&
>  	    !capable(CAP_SYS_PTRACE) &&
>  	    !task_is_descendant(current, child) &&
>  	    !ptracer_exception_found(current, child))
> @@ -292,7 +289,7 @@ static int yama_inode_follow_link(struct dentry *dentry,
>  	const struct inode *inode;
>  	const struct cred *cred;
>  
> -	if (!protected_sticky_symlinks)
> +	if (!current->nsproxy->pid_ns->protected_sticky_symlinks)
>  		return 0;
>  
>  	/* if inode isn't a symlink, don't try to evaluate blocking it */
> @@ -362,7 +359,7 @@ static int yama_path_link(struct dentry *old_dentry, struct path *new_dir,
>  	const int mode = inode->i_mode;
>  	const struct cred *cred = current_cred();
>  
> -	if (!protected_nonaccess_hardlinks)
> +	if (!current->nsproxy->pid_ns->protected_nonaccess_hardlinks)
>  		return 0;
>  
>  	if (cred->fsuid != inode->i_uid &&
> @@ -395,6 +392,26 @@ static struct security_operations yama_ops = {
>  static int zero;
>  static int one = 1;
>  
> +static void *get_pid_data(ctl_table *table)
> +{
> +	char *which = table->data;
> +	struct pid_namespace *pid_ns = current->nsproxy->pid_ns;
> +	which = (which - (char *)&init_pid_ns) + (char *)pid_ns;
> +	return which;
> +}
> +
> +static int proc_pid_dointvec_minmax(ctl_table *table, int write,
> +	void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	struct ctl_table pid_table;
> +
> +	memcpy(&pid_table, table, sizeof(pid_table));
> +	pid_table.data = get_pid_data(table);
> +
> +	return proc_dointvec_minmax(&pid_table, write, buffer, lenp, ppos);
> +}
> +
> +
>  struct ctl_path yama_sysctl_path[] = {
>  	{ .procname = "kernel", },
>  	{ .procname = "yama", },
> @@ -404,28 +421,28 @@ struct ctl_path yama_sysctl_path[] = {
>  static struct ctl_table yama_sysctl_table[] = {
>  	{
>  		.procname       = "protected_sticky_symlinks",
> -		.data           = &protected_sticky_symlinks,
> +		.data           = &init_pid_ns.protected_sticky_symlinks,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
>  	{
>  		.procname       = "protected_nonaccess_hardlinks",
> -		.data           = &protected_nonaccess_hardlinks,
> +		.data           = &init_pid_ns.protected_nonaccess_hardlinks,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
>  	{
>  		.procname       = "ptrace_scope",
> -		.data           = &ptrace_scope,
> +		.data           = &init_pid_ns.ptrace_scope,
>  		.maxlen         = sizeof(int),
>  		.mode           = 0644,
> -		.proc_handler   = proc_dointvec_minmax,
> +		.proc_handler   = proc_pid_dointvec_minmax,
>  		.extra1         = &zero,
>  		.extra2         = &one,
>  	},
> ---