Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common()

From: NeilBrown <neilb@suse.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>,
	linux-kernel@vger.kernel.org, Greg Kroah-Hartman <gregkh@suse.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	"David S. Miller" <davem@davemloft.net>,
	kernel-hardening@lists.openwall.com, Jiri Slaby <jslaby@suse.cz>,
	James Morris <jmorris@namei.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common()
Date: Wed, 13 Jul 2011 09:14:08 +1000	[thread overview]
Message-ID: <20110713091408.0d456352@notabene.brown> (raw)
In-Reply-To: <CA+55aFysXOeBqEvmVbuqL3i0jRFDcE=vDuVhmYL38XQ0+x=deQ@mail.gmail.com>

On Tue, 12 Jul 2011 14:16:10 -0700 Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> On Tue, Jul 12, 2011 at 6:27 AM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> >
> > The NPROC can still be enforced in the common code flow of daemons
> > spawning user processes.  Most of daemons do fork()+setuid()+execve().
> > The check introduced in execve() enforces the same limit as in setuid()
> > and doesn't create similar security issues.
> 
> 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.
> 
> 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.
> 
>                    Linus

I am happy with the patch in general - it adequately addresses the problem
which I fixed by adding the test to set_user which is now being removed.

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 is
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 consequences.

Between the 'setuid(ordinary-user)' and a subsequent 'exit()' after execve()
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_FORKNOEXEC
in current->flags and only fail the execve if both are set.
i.e.
    (current->flags & (PF_SUPERPRIV|PF_FORKNOEXEC)) == (PF_SUPERPRIV|PF_FORKNOEXEC)

That should narrow it down to only failing in the particular case that we are
interested in.

NeilBrown