Message-ID: <1432005006-3428-1-git-send-email-calvinowens@fb.com>
Date:	Mon, 18 May 2015 20:10:06 -0700
From:	Calvin Owens <calvinowens@...com>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...il.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Miklos Szeredi <miklos@...redi.hu>,
	Zefan Li <lizefan@...wei.com>, Oleg Nesterov <oleg@...hat.com>,
	Joe Perches <joe@...ches.com>,
	David Howells <dhowells@...hat.com>
CC:	Calvin Owens <calvinowens@...com>, <linux-kernel@...r.kernel.org>,
	<kernel-team@...com>, Andy Lutomirski <luto@...capital.net>,
	Kees Cook <keescook@...omium.org>,
	"Kirill A. Shutemov" <kirill@...temov.name>
Subject: [PATCH v5] procfs: Always expose /proc/<pid>/map_files/ and make it readable

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface is
very useful for enumerating the files mapped into a process when the
more verbose information in /proc/<pid>/maps is not needed. It also
allows access to file descriptors for files that have been deleted and
closed but are still mmapped into a process, which can be very useful
for introspection and debugging.

This patch moves the folder out from behind CHECKPOINT_RESTORE, and
removes the CAP_SYS_ADMIN restrictions. With that change alone,
following the links would have required PTRACE_MODE_READ like the
links in /proc/<pid>/fd/*.

However, a discussion on lkml concluded that MODE_READ is not
sufficient, both because write access to the inodes these links point
to allows direct modification of a process's address space, and
because it exposes files that users may have overlooked permissions on
because it was assumed they would be inaccessible (either deleted as
per above, or created via O_TMPFILE).

So, in addition to the above, this patch enforces PTRACE_MODE_ATTACH on
all the map_files operations. Since this is the same check that
determines if access to /proc/<pid>/mem is allowed, it will not allow an
attacker to do anything that was not already possible through that
interface.

Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Andy Lutomirski <luto@...capital.net>
Cc: Kees Cook <keescook@...omium.org>
Cc: Kirill A. Shutemov <kirill@...temov.name>
Reviewed-by: Cyrill Gorcunov <gorcunov@...nvz.org>
Signed-off-by: Calvin Owens <calvinowens@...com>
---
Changes in v5:	s/dentry->d_inode/d_inode(dentry)/g

Changes in v4:	Return -ESRCH from follow_link() when get_proc_task()
		returns NULL.

Changes in v3:	Changed permission checks to use PTRACE_MODE_ATTACH
		instead of PTRACE_MODE_READ, and added a stub to
		enforce MODE_ATTACH on follow_link() as well.

Changes in v2:	Removed the follow_link() stub that returned -EPERM if
		the caller didn't have CAP_SYS_ADMIN, since the caller
		in my chroot() scenario gets -EACCES anyway.

 fs/proc/base.c | 61 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 093ca14..22d95a7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1641,8 +1641,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.
@@ -1669,17 +1667,12 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
-	if (!capable(CAP_SYS_ADMIN)) {
-		status = -EPERM;
-		goto out_notask;
-	}
-
 	inode = d_inode(dentry);
 	task = get_proc_task(inode);
 	if (!task)
 		goto out_notask;
 
-	mm = mm_access(task, PTRACE_MODE_READ);
+	mm = mm_access(task, PTRACE_MODE_ATTACH);
 	if (IS_ERR_OR_NULL(mm))
 		goto out;
 
@@ -1762,6 +1755,41 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Enforce stronger PTRACE_MODE_ATTACH permissions on the symlinks under
+ * /proc/<pid>/map_files, since these links may refer to deleted or O_TMPFILE
+ * files that users might assume are inaccessible regardless of their
+ * ownership/permissions.
+ */
+static void *proc_map_files_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	struct inode *inode = d_inode(dentry);
+	struct task_struct *task;
+	int allowed = 0;
+
+	task = get_proc_task(inode);
+	if (task) {
+		allowed = ptrace_may_access(task, PTRACE_MODE_ATTACH);
+		put_task_struct(task);
+	} else {
+		return ERR_PTR(-ESRCH);
+	}
+
+	if (!allowed)
+		return ERR_PTR(-EACCES);
+
+	return proc_pid_follow_link(dentry, nd);
+}
+
+/*
+ * Identical to proc_pid_link_inode_operations except for follow_link()
+ */
+static const struct inode_operations proc_map_files_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_map_files_follow_link,
+	.setattr	= proc_setattr,
+};
+
 static int
 proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
 			   struct task_struct *task, const void *ptr)
@@ -1777,7 +1805,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
 	ei = PROC_I(inode);
 	ei->op.proc_get_link = proc_map_files_get_link;
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_map_files_link_inode_operations;
 	inode->i_size = 64;
 	inode->i_mode = S_IFLNK;
 
@@ -1801,17 +1829,13 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	int result;
 	struct mm_struct *mm;
 
-	result = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	result = -ENOENT;
 	task = get_proc_task(dir);
 	if (!task)
 		goto out;
 
 	result = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_may_access(task, PTRACE_MODE_ATTACH))
 		goto out_put_task;
 
 	result = -ENOENT;
@@ -1858,17 +1882,13 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	struct map_files_info *p;
 	int ret;
 
-	ret = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	ret = -ENOENT;
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		goto out;
 
 	ret = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_may_access(task, PTRACE_MODE_ATTACH))
 		goto out_put_task;
 
 	ret = 0;
@@ -2050,7 +2070,6 @@ static const struct file_operations proc_timers_operations = {
 	.llseek		= seq_lseek,
 	.release	= seq_release_private,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
 
 static int proc_pident_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)
@@ -2549,9 +2568,7 @@ static const struct inode_operations proc_task_inode_operations;
 static const struct pid_entry tgid_base_stuff[] = {
 	DIR("task",       S_IRUGO|S_IXUGO, proc_task_inode_operations, proc_task_operations),
 	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
-#ifdef CONFIG_CHECKPOINT_RESTORE
 	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
-#endif
 	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
 	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
 #ifdef CONFIG_NET
-- 
1.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists