Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Solar Designer <solar@openwall.com>
Cc:     linux-kernel@vger.kernel.org, Alexey Gladkov <legion@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Shuah Khan <shuah@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Ran Xiaokai <ran.xiaokai@zte.com.cn>,
        Michal Koutn?? <mkoutny@suse.com>, stable@vger.kernel.org
References: <87o83e2mbu.fsf@email.froward.int.ebiederm.org>
        <20220211021324.4116773-3-ebiederm@xmission.com>
        <20220212231701.GA29483@openwall.com>
Date:   Mon, 14 Feb 2022 09:10:49 -0600
In-Reply-To: <20220212231701.GA29483@openwall.com> (Solar Designer's message
        of "Sun, 13 Feb 2022 00:17:01 +0100")
Message-ID: <87ee45wkjq.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 3/8] ucounts: Fix and simplify RLIMIT_NPROC handling
 during setuid()+execve
Precedence: bulk

Solar Designer <solar@openwall.com> writes:

> On Thu, Feb 10, 2022 at 08:13:19PM -0600, Eric W. Biederman wrote:
>> As of commit 2863643fb8b9 ("set_user: add capability check when
>> rlimit(RLIMIT_NPROC) exceeds") setting the flag to see if execve
>> should check RLIMIT_NPROC is buggy, as it tests the capabilites from
>> before the credential change and not aftwards.
>> 
>> As of commit 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of
>> ucounts") examining the rlimit is buggy as cred->ucounts has not yet
>> been properly set in the new credential.
>> 
>> Make the code correct and more robust moving the test to see if
>> execve() needs to test RLIMIT_NPROC into commit_creds, and defer all
>> of the rest of the logic into execve() itself.
>> 
>> As the flag only indicateds that RLIMIT_NPROC should be checked
>> in execve rename it from PF_NPROC_EXCEEDED to PF_NPROC_CHECK.
>> 
>> Cc: stable@vger.kernel.org
>> Link: https://lkml.kernel.org/r/20220207121800.5079-2-mkoutny@suse.com
>> Link: https://lkml.kernel.org/r/20220207121800.5079-3-mkoutny@suse.com
>> Reported-by: Michal Koutn?? <mkoutny@suse.com>
>> Fixes: 2863643fb8b9 ("set_user: add capability check when rlimit(RLIMIT_NPROC) exceeds")
>> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> On one hand, this looks good.
>
> On the other, you asked about the Apache httpd suexec scenario in the
> other thread, and here's what this means for it (per my code review):
>
> In that scenario, we have two execve(): first from httpd to suexec, then
> from suexec to the CGI script.  Previously, the limit check only
> occurred on the setuid() call by suexec, and its effect was deferred
> until execve() of the script.  Now wouldn't it occur on both execve()
> calls, because commit_creds() is also called on execve() (such as in
> case the program is SUID, which suexec actually is)?

Yes.  Moving the check into commit_creds means that the exec after a
suid exec will perform an RLIMIT_NPROC check and could possibly fail.  I
would call that a bug.  Anything happening in execve should be checked
and handled in execve as execve can fail.

It also points out that our permission checks for increasing
RLIMIT_NPROC are highly inconsistent.

One set of permissions in fork().
Another set of permissions in set*id() and delayed until execve.
No permission checks for the uid change in execve.

Every time I look into the previous behavior of RLIMIT_NPROC I seem
to find issues.  Currently I am planning a posting to linux-api
so sorting out what when RLIMIT_NPROC should be enforced and how
RLIMIT_NPROC gets accounted receives review.  I am also planning a
feature branch to deal with the historical goofiness.

I really like how cleanly this patch seems to be.  Unfortunately it is
wrong.

> Since the check is
> kind of against real uid (not the euid=0 that suexec gains), it'd apply
> the limit against httpd pseudo-user's process count.  While it could be
> a reasonable kernel policy to impose this limit in more places, this is
> a change of behavior for Apache httpd, and is not the intended behavior
> there.  However, I think the answer to my question earlier in this
> paragraph is actually a "no", the check wouldn't occur on the execve()
> of suexec, because "new->user != old->user" would be false.  Right?
>
> As an alternative, you could keep setting the (renamed and reused) flag
> in set_user().  That would avoid the (non-)issue I described above - but
> again, your patch is probably fine as-is.
>
> I do see it's logical to have these two lines next to each other:
>
>>  		inc_rlimit_ucounts(new->ucounts, UCOUNT_RLIMIT_NPROC, 1);
>> +		task->flags |= PF_NPROC_CHECK;
>
> Of course, someone would need to actually test this.

That too.

I am increasingly of the opinion that the process accounting should not
be in cred.c at all.  That we just remember the who was charged with the
process when we created it, and then at exec time we can update that
charge, and verify that the new user is solid.  At exit time we can look
up who was charged with the process and decrement the count.

Of course at this point my opinion may change after I implement that.

Eric