[PATCH] proc: Properly filter out files that are not visible to a process

Long ago and far away in 2.2 we started checking to ensure the files we
displayed in /proc were visible to the current process.  It was an
unsophisticated time and no one was worried about functions full of FIXMES in
a stable kernel.  As time passed the function became sacred and was enshrined
in the shrine of how things have always been.  The fixes came in but only to
keep the function working no one really remembering or documenting why we did
things that way.

The intent and the functionality make a lot of sense.  Don't let /proc be an
access point for files a process can see no other way.  The implementation
however is completely wrong.

We are currently checking the root directories of the two processes, we are
not checking the actual file descriptors themselves.

We are strangely checking with a permission method instead of just when we use
the data.

This patch fixes the logic to actually check the file descriptors and make a
note that implementing a permission method for this part of /proc almost
certainly indicates a bug in the reasoning.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

1 files changed, 68 insertions, 33 deletions

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2e4356f..a85b073 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -74,6 +74,16 @@
 #include <linux/poll.h>
 #include "internal.h"
 
+/* NOTE:
+ *	Implementing inode permission operations in /proc is almost
+ *	certainly an error.  Permission checks need to happen during
+ *	each system call not at open time.  The reason is that most of
+ *	what we wish to check for permissions in /proc varies at runtime.
+ *
+ *	The classic example of a problem is opening file descriptors
+ *	in /proc for a task before it execs a suid executable.
+ */
+
 /*
  * For hysterical raisins we keep the same inumbers as in the old procfs.
  * Feel free to change the macro below - just keep the range distinct from
@@ -494,13 +504,11 @@ static int proc_oom_score(struct task_struct *task, char *buffer)
 
 /* If the process being read is separated by chroot from the reading process,
  * don't let the reader access the threads.
- *
- * note: this does dput(root) and mntput(vfsmnt) on exit.
  */
-static int proc_check_chroot(struct dentry *root, struct vfsmount *vfsmnt)
+static int proc_check_chroot(struct dentry *de, struct vfsmount *mnt)
 {
-	struct dentry *de, *base;
-	struct vfsmount *our_vfsmnt, *mnt;
+	struct dentry *base;
+	struct vfsmount *our_vfsmnt;
 	int res = 0;
 
 	read_lock(&current->fs->lock);
@@ -509,8 +517,6 @@ static int proc_check_chroot(struct dentry *root, struct vfsmount *vfsmnt)
 	read_unlock(&current->fs->lock);
 
 	spin_lock(&vfsmount_lock);
-	de = root;
-	mnt = vfsmnt;
 
 	while (mnt != our_vfsmnt) {
 		if (mnt == mnt->mnt_parent)
@@ -526,8 +532,6 @@ static int proc_check_chroot(struct dentry *root, struct vfsmount *vfsmnt)
 exit:
 	dput(base);
 	mntput(our_vfsmnt);
-	dput(root);
-	mntput(vfsmnt);
 	return res;
 out:
 	spin_unlock(&vfsmount_lock);
@@ -535,23 +539,6 @@ out:
 	goto exit;
 }
 
-static int proc_check_root(struct inode *inode)
-{
-	struct dentry *root;
-	struct vfsmount *vfsmnt;
-
-	if (proc_root_link(inode, &root, &vfsmnt)) /* Ewww... */
-		return -ENOENT;
-	return proc_check_chroot(root, vfsmnt);
-}
-
-static int proc_permission(struct inode *inode, int mask, struct nameidata *nd)
-{
-	if (generic_permission(inode, mask, NULL) != 0)
-		return -EACCES;
-	return proc_check_root(inode);
-}
-
 extern struct seq_operations proc_pid_maps_op;
 static int maps_open(struct inode *inode, struct file *file)
 {
@@ -1048,6 +1035,48 @@ static struct file_operations proc_seccomp_operations = {
 };
 #endif /* CONFIG_SECCOMP */
 
+static int proc_check_dentry_visible(struct inode *inode,
+	struct dentry *dentry, struct vfsmount *mnt)
+{
+	/* Verify that the current process can already see the
+	 * file pointed at by the file descriptor.
+	 * This prevents /proc from being an accidental information leak.
+	 *
+	 * This prevents access to files that are not visible do to
+	 * being on the otherside of a chroot, in a different
+	 * namespace, or are simply process local (like pipes).
+	 */
+	struct task_struct *task;
+	struct files_struct *task_files, *files;
+	int error = -EACCES;
+
+	/* See if the the two tasks share a commone set of
+	 * file descriptors.  If so everything is visible.
+	 */
+	task = proc_task(inode);
+	if (!task)
+		goto out;
+	files = get_files_struct(current);
+	task_files = get_files_struct(task);
+	if (files && task_files && (files == task_files))
+		error = 0;
+	if (task_files)
+		put_files_struct(task_files);
+	if (files)
+		put_files_struct(files);
+	if (!error)
+		goto out;
+
+	/* If the two tasks don't share a common set of file
+	 * descriptors see if the destination dentry is already
+	 * visible in the current tasks filesystem namespace.
+	 */
+	error = proc_check_chroot(dentry, mnt);
+out:
+	return error;
+
+}
+
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
@@ -1058,12 +1087,16 @@ static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 
 	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
 		goto out;
-	error = proc_check_root(inode);
-	if (error)
-		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(inode, &nd->dentry, &nd->mnt);
 	nd->last_type = LAST_BIND;
+	if (error)
+		goto out;
+
+	/* Only return files this task can already see */
+	error = proc_check_dentry_visible(inode, nd->dentry, nd->mnt);
+	if (error)
+		path_release(nd);
 out:
 	return ERR_PTR(error);
 }
@@ -1104,15 +1137,18 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
 
 	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
 		goto out;
-	error = proc_check_root(inode);
-	if (error)
-		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(inode, &de, &mnt);
 	if (error)
 		goto out;
 
+	/* Only return files this task can already see */
+	error = proc_check_dentry_visible(inode, de, mnt);
+	if (error)
+		goto out_put;
+
 	error = do_proc_readlink(de, mnt, buffer, buflen);
+out_put:
 	dput(de);
 	mntput(mnt);
 out:
@@ -1512,7 +1548,6 @@ static struct file_operations proc_task_operations = {
  */
 static struct inode_operations proc_fd_inode_operations = {
 	.lookup		= proc_lookupfd,
-	.permission	= proc_permission,
 };
 
 static struct inode_operations proc_task_inode_operations = {