Subject: Re: [2.6.18 patch] fix mem_write return value (was: Re: bug report:
 mem_write)
In-Reply-To: <20060824220747.GA3197@slug>
To: Frederik Deweerdt <deweerdt@free.fr>
Date: Fri, 25 Aug 2006 04:05:36 +0300 (IDT)
CC: "Eric W. Biederman" <ebiederm@xmission.com>,
       Amnon Shiloh <amnons@cs.huji.ac.il>, linux-kernel@vger.kernel.org,
       akpm@osdl.org, gregkh@suse.de
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII
Message-Id: <E1GGQ8f-00053J-02@cab-20.cs.huji.ac.il>
From: Amnon Shiloh <amnons@cs.huji.ac.il>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2991
Lines: 64

> 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.
-
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/