[PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[Eric W. Biederman]

Since 2.2 we have been doing a chroot check to see if it is appropriate
to return a read or follow one of these magic symlinks.  The chroot
check was asking a question about the visibility of files to the
calling process and it was actually checking the destination process,
and not the files themselves.  That test was clearly bogus.

In my first pass through I simply fixed the test to check the visibility
of the files themselves.  That naive approach to fixing the permissions
was too strict and resulted in cases where a task could not even see
all of it's file descriptors.

What has disturbed me about relaxing this check is that file descriptors are
per-process private things, and they are occasionaly used a user space
capability tokens.  Looking a little farther into the symlink path on
/proc I did find userid checks and a check for capability (CAP_DAC_OVERRIDE)
so there were permissions checking this.

But I was still concerned about privacy.  Besides /proc there is only one
other way to find out this kind of information, and that is ptrace.  ptrace
has been around for a long time and it has a well established security
model.

So after thinking about it I finally realized that the permission checks
that make sense are the permission checks applied to ptrace_attach.
The checks are simple per process, and won't cause nasty surprises for
people coming from less capable unices.

Unfortunately there is one case that the current ptrace_attach test
does not cover: Zombies and kernel threads.  Single stepping those
kinds of processes is impossible.  Being able to see which file
descriptors are open on these tasks is important to lsof, fuser and
friends.  So for these special processes I made the rule you can't
find out unless you have CAP_SYS_PTRACE.

These proc permission checks should now conform to the principle of
least surprise.  As well as using much less code to implement :)

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |  123 ++++++++++++--------------------------------------------
 1 files changed, 27 insertions(+), 96 deletions(-)

64ea648d12af9f8bc0556ec118b27fbfcbe17722
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6a26847..d9be807 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -508,44 +508,33 @@ static int proc_oom_score(struct task_st
 /************************************************************************/
 
 /* permission checks */
-
-/* If the process being read is separated by chroot from the reading process,
- * don't let the reader access the threads.
- */
-static int proc_check_chroot(struct dentry *de, struct vfsmount *mnt)
+static int proc_fd_access_allowed(struct inode *inode)
 {
-	struct dentry *base;
-	struct vfsmount *our_vfsmnt;
-	int res = 0;
-
-	read_lock(&current->fs->lock);
-	our_vfsmnt = mntget(current->fs->rootmnt);
-	base = dget(current->fs->root);
-	read_unlock(&current->fs->lock);
-
-	spin_lock(&vfsmount_lock);
-
-	while (mnt != our_vfsmnt) {
-		if (mnt == mnt->mnt_parent)
-			goto out;
-		de = mnt->mnt_mountpoint;
-		mnt = mnt->mnt_parent;
+	struct task_struct *task;
+	int allowed = 0;
+	/* Allow access to a task's file descriptors if either we may
+	 * use ptrace attach to the process and find out that
+	 * information, or if the task cannot possibly be ptraced
+	 * allow access if we have the proper capability.
+	 */
+	task = get_proc_task(inode);
+	if (task) {
+		int alive;
+		task_lock(task);
+		alive = !!task->mm;
+		task_unlock(task);
+		if (alive)
+			/* For a living task obey ptrace_may_attach */
+			allowed = ptrace_may_attach(task);
+		else 
+			/* For a special task simply check the capability */
+			allowed = capable(CAP_SYS_PTRACE);
 	}
-
-	if (!is_subdir(de, base))
-		goto out;
-	spin_unlock(&vfsmount_lock);
-
-exit:
-	dput(base);
-	mntput(our_vfsmnt);
-	return res;
-out:
-	spin_unlock(&vfsmount_lock);
-	res = -EACCES;
-	goto exit;
+	put_task_struct(task);
+	return allowed;
 }
 
+
 extern struct seq_operations mounts_op;
 struct proc_mounts {
 	struct seq_file m;
@@ -1001,52 +990,6 @@ static struct file_operations proc_secco
 };
 #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;
-	int error = -EACCES;
-
-	/* See if the the two tasks share a commone set of 
-	 * file descriptors.  If so everything is visible.
-	 */
-	rcu_read_lock();
-	task = tref_task(proc_tref(inode));
-	if (task) {
-		struct files_struct *task_files, *files;
-		/* This test answeres the question:
-		 * Is there a point in time since we looked up the
-		 * file descriptor where the two tasks share the
-		 * same files struct?
-		 */
-		rmb();
-		files = current->files;
-		task_files = task->files;
-		if (files && (files == task_files))
-			error = 0;
-	}
-	rcu_read_unlock();
-	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;
@@ -1055,18 +998,12 @@ static void *proc_pid_follow_link(struct
 	/* We don't need a base pointer in the /proc filesystem */
 	path_release(nd);
 
-	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
+	/* Are we allowed to snoop on the tasks file descriptors? */
+	if (!proc_fd_access_allowed(inode))
 		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,21 +1041,15 @@ static int proc_pid_readlink(struct dent
 	struct dentry *de;
 	struct vfsmount *mnt = NULL;
 
-
-	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
+	/* Are we allowed to snoop on the tasks file descriptors? */
+	if (!proc_fd_access_allowed(inode))
 		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:
-- 
1.2.2.g709a-dirty

[PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[Sam Vilain]

From: Eric W. Biederman <ebiederm@xmission.com>

Since 2.2 we have been doing a chroot check to see if it is appropriate
to return a read or follow one of these magic symlinks.  The chroot
check was asking a question about the visibility of files to the
calling process and it was actually checking the destination process,
and not the files themselves.  That test was clearly bogus.

In my first pass through I simply fixed the test to check the visibility
of the files themselves.  That naive approach to fixing the permissions
was too strict and resulted in cases where a task could not even see
all of it's file descriptors.

What has disturbed me about relaxing this check is that file descriptors 
are per-process private things, and they are occasionaly used a user 
space capability tokens.  Looking a little farther into the symlink path 
on /proc I did find userid checks and a check for capability 
(CAP_DAC_OVERRIDE) so there were permissions checking this.

But I was still concerned about privacy.  Besides /proc there is only 
one other way to find out this kind of information, and that is ptrace. 
  ptrace has been around for a long time and it has a well established 
security model.

So after thinking about it I finally realized that the permission checks
that make sense are the permission checks applied to ptrace_attach.
The checks are simple per process, and won't cause nasty surprises for
people coming from less capable unices.

Unfortunately there is one case that the current ptrace_attach test
does not cover: Zombies and kernel threads.  Single stepping those
kinds of processes is impossible.  Being able to see which file
descriptors are open on these tasks is important to lsof, fuser and
friends.  So for these special processes I made the rule you can't
find out unless you have CAP_SYS_PTRACE.

These proc permission checks should now conform to the principle of
least surprise.  As well as using much less code to implement :)

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
Nice solution to this problem we were discussion, Eric!  I agree with 
the ptrace analogy and support it in the context of Linux-VServer.

Sorry for the missing References: header, blame Thunderbird :)

There are long lines here that have been wrapped, if your e-mail client 
does not support flowed content you will have to un-flow it before the 
patch will apply.

  fs/proc/base.c |  123 
++++++++++++--------------------------------------------
  1 files changed, 27 insertions(+), 96 deletions(-)

64ea648d12af9f8bc0556ec118b27fbfcbe17722
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6a26847..d9be807 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -508,44 +508,33 @@ static int proc_oom_score(struct task_st
  /************************************************************************/

  /* permission checks */
-
-/* If the process being read is separated by chroot from the reading 
process,
- * don't let the reader access the threads.
- */
-static int proc_check_chroot(struct dentry *de, struct vfsmount *mnt)
+static int proc_fd_access_allowed(struct inode *inode)
  {
-	struct dentry *base;
-	struct vfsmount *our_vfsmnt;
-	int res = 0;
-
-	read_lock(&current->fs->lock);
-	our_vfsmnt = mntget(current->fs->rootmnt);
-	base = dget(current->fs->root);
-	read_unlock(&current->fs->lock);
-
-	spin_lock(&vfsmount_lock);
-
-	while (mnt != our_vfsmnt) {
-		if (mnt == mnt->mnt_parent)
-			goto out;
-		de = mnt->mnt_mountpoint;
-		mnt = mnt->mnt_parent;
+	struct task_struct *task;
+	int allowed = 0;
+	/* Allow access to a task's file descriptors if either we may
+	 * use ptrace attach to the process and find out that
+	 * information, or if the task cannot possibly be ptraced
+	 * allow access if we have the proper capability.
+	 */
+	task = get_proc_task(inode);
+	if (task) {
+		int alive;
+		task_lock(task);
+		alive = !!task->mm;
+		task_unlock(task);
+		if (alive)
+			/* For a living task obey ptrace_may_attach */
+			allowed = ptrace_may_attach(task);
+		else
+			/* For a special task simply check the capability */
+			allowed = capable(CAP_SYS_PTRACE);
  	}
-
-	if (!is_subdir(de, base))
-		goto out;
-	spin_unlock(&vfsmount_lock);
-
-exit:
-	dput(base);
-	mntput(our_vfsmnt);
-	return res;
-out:
-	spin_unlock(&vfsmount_lock);
-	res = -EACCES;
-	goto exit;
+	put_task_struct(task);
+	return allowed;
  }

+
  extern struct seq_operations mounts_op;
  struct proc_mounts {
  	struct seq_file m;
@@ -1001,52 +990,6 @@ static struct file_operations proc_secco
  };
  #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;
-	int error = -EACCES;
-
-	/* See if the the two tasks share a commone set of
-	 * file descriptors.  If so everything is visible.
-	 */
-	rcu_read_lock();
-	task = tref_task(proc_tref(inode));
-	if (task) {
-		struct files_struct *task_files, *files;
-		/* This test answeres the question:
-		 * Is there a point in time since we looked up the
-		 * file descriptor where the two tasks share the
-		 * same files struct?
-		 */
-		rmb();
-		files = current->files;
-		task_files = task->files;
-		if (files && (files == task_files))
-			error = 0;
-	}
-	rcu_read_unlock();
-	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;
@@ -1055,18 +998,12 @@ static void *proc_pid_follow_link(struct
  	/* We don't need a base pointer in the /proc filesystem */
  	path_release(nd);

-	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
+	/* Are we allowed to snoop on the tasks file descriptors? */
+	if (!proc_fd_access_allowed(inode))
  		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,21 +1041,15 @@ static int proc_pid_readlink(struct dent
  	struct dentry *de;
  	struct vfsmount *mnt = NULL;

-
-	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
+	/* Are we allowed to snoop on the tasks file descriptors? */
+	if (!proc_fd_access_allowed(inode))
  		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:

Re: [PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[Eric W. Biederman]

Sam Vilain <sam@vilain.net> writes:


> Nice solution to this problem we were discussion, Eric!  I agree with the ptrace
> analogy and support it in the context of Linux-VServer.

Thanks.

I'm getting there one small step at a time.

Eric

Re: [PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[Andrew Morton]

With all your latest patches I get a big spew lateish in the boot:

EXT3-fs: INFO: recovery required on readonly filesystem.
EXT3-fs: write access will be enabled during recovery.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: recovery complete.
EXT3-fs: mounted filesystem with ordered data mode.
security:  3 users, 6 roles, 1135 types, 133 bools, 1 sens, 256 cats
security:  55 classes, 37666 rules
SELinux:  Completing initialization.
SELinux:  Setting up existing superblocks.
SELinux: initialized (dev sda6, type ext3), uses xattr
SELinux: initialized (dev tmpfs, type tmpfs), uses transition SIDs
SELinux: initialized (dev debugfs, type debugfs), uses genfs_contexts
SELinux: initialized (dev selinuxfs, type selinuxfs), uses genfs_contexts
SELinux: initialized (dev mqueue, type mqueue), uses transition SIDs
SELinux: initialized (dev hugetlbfs, type hugetlbfs), uses genfs_contexts
SELinux: initialized (dev devpts, type devpts), uses transition SIDs
SELinux: initialized (dev eventpollfs, type eventpollfs), uses genfs_contexts
SELinux: initialized (dev inotifyfs, type inotifyfs), uses genfs_contexts
SELinux: initialized (dev tmpfs, type tmpfs), uses transition SIDs
SELinux: initialized (dev futexfs, type futexfs), uses genfs_contexts
SELinux: initialized (dev pipefs, type pipefs), uses task SIDs
SELinux: initialized (dev sockfs, type sockfs), uses task SIDs
SELinux: initialized (dev proc, type proc), uses genfs_contexts
SELinux: initialized (dev bdev, type bdev), uses genfs_contexts
SELinux: initialized (dev rootfs, type rootfs), uses genfs_contexts
SELinux: initialized (dev sysfs, type sysfs), uses genfs_contexts
SELinux: initialized (dev usbfs, type usbfs), uses genfs_contexts
audit(1141341858.520:2): avc:  denied  { ptrace } for  pid=372 comm="restorecon" scontext=system_u:system_r:restorecon_t:s0 tcontext=system_u:system_r:restorecon_t:s0 tclass=process
audit(1141341858.520:3): avc:  denied  { ptrace } for  pid=372 comm="restorecon" scontext=system_u:system_r:restorecon_t:s0 tcontext=system_u:system_r:restorecon_t:s0 tclass=process
audit(1141341858.520:4): avc:  denied  { ptrace } for  pid=372 comm="restorecon" scontext=system_u:system_r:restorecon_t:s0 tcontext=system_u:system_r:restorecon_t:s0 tclass=process
audit(1141341858.520:5): avc:  denied  { ptrace } for  pid=372 comm="restorecon" scontext=system_u:system_r:restorecon_t:s0 tcontext=system_u:system_r:restorecon_t:s0 tclass=process
audit(1141341858.520:6): avc:  denied  { ptrace } for  pid=372 comm="restorecon" scontext=system_u:system_r:restorecon_t:s0 tcontext=system_u:system_r:restorecon_t:s0 tclass=process
...
audit(1141370661.947:106): avc:  denied  { ptrace } for  pid=380 comm="hwclock" scontext=system_u:system_r:hwclock_t:s0 tcontext=system_u:system_r:hwclock_t:s0 tclass=process
audit(1141370661.947:107): avc:  denied  { ptrace } for  pid=380 comm="hwclock" scontext=system_u:system_r:hwclock_t:s0 tcontext=system_u:system_r:hwclock_t:s0 tclass=process
ICH6: IDE controller at PCI slot 0000:00:1f.1
ACPI: PCI Interrupt 0000:00:1f.1[B] -> GSI 18 (level, low) -> IRQ 18
ICH6: chipset revision 3
ICH6: not 100% native mode: will probe irqs later


Reverting just this patch prevents the above.

This is with basically unaltered FC5 as of a few days ago.  The audit
patches weren't applied.

What is happening is that both `current' and get_proc_task(inode) are the
same task_struct: `restorecon' is trying to read its own proc files.  But
ptrace_may_attach()->security_ptrace() is returning -EACCES.

So I bodged it in the obvious manner:

--- devel/fs/proc/base.c~proc-use-sane-permission-checks-on-the-proc-pid-fd-fix	2006-03-03 00:38:17.000000000 -0800
+++ devel-akpm/fs/proc/base.c	2006-03-03 00:43:54.000000000 -0800
@@ -521,8 +521,11 @@ static int proc_fd_access_allowed(struct
 	 * allow access if we have the proper capability.
 	 */
 	task = get_proc_task(inode);
-	if (task) {
+	if (task == current)
+		allowed = 1;
+	if (task && !allowed) {
 		int alive;
+
 		task_lock(task);
 		alive = !!task->mm;
 		task_unlock(task);

And the messages went away.

But I have a bad feeling about these /proc permission changes, Eric.  I
suspect we'll be chasing a gradually decreasing frequency of weird problems
for a long time.


That task_lock() you have in proc_fd_access_allowed() looks very fishy,
btw.  As soon as the lock is dropped, local `alive' becomes meaningless. 
Either that, or the lock wasn't needed.


btw, it's surprising (to me) that security_ptrace(t1, t2) fails when
t1==t2?

Re: [PATCH] proc: Use sane permission checks on the /proc/<pid>/fd/ symlinks.

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> With all your latest patches I get a big spew lateish in the boot:
>
> Reverting just this patch prevents the above.
>
> This is with basically unaltered FC5 as of a few days ago.  The audit
> patches weren't applied.
>
> What is happening is that both `current' and get_proc_task(inode) are the
> same task_struct: `restorecon' is trying to read its own proc files.  But
> ptrace_may_attach()->security_ptrace() is returning -EACCES.
>
> So I bodged it in the obvious manner:

Ugh.  Unless I am misreading the code we have the exact same problem
with /proc/self/mem.  Which uses a stricter form of the same test.

> btw, it's surprising (to me) that security_ptrace(t1, t2) fails when
> t1==t2?

Same here.  ptrace_attach explicitly denies that case but that happens
outside of ptrace_may_attach to allow for the /proc usage.

> --- devel/fs/proc/base.c~proc-use-sane-permission-checks-on-the-proc-pid-fd-fix
> 2006-03-03 00:38:17.000000000 -0800
> +++ devel-akpm/fs/proc/base.c	2006-03-03 00:43:54.000000000 -0800
> @@ -521,8 +521,11 @@ static int proc_fd_access_allowed(struct
>  	 * allow access if we have the proper capability.
>  	 */
>  	task = get_proc_task(inode);
> -	if (task) {
> +	if (task == current)
> +		allowed = 1;
> +	if (task && !allowed) {
>  		int alive;
> +
>  		task_lock(task);
>  		alive = !!task->mm;
>  		task_unlock(task);
>
> And the messages went away.

Agreed.  Examining your self should always be valid.

I suspect this check get put in ptrace_may_attach, so we can always
read /proc/self/mem.

> But I have a bad feeling about these /proc permission changes, Eric.  I
> suspect we'll be chasing a gradually decreasing frequency of weird problems
> for a long time.

You may be right, but even if that is the case I think it is worth it.
The old checks were bizarre if not outright wrong.  And cause their
own share of weird cases when you start using things like namespaces.

The checks unify the two cases where we allow violating a tasks
privacy.  Which is an important case to get right, and unification
of permissions means it is easier to administer if you don't want
someone doing that.

/proc and ptrace have had a connection since at least 2.0 so this
is nothing new.  So if by using this on more than just /proc/<pid>/mem
I increase the frequency of usage and thus increase the frequency
of problem detection I think it is a good thing.

I think it is actually surprising no one noticed how bogus the
permissions on /proc/<pid>/fd/<fd#> have been, especially the people
concerned with security.

> That task_lock() you have in proc_fd_access_allowed() looks very fishy,
> btw.  As soon as the lock is dropped, local `alive' becomes meaningless. 
> Either that, or the lock wasn't needed.

Agreed.  That does look a fishy. 

I also spotted an outright bug. put_task_struct does not work on a
NULL pointer. Duh.

Sorry I obviously didn't switch levels very well when auditing that
code.

I will send in a patch to clean up those bits.

Eric