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

[Vasiliy Kulikov <segoon@openwall.com>]

Neil,

On Fri, Jul 15, 2011 at 17:06 +1000, NeilBrown wrote:
> How about this then?

AFAIU, with this patch:

1) setuid() doesn't fail in NPROC exceed case.
2) NPROC is forced on execve() after setuid().
3) execve() fails only if NPROC was exceeded during setuid() execution.
4) Other processes of the same user doesn't suffer from "occasional"
execve() failures.

If it is correct, then I like the patch :)  It does RLIMIT_NPROC
enforcement without mixing other execve() calls like -ow patch did.

See minor suggestions about the comments below.


> Signed-off-by: NeilBrown <neilb@suse.de>

Acked-by: Vasiliy Kulikov <segoon@openwall.com>

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

It doesn't lead to privileges leaking as is, it is a threat of buggy
programs not checking return code of syscalls dropping privileges (setuid here).

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

The comment about why all this bustle is needed is better to put here
because the check is logically belongs to set_user().

Specifically, I'd mention that (1) we don't want setuid() to fail while
having enough privileges and (2) RLIMIT_NPROC can be still harmlessly
checked for programs doing setuid()+execve() if the error code is
deferred to the execve stage.

> +		current->flags |= PF_NPROC_EXCEEDED;
>  
>  	free_uid(new->user);
>  	new->user = new_user;

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments