linux-kernel - Re: [PATCH] ucounts: Fix systemd LimigtNPROC with private users regression

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202202240826.E31BADF@keescook>
Date:   Thu, 24 Feb 2022 08:28:41 -0800
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     linux-kernel@...r.kernel.org,
        Linux API <linux-api@...r.kernel.org>,
        Etienne Dechamps <etienne@...champs.fr>,
        Alexey Gladkov <legion@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        Solar Designer <solar@...nwall.com>,
        Ran Xiaokai <ran.xiaokai@....com.cn>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        Michal Koutný <mkoutny@...e.com>,
        Security Officers <security@...nel.org>,
        Neil Brown <neilb@....unsw.edu.au>, NeilBrown <neilb@...e.de>,
        "Serge E. Hallyn" <serge@...lyn.com>, Jann Horn <jannh@...gle.com>,
        Andy Lutomirski <luto@...nel.org>, Willy Tarreau <w@....eu>,
        Linus Torvalds <linus@...valds.org>
Subject: Re: [PATCH] ucounts: Fix systemd LimigtNPROC with private users
 regression

typo: Subject's LimigtNPROC -> LimitNPROC

On Thu, Feb 24, 2022 at 09:41:44AM -0600, Eric W. Biederman wrote:
> 
> Long story short recursively enforcing RLIMIT_NPROC when it is not
> enforced on the process that creates a new user namespace, causes
> currently working code to fail.  There is no reason to enforce
> RLIMIT_NPROC recursively when we don't enforce it normally so update
> the code to detect this case.
> 
> I would like to simply use capable(CAP_SYS_RESOURCE) to detect when
> RLIMIT_NPROC is not enforced upon the caller.  Unfortunately because
> RLIMIT_NPROC is charged and checked for enforcement based upon the
> real uid, using capable() wich is euid based is inconsistent with reality.

typo: wich -> which

> Come as close as possible to testing for capable(CAP_SYS_RESOURCE) by
> testing for when the real uid would match the conditions when
> CAP_SYS_RESOURCE would be present if the real uid was the effective
> uid.
> 
> Reported-by: Etienne Dechamps <etienne@...champs.fr>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215596
> Link: https://lkml.kernel.org/r/e9589141-cfeb-90cd-2d0e-83a62787239a@edechamps.fr
> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
> 
> The previous conversation has given me enough clarity that I can see
> which tests I am comfortable with use for this pending regression fix.
> 
> I have tested this and it works for me.  Does anyone have any concerns
> with this change?

I'd really love some kind of selftest that exercises the edge cases; do
you have your tests in some form that could be converted?

But otherwise, yes, this looks like the best option here.

Reviewed-by: Kees Cook <keescook@...omium.org>

> 
>  kernel/user_namespace.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 6b2e3ca7ee99..5481ba44a8d6 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -58,6 +58,18 @@ static void set_cred_user_ns(struct cred *cred, struct user_namespace *user_ns)
>  	cred->user_ns = user_ns;
>  }
>  
> +static unsigned long enforced_nproc_rlimit(void)
> +{
> +	unsigned long limit = RLIM_INFINITY;
> +
> +	/* Is RLIMIT_NPROC currently enforced? */
> +	if (!uid_eq(current_uid(), GLOBAL_ROOT_UID) ||
> +	    (current_user_ns() != &init_user_ns))
> +		limit = rlimit(RLIMIT_NPROC);
> +
> +	return limit;
> +}
> +
>  /*
>   * Create a new user namespace, deriving the creator from the user in the
>   * passed credentials, and replacing that user with the new root user for the
> @@ -122,7 +134,7 @@ int create_user_ns(struct cred *new)
>  	for (i = 0; i < MAX_PER_NAMESPACE_UCOUNTS; i++) {
>  		ns->ucount_max[i] = INT_MAX;
>  	}
> -	set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC));
> +	set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_NPROC, enforced_nproc_rlimit());
>  	set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MSGQUEUE, rlimit(RLIMIT_MSGQUEUE));
>  	set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_SIGPENDING, rlimit(RLIMIT_SIGPENDING));
>  	set_rlimit_ucount_max(ns, UCOUNT_RLIMIT_MEMLOCK, rlimit(RLIMIT_MEMLOCK));
> -- 
> 2.29.2
> 

-- 
Kees Cook