bug report: mem_write
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.
Quoted 11 lines · 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.
Is there any reason copied shouldn't get set to 0 just prior to entering the while loop?
Quoted 7 lines
Amnon.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
PATCH 1/1 fix mem_write return value (was: Re: bug report: mem_write)
In reply to Amnon ShilohQuoted 10 lines · 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.
True, it looks like the faulty commit is: de7587343bfebc186995ad294e3de0da382eb9bc
The attached patch should fix it. Maybe that should go to 2.6.18. Thanks for the bug report,
Frederik
proc/base.c renamed from proc/base.c.orig +2 -1
@@ -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)
Quoted 7 lines · Frederik Deweerdt
Quoted 6 lines · 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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 4 lines
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.
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.
Quoted 23 lines
Frederik
Signed-off-by Frederik Deweerdt <[email protected]>proc/base.c renamed from proc/base.c.orig +2
-1@@ -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)
Quoted 19 lines · Eric W. Biederman
Quoted 7 lines · Frederik Deweerdt
Quoted 6 lines · 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.Quoted 6 lines
Quoted 4 lines
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:
de7587343bfebc186995ad294e3de0da382eb9bcActually it was:
99f895518368252ba862cc15ce4eb98ebbe1bec6Which is what you url points to, odd.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 11 lines
Signed-off-by Eric Biederman <[email protected]>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)
Quoted 46 lines · Eric W. Biederman
Quoted 19 lines
Quoted 7 lines · Frederik Deweerdt
Quoted 6 lines · 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.Quoted 6 lines
Quoted 4 lines
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:
de7587343bfebc186995ad294e3de0da382eb9bcActually it was:
99f895518368252ba862cc15ce4eb98ebbe1bec6Which is what you url points to, odd.Quoted 7 lines
Quoted 4 lines
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.
Quoted 11 lines
Signed-off-by Eric Biederman <[email protected]>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.
Quoted 3 lines
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.