[PATCH] supress uid comparison test if core output files are pipes

[PATCH] supress uid comparison test if core output files are pipes

[Neil Horman]

Modify uid check in do_coredump so as to not apply it in the case of pipes

So this just got noticed in testing.  The end of do_coredump validates the uid
of the inode for the created file against the uid of the crashing process to
ensure that no one can pre-create a core file with different ownership and grab
the information contained in the core when they shouldn' tbe able to.  This
causes failures when using pipes for a core dumps if the crashing process is not
root, which is the uid of the pipe when it is created.

The fix is simple.  Since the check for matching uid's isn't relevant for pipes
(a process can't create a pipe that the uermodehelper code will open anyway), we
can just just skip it in the event ispipe is non-zero

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 exec.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index 6303d18..6af2214 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 	/*
 	 * Dont allow local users get cute and trick others to coredump
 	 * into their pre-created files:
+	 * Note, this is not relevant for pipes
 	 */
-	if (inode->i_uid != current_fsuid())
+	if (!ispipe && (inode->i_uid != current_fsuid()))
 		goto close_fail;
 	if (!cprm.file->f_op)
 		goto close_fail;

Re: [PATCH] supress uid comparison test if core output files are pipes

[Oleg Nesterov]

On 02/22, Neil Horman wrote:
>
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  	/*
>  	 * Dont allow local users get cute and trick others to coredump
>  	 * into their pre-created files:
> +	 * Note, this is not relevant for pipes
>  	 */
> -	if (inode->i_uid != current_fsuid())
> +	if (!ispipe && (inode->i_uid != current_fsuid()))
>  		goto close_fail;

Ah. This is because the previous recursion-check moved create_write_pipe()
from current's context to kthread's context, right?

Looks like a right (and "must have") fix for recent -mm changes to me.



This also reminds me do_coredump() asks for cleanup. I'll try to redo/resend
my old cleanup patches on top of your changes.

Oleg.

Re: [PATCH] supress uid comparison test if core output files are pipes

[Neil Horman]

On Wed, Feb 24, 2010 at 12:09:03PM +0100, Oleg Nesterov wrote:
> On 02/22, Neil Horman wrote:
> >
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> >  	/*
> >  	 * Dont allow local users get cute and trick others to coredump
> >  	 * into their pre-created files:
> > +	 * Note, this is not relevant for pipes
> >  	 */
> > -	if (inode->i_uid != current_fsuid())
> > +	if (!ispipe && (inode->i_uid != current_fsuid()))
> >  		goto close_fail;
> 
> Ah. This is because the previous recursion-check moved create_write_pipe()
> from current's context to kthread's context, right?
> 
Yes, thats correct.

> Looks like a right (and "must have") fix for recent -mm changes to me.
> 
Also correct.

> 
> 
> This also reminds me do_coredump() asks for cleanup. I'll try to redo/resend
> my old cleanup patches on top of your changes.
> 
Cool, thanks!
Neil

> Oleg.
> 
> 

Re: [PATCH] supress uid comparison test if core output files are pipes

[Andrew Morton]

On Mon, 22 Feb 2010 15:44:29 -0500 Neil Horman <nhorman@tuxdriver.com> wrote:

> Modify uid check in do_coredump so as to not apply it in the case of pipes
> 
> So this just got noticed in testing.  The end of do_coredump validates the uid
> of the inode for the created file against the uid of the crashing process to
> ensure that no one can pre-create a core file with different ownership and grab
> the information contained in the core when they shouldn' tbe able to.  This
> causes failures when using pipes for a core dumps if the crashing process is not
> root, which is the uid of the pipe when it is created.
> 
> The fix is simple.  Since the check for matching uid's isn't relevant for pipes
> (a process can't create a pipe that the uermodehelper code will open anyway), we
> can just just skip it in the event ispipe is non-zero
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  exec.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 6303d18..6af2214 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
>  	/*
>  	 * Dont allow local users get cute and trick others to coredump
>  	 * into their pre-created files:
> +	 * Note, this is not relevant for pipes
>  	 */
> -	if (inode->i_uid != current_fsuid())
> +	if (!ispipe && (inode->i_uid != current_fsuid()))
>  		goto close_fail;
>  	if (!cprm.file->f_op)
>  		goto close_fail;

hm, this actually appears to fix a regression, added by:



commit c46f739dd39db3b07ab5deb4e3ec81e1c04a91af
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Wed Nov 28 13:59:18 2007 +0100
Commit:     Linus Torvalds <torvalds@woody.linux-foundation.org>
CommitDate: Wed Nov 28 10:58:01 2007 -0800

    vfs: coredumping fix
    
    fix: http://bugzilla.kernel.org/show_bug.cgi?id=3043
    
    only allow coredumping to the same uid that the coredumping
    task runs under.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Acked-by: Alan Cox <alan@redhat.com>
    Acked-by: Christoph Hellwig <hch@lst.de>
    Acked-by: Al Viro <viro@ftp.linux.org.uk>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/exec.c b/fs/exec.c
index 4ccaaa4..282240a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1780,6 +1780,12 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	   but keep the previous behaviour for now. */
 	if (!ispipe && !S_ISREG(inode->i_mode))
 		goto close_fail;
+	/*
+	 * Dont allow local users get cute and trick others to coredump
+	 * into their pre-created files:
+	 */
+	if (inode->i_uid != current->fsuid)
+		goto close_fail;
 	if (!file->f_op)
 		goto close_fail;
 	if (!file->f_op->write)

Re: [PATCH] supress uid comparison test if core output files are pipes

[Neil Horman]

On Wed, Feb 24, 2010 at 01:50:53PM -0800, Andrew Morton wrote:
> On Mon, 22 Feb 2010 15:44:29 -0500 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > Modify uid check in do_coredump so as to not apply it in the case of pipes
> > 
> > So this just got noticed in testing.  The end of do_coredump validates the uid
> > of the inode for the created file against the uid of the crashing process to
> > ensure that no one can pre-create a core file with different ownership and grab
> > the information contained in the core when they shouldn' tbe able to.  This
> > causes failures when using pipes for a core dumps if the crashing process is not
> > root, which is the uid of the pipe when it is created.
> > 
> > The fix is simple.  Since the check for matching uid's isn't relevant for pipes
> > (a process can't create a pipe that the uermodehelper code will open anyway), we
> > can just just skip it in the event ispipe is non-zero
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > 
> >  exec.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 6303d18..6af2214 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> >  	/*
> >  	 * Dont allow local users get cute and trick others to coredump
> >  	 * into their pre-created files:
> > +	 * Note, this is not relevant for pipes
> >  	 */
> > -	if (inode->i_uid != current_fsuid())
> > +	if (!ispipe && (inode->i_uid != current_fsuid()))
> >  		goto close_fail;
> >  	if (!cprm.file->f_op)
> >  		goto close_fail;
> 
> hm, this actually appears to fix a regression, added by:
> 
In a sense yes.  Previously though, Ingos check was both useless for pipes
(since you can't have a user create a pipe that will map to what the coredump
path creates anyway, making this check useless), but it worked anyway, since the
crashing process created the pipe, so the uid check always matched.  With the
refactoring andi and I did, thats no longer true, so the check could fail.  This
patch just fixes it up by recongnizing that the check isn't needed at all for
pipes

Regards
Neil

> 
> 
> commit c46f739dd39db3b07ab5deb4e3ec81e1c04a91af
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Wed Nov 28 13:59:18 2007 +0100
> Commit:     Linus Torvalds <torvalds@woody.linux-foundation.org>
> CommitDate: Wed Nov 28 10:58:01 2007 -0800
> 
>     vfs: coredumping fix
>     
>     fix: http://bugzilla.kernel.org/show_bug.cgi?id=3043
>     
>     only allow coredumping to the same uid that the coredumping
>     task runs under.
>     
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
>     Acked-by: Alan Cox <alan@redhat.com>
>     Acked-by: Christoph Hellwig <hch@lst.de>
>     Acked-by: Al Viro <viro@ftp.linux.org.uk>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 4ccaaa4..282240a 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1780,6 +1780,12 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
>  	   but keep the previous behaviour for now. */
>  	if (!ispipe && !S_ISREG(inode->i_mode))
>  		goto close_fail;
> +	/*
> +	 * Dont allow local users get cute and trick others to coredump
> +	 * into their pre-created files:
> +	 */
> +	if (inode->i_uid != current->fsuid)
> +		goto close_fail;
>  	if (!file->f_op)
>  		goto close_fail;
>  	if (!file->f_op->write)
> 
> 

Re: [PATCH] supress uid comparison test if core output files are pipes

[Andrew Morton]

On Wed, 24 Feb 2010 20:32:10 -0500 Neil Horman <nhorman@tuxdriver.com> wrote:

> > > diff --git a/fs/exec.c b/fs/exec.c
> > > index 6303d18..6af2214 100644
> > > --- a/fs/exec.c
> > > +++ b/fs/exec.c
> > > @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> > >  	/*
> > >  	 * Dont allow local users get cute and trick others to coredump
> > >  	 * into their pre-created files:
> > > +	 * Note, this is not relevant for pipes
> > >  	 */
> > > -	if (inode->i_uid != current_fsuid())
> > > +	if (!ispipe && (inode->i_uid != current_fsuid()))
> > >  		goto close_fail;
> > >  	if (!cprm.file->f_op)
> > >  		goto close_fail;
> > 
> > hm, this actually appears to fix a regression, added by:
> > 
> In a sense yes.  Previously though, Ingos check was both useless for pipes
> (since you can't have a user create a pipe that will map to what the coredump
> path creates anyway, making this check useless), but it worked anyway, since the
> crashing process created the pipe, so the uid check always matched.  With the
> refactoring andi and I did, thats no longer true, so the check could fail.  This
> patch just fixes it up by recongnizing that the check isn't needed at all for
> pipes

Which refactoring?  Please identify precisely the patch which this patch fixes up?

Re: [PATCH] supress uid comparison test if core output files are pipes

[Neil Horman]

On Wed, Feb 24, 2010 at 06:13:32PM -0800, Andrew Morton wrote:
> On Wed, 24 Feb 2010 20:32:10 -0500 Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > index 6303d18..6af2214 100644
> > > > --- a/fs/exec.c
> > > > +++ b/fs/exec.c
> > > > @@ -1987,8 +1987,9 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
> > > >  	/*
> > > >  	 * Dont allow local users get cute and trick others to coredump
> > > >  	 * into their pre-created files:
> > > > +	 * Note, this is not relevant for pipes
> > > >  	 */
> > > > -	if (inode->i_uid != current_fsuid())
> > > > +	if (!ispipe && (inode->i_uid != current_fsuid()))
> > > >  		goto close_fail;
> > > >  	if (!cprm.file->f_op)
> > > >  		goto close_fail;
> > > 
> > > hm, this actually appears to fix a regression, added by:
> > > 
> > In a sense yes.  Previously though, Ingos check was both useless for pipes
> > (since you can't have a user create a pipe that will map to what the coredump
> > path creates anyway, making this check useless), but it worked anyway, since the
> > crashing process created the pipe, so the uid check always matched.  With the
> > refactoring andi and I did, thats no longer true, so the check could fail.  This
> > patch just fixes it up by recongnizing that the check isn't needed at all for
> > pipes
> 
> Which refactoring?  Please identify precisely the patch which this patch fixes up?
> 

kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch

Sorry, when I say refactoring, I was referring to the addition of the init and
cleanup functions that Andi and I added a few weeks ago.
Neil