[v3,6/8] fs/proc: fix attr access check

Commit Message

Jann Horn Oct. 30, 2016, 9:46 p.m. UTC

Make sure files in /proc/$pid/attr/ can only be written by the task that
opened them.

This prevents an attacking process from changing the security context of
another process that it can force to write attacker-controlled data into an
attacker-supplied file descriptor. I'm not sure what the impact of this is.

changed in v2:
 - changed privunit-using code

Signed-off-by: Jann Horn <[e-mail]>
---
 fs/proc/base.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 32ea9bc3d320..cbba490543e2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2518,6 +2518,18 @@  static int proc_pident_readdir(struct file *file, struct dir_context *ctx,
 }
 
 #ifdef CONFIG_SECURITY
+static int proc_pid_attr_open(struct inode *inode, struct file *file)
+{
+	struct luid *opener_privunit;
+
+	opener_privunit = kmalloc(sizeof(struct luid), GFP_KERNEL);
+	if (opener_privunit == NULL)
+		return -ENOMEM;
+	*opener_privunit = current->privunit;
+	file->private_data = opener_privunit;
+	return 0;
+}
+
 static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
 				  size_t count, loff_t *ppos)
 {
@@ -2546,6 +2558,7 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	void *page;
 	ssize_t length;
 	struct task_struct *task = get_proc_task(inode);
+	struct luid *opener_privunit = file->private_data;
 
 	length = -ESRCH;
 	if (!task)
@@ -2569,9 +2582,29 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	if (length < 0)
 		goto out_free;
 
+	/*
+	 * Ensure that a process can't be tricked into writing into its own attr
+	 * files without intending to do so.
+	 *
+	 * SELinux has a rule that prevents anyone other than `task` from
+	 * writing, but if the fd stays open across execve, or is sent across a
+	 * unix domain socket or whatever, that is bypassable.
+	 * Same thing in AppArmor and in Smack.
+	 *
+	 * To prevent this, compare the opener's exec_id with the target's to
+	 * ensure that they're in the same task group and no exec happened in
+	 * the meantime.
+	 *
+	 * Why is this a file and not a prctl or whatever. :/
+	 */
+	length = -EACCES;
+	if (!luid_eq(opener_privunit, &task->privunit))
+		goto out_unlock;
+
 	length = security_setprocattr(task,
 				      (char*)file->f_path.dentry->d_name.name,
 				      page, count);
+out_unlock:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out_free:
 	kfree(page);
@@ -2581,10 +2614,20 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	return length;
 }
 
+static int proc_pid_attr_release(struct inode *inode, struct file *file)
+{
+	struct luid *opener_privunit = file->private_data;
+
+	kfree(opener_privunit);
+	return 0;
+}
+
 static const struct file_operations proc_pid_attr_operations = {
+	.open		= proc_pid_attr_open,
 	.read		= proc_pid_attr_read,
 	.write		= proc_pid_attr_write,
 	.llseek		= generic_file_llseek,
+	.release	= proc_pid_attr_release,
 };
 
 static const struct pid_entry attr_dir_stuff[] = {