From mboxrd@z Thu Jan 1 00:00:00 1970 From: NeilBrown Subject: Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common() Date: Wed, 13 Jul 2011 09:14:08 +1000 Message-ID: <20110713091408.0d456352@notabene.brown> References: <20110612130953.GA3709@albatros> <20110706173631.GA5431@albatros> <20110706185932.GB3299@albatros> <20110707075610.GA3411@albatros> <20110707081930.GA4393@albatros> <20110712132723.GA3193@albatros> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Vasiliy Kulikov , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Andrew Morton , "David S. Miller" , kernel-hardening@lists.openwall.com, Jiri Slaby , James Morris , Alexander Viro , linux-fsdevel@vger.kernel.org To: Linus Torvalds Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Tue, 12 Jul 2011 14:16:10 -0700 Linus Torvalds wrote: > On Tue, Jul 12, 2011 at 6:27 AM, Vasiliy Kulikov wrote: > > > > The NPROC can still be enforced in the common code flow of daemons > > spawning user processes. =A0Most of daemons do fork()+setuid()+exec= ve(). > > The check introduced in execve() enforces the same limit as in setu= id() > > and doesn't create similar security issues. >=20 > Ok, this looks fine by me. I'd like to get some kind of comment from > the selinux etc people (James?) but I'd certainly be willing to take > this. >=20 > Failing when executing a suid application rather than when a suid > application releases its heightened credentials seems to be the > fundamentally saner approach. IOW, failing to raise privileges rather > than failing to lower them. >=20 > Linus I am happy with the patch in general - it adequately addresses the prob= lem which I fixed by adding the test to set_user which is now being removed= =2E However I don't think your characterisation is quite correct Linus. There is no setuid application, and there is no raising of privileges. The contrast is really "failing when trying to use reduced privileges i= s safer than failing to reduce privileges - if the reduced privileges are= not available". Note that there is room for a race that could have unintended consequen= ces. Between the 'setuid(ordinary-user)' and a subsequent 'exit()' after exe= cve() has failed, any other process owned by the same user (and we know where= are quite a few) would fail an execve() where it really should not. I think it would be safer to add a test for PF_SUPERPRIV and PF_FORKNOE= XEC in current->flags and only fail the execve if both are set. i.e. (current->flags & (PF_SUPERPRIV|PF_FORKNOEXEC)) =3D=3D (PF_SUPERPRI= V|PF_FORKNOEXEC) That should narrow it down to only failing in the particular case that = we are interested in. NeilBrown