[PATCH] Enable writing to /proc/PID/mem.

[PATCH] Enable writing to /proc/PID/mem.

[Stephen Wilson]

For a long time /proc/PID/mem has provided a read-only interface, at least since
2.4.0.  However, a write capability has existed "forever" in tree via the
function mem_write, disabled with an #ifdef along with the comment "this is a
security hazard".  Charles Wright, back in 2006, gave some history on the
subject:

    http://lkml.org/lkml/2006/3/10/224

Later, in commit 638fa202c, Roland McGrath updated mem_write to call
check_mem_permission which ensures an identical security policy for
/proc/PID/mem as for ptrace().  IOW, the proc interface provides a simpler, more
efficient, but otherwise equivalent mechanism for probing a processes memory as
available via ptrace.

There is no longer a security hazard and the world can safely use read/write
instead of ptrace PEEK/POKE's.  Remove the #ifdef.

Signed-off-by: Stephen Wilson <wilsons@start.ca>
---
 fs/proc/base.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9d096e8..70fc4db 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -829,10 +829,6 @@ out_no_task:
 	return ret;
 }
 
-#define mem_write NULL
-
-#ifndef mem_write
-/* This is a security hazard */
 static ssize_t mem_write(struct file * file, const char __user *buf,
 			 size_t count, loff_t *ppos)
 {
@@ -880,7 +876,6 @@ out:
 out_no_task:
 	return copied;
 }
-#endif
 
 loff_t mem_lseek(struct file *file, loff_t offset, int orig)
 {
-- 
1.7.3.5

Re: [PATCH] Enable writing to /proc/PID/mem.

[KOSAKI Motohiro]

> For a long time /proc/PID/mem has provided a read-only interface, at least since
> 2.4.0.  However, a write capability has existed "forever" in tree via the
> function mem_write, disabled with an #ifdef along with the comment "this is a
> security hazard".  Charles Wright, back in 2006, gave some history on the
> subject:
> 
>     http://lkml.org/lkml/2006/3/10/224
> 
> Later, in commit 638fa202c, Roland McGrath updated mem_write to call
> check_mem_permission which ensures an identical security policy for
> /proc/PID/mem as for ptrace().  IOW, the proc interface provides a simpler, more
> efficient, but otherwise equivalent mechanism for probing a processes memory as
> available via ptrace.
> 
> There is no longer a security hazard and the world can safely use read/write
> instead of ptrace PEEK/POKE's.  Remove the #ifdef.
> 
> Signed-off-by: Stephen Wilson <wilsons@start.ca>

I haven't found any problem in this patch. But, I really believe we need
to understand why it was marked "security hazard". Al, I guess you know it,
right? So, can you please talk us your mention?

Re: [PATCH] Enable writing to /proc/PID/mem.

[Stephen Wilson]

On Thu, Mar 03, 2011 at 11:22:59AM +0900, KOSAKI Motohiro wrote:
> > For a long time /proc/PID/mem has provided a read-only interface, at least since
> > 2.4.0.  However, a write capability has existed "forever" in tree via the
> > function mem_write, disabled with an #ifdef along with the comment "this is a
> > security hazard".  Charles Wright, back in 2006, gave some history on the
> > subject:
> > 
> >     http://lkml.org/lkml/2006/3/10/224
> > 
> > Later, in commit 638fa202c, Roland McGrath updated mem_write to call
> > check_mem_permission which ensures an identical security policy for
> > /proc/PID/mem as for ptrace().  IOW, the proc interface provides a simpler, more
> > efficient, but otherwise equivalent mechanism for probing a processes memory as
> > available via ptrace.
> > 
> > There is no longer a security hazard and the world can safely use read/write
> > instead of ptrace PEEK/POKE's.  Remove the #ifdef.
> > 
> > Signed-off-by: Stephen Wilson <wilsons@start.ca>
> 
> I haven't found any problem in this patch. But, I really believe we need
> to understand why it was marked "security hazard". Al, I guess you know it,
> right? So, can you please talk us your mention?

I did a bit more digging trying to find why mem_write was marked a security
hazard.

It goes back to 2.4.0-test10pre4.  Unfortunately, the changelog entry is
not at all informative either:

	- disable writing to /proc/xxx/mem. Sure, it works now, but it's
	  still a security risk.

For the interested, some of the history is visible here:

	http://tinyurl.com/4aj4d3v


Personally, I have doubts that there is much to be gleaned from this
"security hazard" comment or any amount of archeology.

The code in question has been maintained for over a decade without use.
However, the implementation looks sane to me from a security POV as it
mirrors the policy for ptrace.  But it would be great to have a few more
eyes audit this change.

-- 
steve

Re: [PATCH] Enable writing to /proc/PID/mem.

[Al Viro]

On Thu, Mar 03, 2011 at 02:38:02PM -0500, Stephen Wilson wrote:
> > I haven't found any problem in this patch. But, I really believe we need
> > to understand why it was marked "security hazard". Al, I guess you know it,
> > right? So, can you please talk us your mention?
> 
> I did a bit more digging trying to find why mem_write was marked a security
> hazard.
> 
> It goes back to 2.4.0-test10pre4.  Unfortunately, the changelog entry is
> not at all informative either:
> 
> 	- disable writing to /proc/xxx/mem. Sure, it works now, but it's
> 	  still a security risk.

Think what happens if the target execs suid-root binary in the middle of your
call.  After you've done your check.  E.g. during copy_from_user().

On the read side we actually recheck permissions after having copied into
buffer and if the check fails we don't copy that buffer into userland.
Not feasible on the write side...

Re: [PATCH] Enable writing to /proc/PID/mem.

[Stephen Wilson]

On Thu, Mar 03, 2011 at 07:46:26PM +0000, Al Viro wrote:
> Think what happens if the target execs suid-root binary in the middle of your
> call.  After you've done your check.  E.g. during copy_from_user().
> 
> On the read side we actually recheck permissions after having copied into
> buffer and if the check fails we don't copy that buffer into userland.
> Not feasible on the write side...

You are right.  Looks like we would need to hold task_lock over both the
permission check and write -- but I do not see a clean/simple way of doing
that today.  Might be worth looking into...

Thanks!

-- 
steve