[RESEND][PATCH] Let even non-dumpable tasks access /proc/self/fd

[RESEND][PATCH] Let even non-dumpable tasks access /proc/self/fd

[Petr Baudis]

All tasks calling setuid() from root to non-root during their lifetime
will not be able to access their /proc/self/fd.  This is troublesome
because the fstatat() and other *at() routines are emulated by accessing
/proc/self/fd/*/path and that will break with setuid()ing programs,
leading to various weird consequences (e.g. with the latest glibc,
nftw() does not work with setuid()ing programs on ppc and furthermore
causes the LSB testsuite to fail because of this).

This kernel patch fixes the problem by letting the process access its
own /proc/self/fd - as far as I can see, this should be reasonably safe
since for the process, this does not reveal "anything new". Feel free to
comment on this.

Signed-off-by: Petr Baudis <pasky@suse.cz>

---

So far it's got one positive review on LKML and not much attention
otherwise; Pavel Machek hinted me to steer it your way...

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6cc77dc..ea36a25 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1368,7 +1368,9 @@ static struct inode *proc_pid_make_inode
 	ei->type = ino;
 	inode->i_uid = 0;
 	inode->i_gid = 0;
-	if (ino == PROC_TGID_INO || ino == PROC_TID_INO || task_dumpable(task)) {
+	if (ino == PROC_TGID_INO || ino == PROC_TID_INO ||
+	    ((ino == PROC_TGID_FD || ino == PROC_TID_FD || ino >= PROC_TID_FD_DIR) && task == current) ||
+	    task_dumpable(task)) {
 		inode->i_uid = task->euid;
 		inode->i_gid = task->egid;
 	}
@@ -1398,7 +1400,9 @@ static int pid_revalidate(struct dentry 
 	struct inode *inode = dentry->d_inode;
 	struct task_struct *task = proc_task(inode);
 	if (pid_alive(task)) {
-		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO || task_dumpable(task)) {
+		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO ||
+		    ((proc_type(inode) == PROC_TGID_FD || proc_type(inode) == PROC_TID_FD) && task == current) ||
+		     task_dumpable(task)) {
 			inode->i_uid = task->euid;
 			inode->i_gid = task->egid;
 		} else {
@@ -1425,7 +1429,7 @@ static int tid_fd_revalidate(struct dent
 		if (fcheck_files(files, fd)) {
 			rcu_read_unlock();
 			put_files_struct(files);
-			if (task_dumpable(task)) {
+			if (task_dumpable(task) || task == current) {
 				inode->i_uid = task->euid;
 				inode->i_gid = task->egid;
 			} else {


-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Right now I am having amnesia and deja-vu at the same time.  I think
I have forgotten this before.

Re: [RESEND][PATCH] Let even non-dumpable tasks access /proc/self/fd

[Andrew Morton]

On Fri, 16 Jun 2006 14:41:57 +0200
Petr Baudis <pasky@suse.cz> wrote:

> All tasks calling setuid() from root to non-root during their lifetime
> will not be able to access their /proc/self/fd.  This is troublesome
> because the fstatat() and other *at() routines are emulated by accessing
> /proc/self/fd/*/path and that will break with setuid()ing programs,
> leading to various weird consequences (e.g. with the latest glibc,
> nftw() does not work with setuid()ing programs on ppc and furthermore
> causes the LSB testsuite to fail because of this).

Odd. Did something actually change in glibc to make this start happening?

> This kernel patch fixes the problem by letting the process access its
> own /proc/self/fd - as far as I can see, this should be reasonably safe
> since for the process, this does not reveal "anything new". Feel free to
> comment on this.
> 

Eric, Chris - any thought on this one?

Thanks.

> 
> So far it's got one positive review on LKML and not much attention
> otherwise; Pavel Machek hinted me to steer it your way...
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6cc77dc..ea36a25 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1368,7 +1368,9 @@ static struct inode *proc_pid_make_inode
>  	ei->type = ino;
>  	inode->i_uid = 0;
>  	inode->i_gid = 0;
> -	if (ino == PROC_TGID_INO || ino == PROC_TID_INO || task_dumpable(task)) {
> +	if (ino == PROC_TGID_INO || ino == PROC_TID_INO ||
> +	    ((ino == PROC_TGID_FD || ino == PROC_TID_FD || ino >= PROC_TID_FD_DIR) && task == current) ||
> +	    task_dumpable(task)) {
>  		inode->i_uid = task->euid;
>  		inode->i_gid = task->egid;
>  	}
> @@ -1398,7 +1400,9 @@ static int pid_revalidate(struct dentry 
>  	struct inode *inode = dentry->d_inode;
>  	struct task_struct *task = proc_task(inode);
>  	if (pid_alive(task)) {
> -		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO || task_dumpable(task)) {
> +		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO ||
> +		    ((proc_type(inode) == PROC_TGID_FD || proc_type(inode) == PROC_TID_FD) && task == current) ||
> +		     task_dumpable(task)) {
>  			inode->i_uid = task->euid;
>  			inode->i_gid = task->egid;
>  		} else {
> @@ -1425,7 +1429,7 @@ static int tid_fd_revalidate(struct dent
>  		if (fcheck_files(files, fd)) {
>  			rcu_read_unlock();
>  			put_files_struct(files);
> -			if (task_dumpable(task)) {
> +			if (task_dumpable(task) || task == current) {
>  				inode->i_uid = task->euid;
>  				inode->i_gid = task->egid;
>  			} else {
> 
> 
> -- 
> 				Petr "Pasky" Baudis
> Stuff: http://pasky.or.cz/
> Right now I am having amnesia and deja-vu at the same time.  I think
> I have forgotten this before.

Re: [RESEND][PATCH] Let even non-dumpable tasks access /proc/self/fd

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> On Fri, 16 Jun 2006 14:41:57 +0200
> Petr Baudis <pasky@suse.cz> wrote:
>
>> All tasks calling setuid() from root to non-root during their lifetime
>> will not be able to access their /proc/self/fd.  This is troublesome
>> because the fstatat() and other *at() routines are emulated by accessing
>> /proc/self/fd/*/path and that will break with setuid()ing programs,
>> leading to various weird consequences (e.g. with the latest glibc,
>> nftw() does not work with setuid()ing programs on ppc and furthermore
>> causes the LSB testsuite to fail because of this).
>
> Odd. Did something actually change in glibc to make this start happening?
>
>> This kernel patch fixes the problem by letting the process access its
>> own /proc/self/fd - as far as I can see, this should be reasonably safe
>> since for the process, this does not reveal "anything new". Feel free to
>> comment on this.
>> 
>
> Eric, Chris - any thought on this one?

This can't fix the glibc emulation problem.  As the kernel
this patch would apply to doesn't need emulation.

The basic goal of allowing the current process to access it's
own proc directories is reasonable.

I don't like the implementation. It is not obvious that this case
applies just to the current process.

I admit that any permission checking in /proc that happens at
open time instead of at access time is buggy but that is
the best we have right now.

Anything more requires a very close review.


Eric

Re: [RESEND][PATCH] Let even non-dumpable tasks access /proc/self/fd

[Jeff Layton]

On Tue, 2006-06-20 at 00:24 -0600, Eric W. Biederman wrote:
> Andrew Morton <akpm@osdl.org> writes:
> 
> > On Fri, 16 Jun 2006 14:41:57 +0200
> > Petr Baudis <pasky@suse.cz> wrote:
> >
> >> All tasks calling setuid() from root to non-root during their lifetime
> >> will not be able to access their /proc/self/fd.  This is troublesome
> >> because the fstatat() and other *at() routines are emulated by accessing
> >> /proc/self/fd/*/path and that will break with setuid()ing programs,
> >> leading to various weird consequences (e.g. with the latest glibc,
> >> nftw() does not work with setuid()ing programs on ppc and furthermore
> >> causes the LSB testsuite to fail because of this).
> >
> > Odd. Did something actually change in glibc to make this start happening?
> >
> >> This kernel patch fixes the problem by letting the process access its
> >> own /proc/self/fd - as far as I can see, this should be reasonably safe
> >> since for the process, this does not reveal "anything new". Feel free to
> >> comment on this.
> >> 
> >
> > Eric, Chris - any thought on this one?
> 
> This can't fix the glibc emulation problem.  As the kernel
> this patch would apply to doesn't need emulation.
> 
> The basic goal of allowing the current process to access it's
> own proc directories is reasonable.
> 
> I don't like the implementation. It is not obvious that this case
> applies just to the current process.
> 
> I admit that any permission checking in /proc that happens at
> open time instead of at access time is buggy but that is
> the best we have right now.
> 
> Anything more requires a very close review.
> 
> 
> Eric

This might be a stupid question, but can someone explain to me why all
of this whole thing is keyed off of the dumpable flag? With the current
scheme setting suid_dumpable to true works around this problem, but that
doesn't seem like it's an intended behavior.

Also, I recently had someone report a problem where a program that
dropped its privileges was not able access /proc/self/maps. So making so
that we can access /proc/self/fd might not be sufficient.

Perhaps we should consider making this use a blacklist instead of a
whitelist approach? That is to make pid_revalidate to change the
ownership of anything under /proc/self to the euid irrespective of the
dumpable flag, except for things that we know need to retain the
previous ownership?

-- Jeff