[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 14 Jul 2011 19:22:33 +0400
From: Solar Designer <solar@...nwall.com>
To: kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] move RLIMIT_NPROC check from set_user() to do_execve_common()
Vasiliy,
On Tue, Jul 12, 2011 at 05:27:23PM +0400, Vasiliy Kulikov wrote:
> + const struct cred *cred = current_cred();
> +
> + /*
> + * We check for RLIMIT_NPROC in execve() instead of set_user() because
> + * too many poorly written programs don't check setuid() return code.
> + * The check in execve() does the same thing for programs doing
> + * setuid()+execve(), but without similar security issues.
> + */
> + if (atomic_read(&cred->user->processes) > rlimit(RLIMIT_NPROC) &&
> + cred->user != INIT_USER) {
> + retval = -EAGAIN;
> + goto out_ret;
> + }
Is cred->user == NULL impossible here? Somehow I had a check for NULL
here in -ow patches (for older kernels), maybe out of paranoia or maybe
for specific reasons (I don't recall).
> +++ b/kernel/sys.c
> @@ -591,12 +591,6 @@ static int set_user(struct cred *new)
> if (!new_user)
> return -EAGAIN;
>
> - if (atomic_read(&new_user->processes) >= rlimit(RLIMIT_NPROC) &&
> - new_user != INIT_USER) {
> - free_uid(new_user);
> - return -EAGAIN;
> - }
So you're moving the check almost literally. However, I think a similar
check on fork() also checked "!capable(CAP_SYS_ADMIN) &&
!capable(CAP_SYS_RESOURCE)", and I had this additional check/bypass
included in -ow patches' execve(). This discrepancy between the two
checks (one allows capable processes to bypass it, the other does not)
is seen in Neil's commit you referenced:
http://lkml.org/lkml/2003/7/13/226
So maybe it was intentional, or maybe it was overlooked. I don't care
about this much, but I thought I'd point it out.
Thanks,
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
