Messages in this thread | | | Date | Wed, 8 Sep 2021 12:24:00 +0200 | From | Solar Designer <> | Subject | Re: [PATCH] set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds |
| |
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
| |