LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Solar Designer <solar@openwall.com>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: CGEL <cgel.zte@gmail.com>,
	peterz@infradead.org, tglx@linutronix.de,
	linux-kernel@vger.kernel.org,
	Ran Xiaokai <ran.xiaokai@zte.com.cn>,
	James Morris <jamorris@linux.microsoft.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>, NeilBrown <neilb@suse.de>
Subject: Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds
Date: Wed, 8 Sep 2021 12:24:00 +0200	[thread overview]
Message-ID: <20210908102400.GA22799@openwall.com> (raw)
In-Reply-To: <20210907213042.GA22626@openwall.com>

Here's a further observation:

On Tue, Sep 07, 2021 at 11:30:42PM +0200, Solar Designer wrote:
> As I understand, the resulting commit:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2863643fb8b92291a7e97ba46e342f1163595fa8
> 
> broke RLIMIT_NPROC support for Apache httpd suexec and likely similar.

The commit above tries to make things consistent by duplicating the
check from copy_process() also in set_user().  However, the check isn't
actually the same because set_user(new) is called _before_
security_task_fix_setuid(new, ...), whereas in the described detour via
fork() its check would be reached already as the new user.  So those
capable() calls just look the same, but are actually very different, and
that's the problem.  My current understanding is the commit actually
increases inconsistency.

The commit message starts with:

"in copy_process(): non root users but with capability CAP_SYS_RESOURCE
or CAP_SYS_ADMIN will clean PF_NPROC_EXCEEDED flag even
rlimit(RLIMIT_NPROC) exceeds. Add the same capability check logic here."

It talks about the obscure case of "non root users but with capability".
However, the capable() calls added by the commit actually also apply to
root, such as in suexec.

> Anyway, now I suggest that 2863643fb8b92291a7e97ba46e342f1163595fa8 be
> reverted, and if there's any reason to make any change (what reason?
> mere consistency or any real issue?) then I suggest that the flag
> resetting on fork() be made conditional.  Something like this:
> 
> 	if (atomic_read(&p->real_cred->user->processes) >=
> 			task_rlimit(p, RLIMIT_NPROC)) {
> 		if (p->real_cred->user != INIT_USER &&
> 		    !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
> 			goto bad_fork_free;
> -	}
> -	current->flags &= ~PF_NPROC_EXCEEDED;
> +	} else
> +		current->flags &= ~PF_NPROC_EXCEEDED;

Alternatively, we could postpone the set_user() calls until we're
running with the new user's capabilities, but that's an invasive change
that's likely to create its own issues.  So my suggestion above holds.

Alexander

  reply	other threads:[~2021-09-08 10:24 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28  7:26 cgel.zte
2021-07-28 11:59 ` Christian Brauner
2021-07-30  8:23   ` CGEL
2021-08-03 10:03     ` CGEL
2021-08-03 14:07       ` Christian Brauner
2021-09-07 21:30         ` Solar Designer
2021-09-08 10:24           ` Solar Designer [this message]
2021-09-13 10:01             ` Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210908102400.GA22799@openwall.com \
    --to=solar@openwall.com \
    --cc=cgel.zte@gmail.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=jamorris@linux.microsoft.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=peterz@infradead.org \
    --cc=ran.xiaokai@zte.com.cn \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).