bug report: mem_write

bug report: mem_write

[Amnon Shiloh]

Hi,

Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
but I need to use it anyway (as super-user only), and find it broken,
somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.

The point is that in the beginning of the routine, "copied" is set to 0,
but it is no good because in lines 805 and 812 it is set to other values.
Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
it actually did.

Amnon.

Re: bug report: mem_write

[Gerard J Snitselaar]

On  0, Amnon Shiloh <amnons@cs.huji.ac.il> wrote:
> Hi,
> 
> Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
> but I need to use it anyway (as super-user only), and find it broken,
> somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.
> 
> The point is that in the beginning of the routine, "copied" is set to 0,
> but it is no good because in lines 805 and 812 it is set to other values.
> Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
> it actually did.
> 
Is there any reason copied shouldn't get set to 0 just prior to entering
the while loop?

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

[2.6.18 patch] fix mem_write return value (was: Re: bug report: mem_write)

[Frederik Deweerdt]

On Thu, Aug 24, 2006 at 11:25:37AM +0300, Amnon Shiloh wrote:
> Hi,
> 
> Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
> but I need to use it anyway (as super-user only), and find it broken,
> somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.
> 
> The point is that in the beginning of the routine, "copied" is set to 0,
> but it is no good because in lines 805 and 812 it is set to other values.
> Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
> it actually did.
True, it looks like the faulty commit is: de7587343bfebc186995ad294e3de0da382eb9bc

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=99f895518368252ba862cc15ce4eb98ebbe1bec6;hp=8578cea7509cbdec25b31d08b48a92fcc3b1a9e3

The attached patch should fix it. Maybe that should go to 2.6.18.
Thanks for the bug report,
Frederik

Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>

--- fs/proc/base.c.orig	2006-08-24 13:57:22.000000000 +0200
+++ fs/proc/base.c	2006-08-24 13:57:10.000000000 +0200
@@ -797,7 +797,7 @@
 static ssize_t mem_write(struct file * file, const char * buf,
 			 size_t count, loff_t *ppos)
 {
-	int copied = 0;
+	int copied;
 	char *page;
 	struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
 	unsigned long dst = *ppos;
@@ -814,6 +814,7 @@
 	if (!page)
 		goto out;
 
+	copied = 0;
 	while (count > 0) {
 		int this_len, retval;
 

Re: [2.6.18 patch] fix mem_write return value (was: Re: bug report: mem_write)

[Eric W. Biederman]

Frederik Deweerdt <deweerdt@free.fr> writes:

> On Thu, Aug 24, 2006 at 11:25:37AM +0300, Amnon Shiloh wrote:
>> Hi,
>> 
>> Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
>> but I need to use it anyway (as super-user only), and find it broken,
>> somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.
>> 
>> The point is that in the beginning of the routine, "copied" is set to 0,
>> but it is no good because in lines 805 and 812 it is set to other values.
>> Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
>> it actually did.
> True, it looks like the faulty commit is:
> de7587343bfebc186995ad294e3de0da382eb9bc

Actually it was: 99f895518368252ba862cc15ce4eb98ebbe1bec6
Which is what you url points to, odd.

> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=99f895518368252ba862cc15ce4eb98ebbe1bec6;hp=8578cea7509cbdec25b31d08b48a92fcc3b1a9e3
>
> The attached patch should fix it. Maybe that should go to 2.6.18.
> Thanks for the bug report,

The patch looks correct.  Although this won't cause anyone problems as the code
is disabled.

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

As for enabling this.  I believe we need an extra permission check just before
we copy the data from our temporary buffer to the target task, to ensure
nothing has changed.  The history does not really capture why this code
was disabled, but before this gets enabled I would like to understand more
than just the comment.  I believe with a little care this can be safely enabled
as it doesn't let you do anything ptrace wouldn't do, and it should let you do
it anytime except when ptrace would allow it.  Thus not introducing any new
security holes.

> Frederik
>
> Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>
>
> --- fs/proc/base.c.orig	2006-08-24 13:57:22.000000000 +0200
> +++ fs/proc/base.c	2006-08-24 13:57:10.000000000 +0200
> @@ -797,7 +797,7 @@
>  static ssize_t mem_write(struct file * file, const char * buf,
>  			 size_t count, loff_t *ppos)
>  {
> -	int copied = 0;
> +	int copied;
>  	char *page;
>  	struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
>  	unsigned long dst = *ppos;
> @@ -814,6 +814,7 @@
>  	if (!page)
>  		goto out;
>  
> +	copied = 0;
>  	while (count > 0) {
>  		int this_len, retval;
>  

Re: [2.6.18 patch] fix mem_write return value (was: Re: bug report: mem_write)

[Frederik Deweerdt]

On Thu, Aug 24, 2006 at 10:33:20AM -0600, Eric W. Biederman wrote:
> Frederik Deweerdt <deweerdt@free.fr> writes:
> 
> > On Thu, Aug 24, 2006 at 11:25:37AM +0300, Amnon Shiloh wrote:
> >> Hi,
> >> 
> >> Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
> >> but I need to use it anyway (as super-user only), and find it broken,
> >> somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.
> >> 
> >> The point is that in the beginning of the routine, "copied" is set to 0,
> >> but it is no good because in lines 805 and 812 it is set to other values.
> >> Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
> >> it actually did.
> > True, it looks like the faulty commit is:
> > de7587343bfebc186995ad294e3de0da382eb9bc
> 
> Actually it was: 99f895518368252ba862cc15ce4eb98ebbe1bec6
> Which is what you url points to, odd.
> 
> > http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=99f895518368252ba862cc15ce4eb98ebbe1bec6;hp=8578cea7509cbdec25b31d08b48a92fcc3b1a9e3
> >
> > The attached patch should fix it. Maybe that should go to 2.6.18.
> > Thanks for the bug report,
> 
> The patch looks correct.  Although this won't cause anyone problems as the code
> is disabled.
Right, I missed this, so this is really not urgent.
> 
> Signed-off-by: Eric Biederman <ebiederm@xmission.com>
> 
> As for enabling this.  I believe we need an extra permission check just before
> we copy the data from our temporary buffer to the target task, to ensure
> nothing has changed.  The history does not really capture why this code
> was disabled, but before this gets enabled I would like to understand more
> than just the comment.  I believe with a little care this can be safely enabled
> as it doesn't let you do anything ptrace wouldn't do, and it should let you do
> it anytime except when ptrace would allow it.  Thus not introducing any new
> security holes.
I've found two interesting links on that:
http://lkml.org/lkml/2006/3/10/224
and
http://www.google.com/search?q=cache:4y8MWSuHOpIJ:files.security-protocols.com/kernelhacking/procpidmem.pdf&hl=en&ct=clnk&cd=3&client=firefox-a
The second one in particular goes in great detail on why the author
thinks this is dangerous, and what could be done to re-enable it.

Regards,
Frederik

Re: [2.6.18 patch] fix mem_write return value (was: Re: bug report: mem_write)

[Amnon Shiloh]

> On Thu, Aug 24, 2006 at 10:33:20AM -0600, Eric W. Biederman wrote:
> > Frederik Deweerdt <deweerdt@free.fr> writes:
> > 
> > > On Thu, Aug 24, 2006 at 11:25:37AM +0300, Amnon Shiloh wrote:
> > >> Hi,
> > >> 
> > >> Alright, I know that "mem_write" (fs/proc/base.c) is a "security hazard",
> > >> but I need to use it anyway (as super-user only), and find it broken,
> > >> somewhere between Linux-2.6.17 and Linux-2.6.18-rc4.
> > >> 
> > >> The point is that in the beginning of the routine, "copied" is set to 0,
> > >> but it is no good because in lines 805 and 812 it is set to other values.
> > >> Finally, the routine returns as if it copied 12 (=ENOMEM) bytes less than
> > >> it actually did.
> > > True, it looks like the faulty commit is:
> > > de7587343bfebc186995ad294e3de0da382eb9bc
> > 
> > Actually it was: 99f895518368252ba862cc15ce4eb98ebbe1bec6
> > Which is what you url points to, odd.
> > 
> > > http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=99f895518368252ba862cc15ce4eb98ebbe1bec6;hp=8578cea7509cbdec25b31d08b48a92fcc3b1a9e3
> > >
> > > The attached patch should fix it. Maybe that should go to 2.6.18.
> > > Thanks for the bug report,
> > 
> > The patch looks correct.  Although this won't cause anyone problems as the code
> > is disabled.
> Right, I missed this, so this is really not urgent.
> > 
> > Signed-off-by: Eric Biederman <ebiederm@xmission.com>
> > 
> > As for enabling this.  I believe we need an extra permission check just before
> > we copy the data from our temporary buffer to the target task, to ensure
> > nothing has changed.  The history does not really capture why this code
> > was disabled, but before this gets enabled I would like to understand more
> > than just the comment.  I believe with a little care this can be safely enabled
> > as it doesn't let you do anything ptrace wouldn't do, and it should let you do
> > it anytime except when ptrace would allow it.  Thus not introducing any new
> > security holes.
> I've found two interesting links on that:
> http://lkml.org/lkml/2006/3/10/224
> and
> http://www.google.com/search?q=cache:4y8MWSuHOpIJ:files.security-protocols.com/kernelhacking/procpidmem.pdf&hl=en&ct=clnk&cd=3&client=firefox-a
> The second one in particular goes in great detail on why the author
> thinks this is dangerous, and what could be done to re-enable it.
> 
> Regards,
> Frederik
> 

I am aware of those risks, but since I desparately need this feature
and the program that needs it is SETUID-root anyway, I have it enabled
but added a test to make sure that only root can use it.

It works well and I can see no reason on earth how this could be a
security hazard when only called by the super-user.

Regards,
Amnon.