From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965053Ab1GOHHP (ORCPT ); Fri, 15 Jul 2011 03:07:15 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41677 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964786Ab1GOHHK convert rfc822-to-8bit (ORCPT ); Fri, 15 Jul 2011 03:07:10 -0400 Date: Fri, 15 Jul 2011 17:06:50 +1000 From: NeilBrown To: Vasiliy Kulikov Cc: kernel-hardening@lists.openwall.com, Solar Designer , James Morris , Linus Torvalds , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Andrew Morton , "David S. Miller" , Jiri Slaby , Alexander Viro , linux-fsdevel@vger.kernel.org, KOSAKI Motohiro , Eric Paris , Stephen Smalley , Willy Tarreau , Sebastian Krahmer Subject: Re: [kernel-hardening] Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common() Message-ID: <20110715170650.585f1dad@notabene.brown> In-Reply-To: <20110715063113.GA3166@albatros> References: <20110712132723.GA3193@albatros> <20110713091408.0d456352@notabene.brown> <20110713063142.GA19976@openwall.com> <20110713170657.59dae548@notabene.brown> <20110714112751.1bfd998f@notabene.brown> <20110714150602.GA30019@openwall.com> <20110715133013.4fa38d19@notabene.brown> <20110715063113.GA3166@albatros> X-Mailer: Claws Mail 3.7.9 (GTK+ 2.22.1; x86_64-unknown-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 15 Jul 2011 10:31:13 +0400 Vasiliy Kulikov wrote: > Neil, > > On Fri, Jul 15, 2011 at 13:30 +1000, NeilBrown wrote: > > I'm still bothers that the proposed patch can cause exec to fail for an > > separate 'innocent' process. > > It also seems to 'hide' the problem - just shuffling code around. > > The comment in do_execve_common helps. A similar comment in set_user > > wouldn't hurt. > > > > But what do you think of this. It sure that only the process which ignored > > the return value from setuid is inconvenienced. > > I don't like it. You're mixing the main problem and an RLIMIT check > enforcement. The main goal is denying setuid() to fail unless there is not > enough privileges, RLIMIT in execve() is just an *attempt* to still count > NPROC in *some* widespread cases. But you're trying to fix setuid() > where RLIMIT accounting is simple :\ > > Your patch doesn't address the core issue in this situation: > > setuid(); /* it fails because of RLIMIT */ > do_some_fs(); > execve(); > > do_some_fs() should be called ONLY if root is dropped. In your scheme > the process may interact with FS as root while thinking it is nonroot, > which almost always leads to privilege escalation. > > Thanks, > How about this then? >>From 4a1411d10c90510a4eedf94bfd6b2578425271ba Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Fri, 15 Jul 2011 13:20:09 +1000 Subject: [PATCH] Protect execve from being used after 'setuid' exceeds RLIMIT_NPROC Many programs to not check setuid for failure so it is not safe for it to fail. So impose the RLIMIT_NPROC limit by noting the excess in set_user with a process flag, and failing exec() if the flag is set. The patch http://lkml.org/lkml/2003/7/13/226 introduced a RLIMIT_NPROC check in set_user() to check for NPROC exceeding via setuid() and similar functions. Before the check there was a possibility to greatly exceed the allowed number of processes by an unprivileged user if the program relied on rlimit only. But the check created new security threat: many poorly written programs simply don't check setuid() return code and believe it cannot fail if executed with root privileges. So, when the check fails we set a process flag which causes execve to fail. Following examples of vulnerabilities due to processes not checking error from setuid provided by Solar Designer : Here are some examples for 2011-2010: "... missing setuid() retval check in opielogin which leads to easy root compromise": http://www.openwall.com/lists/oss-security/2011/06/22/6 "The /usr/lib/libgnomesu/gnomesu-pam-backend suid binary which belongs to the libgnomesu package is not checking setuid() return values. As a result, two cooperating users, or users with access to guest, cgi or web accounts can run arbitrary commands as root very easily." http://www.openwall.com/lists/oss-security/2011/05/30/2 pam_xauth (exploitable if pam_limits is also used): http://www.openwall.com/lists/oss-security/2010/08/16/2 A collection of examples from 2006: http://lists.openwall.net/linux-kernel/2006/08/20/156 Signed-off-by: NeilBrown diff --git a/fs/exec.c b/fs/exec.c index 6075a1e..dfe9c61 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1434,6 +1434,17 @@ static int do_execve_common(const char *filename, bool clear_in_exec; int retval; + if (current->flags & PF_NPROC_EXCEEDED) { + /* setuid noticed that RLIMIT_NPROC was + * exceeded, but didn't fail as that easily + * leads to privileges leaking. So fail + * here instead - we still limit the number of + * processes running the user's code. + */ + retval = -EAGAIN; + goto out_ret; + } + retval = unshare_files(&displaced); if (retval) goto out_ret; diff --git a/include/linux/sched.h b/include/linux/sched.h index 496770a..f024c63 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1759,6 +1759,7 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t * #define PF_DUMPCORE 0x00000200 /* dumped core */ #define PF_SIGNALED 0x00000400 /* killed by a signal */ #define PF_MEMALLOC 0x00000800 /* Allocating memory */ +#define PF_NPROC_EXCEEDED 0x00001000 /* set_user noticed that RLIMIT_NPROC was exceeded */ #define PF_USED_MATH 0x00002000 /* if unset the fpu must be initialized before use */ #define PF_FREEZING 0x00004000 /* freeze in progress. do not account to load */ #define PF_NOFREEZE 0x00008000 /* this thread should not be frozen */ diff --git a/kernel/cred.c b/kernel/cred.c index 174fa84..8ef31f5 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -508,10 +508,8 @@ int commit_creds(struct cred *new) key_fsgid_changed(task); /* do it - * - What if a process setreuid()'s and this brings the - * new uid over his NPROC rlimit? We can check this now - * cheaply with the new uid cache, so if it matters - * we should be checking for it. -DaveM + * RLIMIT_NPROC limits on user->processes have already been checked + * in set_user(). */ alter_cred_subscribers(new, 2); if (new->user != old->user) diff --git a/kernel/sys.c b/kernel/sys.c index e4128b2..dd1fb9d 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -592,10 +592,9 @@ static int set_user(struct cred *new) return -EAGAIN; if (atomic_read(&new_user->processes) >= rlimit(RLIMIT_NPROC) && - new_user != INIT_USER) { - free_uid(new_user); - return -EAGAIN; - } + new_user != INIT_USER) + /* Cause any subsequent 'exec' to fail */ + current->flags |= PF_NPROC_EXCEEDED; free_uid(new->user); new->user = new_user;