linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Linus Torvalds <linus@torvalds.org>
Cc: "Linux API" <linux-api@vger.kernel.org>,
	"Etienne Dechamps" <etienne@edechamps.fr>,
	"Alexey Gladkov" <legion@kernel.org>,
	"Kees Cook" <keescook@chromium.org>,
	"Shuah Khan" <shuah@kernel.org>,
	"Christian Brauner" <brauner@kernel.org>,
	"Solar Designer" <solar@openwall.com>,
	"Ran Xiaokai" <ran.xiaokai@zte.com.cn>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK"
	<linux-kselftest@vger.kernel.org>,
	"Linux Containers" <containers@lists.linux-foundation.org>,
	"Michal Koutný" <mkoutny@suse.com>,
	"Security Officers" <security@kernel.org>,
	"Neil Brown" <neilb@cse.unsw.edu.au>, NeilBrown <neilb@suse.de>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"Jann Horn" <jannh@google.com>,
	"Andy Lutomirski" <luto@kernel.org>, "Willy Tarreau" <w@1wt.eu>
Subject: Re: How should rlimits, suid exec, and capabilities interact?
Date: Wed, 23 Feb 2022 19:32:41 -0600	[thread overview]
Message-ID: <87ee3toxqe.fsf@email.froward.int.ebiederm.org> (raw)
In-Reply-To: <CAHk-=wjX3VK8QRMDUWwigCTKdHJt0ESXh0Hy5HNaXf7YkEdCAA@mail.gmail.com> (Linus Torvalds's message of "Wed, 23 Feb 2022 11:50:38 -0800")


Linus Torvalds <linus@torvalds.org> writes:

> Basic rule: it's better to be too lenient than to be too strict.

Thank you.  With that guideline I can explore the space of what is
possible.

Question: Running a suid program today charges the activity of that
program to the user who ran that program, not to the user the program
runs as.  Does anyone see a problem with charging the user the program
runs as?

The reason I want to change which user is charged with a process
(besides it making more sense in my head) is so that
"capable(CAP_SYS_RESOURCE)" can be used instead of the magic incantation
"(cred->user == INIT_USER)".

Today "capable(CAP_SYS_RESOURCE)" with respect to RLIMIT_NPROC is
effectively meaningless for suid programs because the of the mismatch of
charging the real user with the effective users credentials.

An accidental experiment happened in v5.14-rc1 in July when the ucount
rlimit code was merged.  It was only this last week when after Michal
Koutný discovered the discrepancy through code inspection I merged a
bug fix because the code was not preserving the existing behavior as
intended.


This behavior has existed in some form since Linux v1.0 when per user
process limits were added.

The original code in v1.0 was:
> static int find_empty_process(void)
> {
>        int free_task;
>        int i, tasks_free;
>        int this_user_tasks;
> 
> repeat:
>        if ((++last_pid) & 0xffff8000)
>                last_pid=1;
>        this_user_tasks = 0;
>        tasks_free = 0;
>        free_task = -EAGAIN;
>        i = NR_TASKS;
>        while (--i > 0) {
>                if (!task[i]) {
>                        free_task = i;
>                        tasks_free++;
>                        continue;
>                }
>                if (task[i]->uid == current->uid)
>                        this_user_tasks++;
>                if (task[i]->pid == last_pid || task[i]->pgrp == last_pid ||
>                    task[i]->session == last_pid)
>                        goto repeat;
>        }
>        if (tasks_free <= MIN_TASKS_LEFT_FOR_ROOT ||
>            this_user_tasks > MAX_TASKS_PER_USER)
>                if (current->uid)
>                        return -EAGAIN;
>        return free_task;
> }

Having tracked the use of real uid in limits back this far my guess
is that it was an accident of the implementation and real uid vs
effective uid had not be considered.

Does anyone know if choosing the real uid vs the effective uid for
accounting a users processes was a deliberate decision anywhere in the
history of Linux?



Linus you were talking about making it possible to login as I think a
non-root user to be able to use sudo and kill a fork bomb.

The counter case is apache having a dedicated user for running
cgi-scripts and using RLIMIT_NPROC to limit how many of those processes
can exist.  Unless I am misunderstanding something that looks exactly
like your login as non-root so you can run sudo to kill a fork-bomb.

A comment from an in-process cleanup patch explains this as best I can:
         /*
         * In general rlimits are only enforced when a new resource
         * is acquired.  That would be during fork for RLIMIT_NPROC.
         * That is insufficient for RLIMIT_NPROC as many attributes of
         * a new process must be set between fork and exec.
         *
         * A case where this matter is when apache runs forks a process
         * and calls setuid to run cgi-scripts as a different user.
         * Generating those processes through a code sequence like:
         *
         *      fork()
 	 *	setrlimit(RLIMIT_NPROC, ...)         
         *      execve()  -- suid wrapper
         *      setuid()
         *      execve()  -- cgi script
         *
         * The cgi-scripts are unlikely to fork on their own so unless
         * RLIMIT_NPROC is checked after the user change and before
         * the cgi-script starts, RLIMIT_NPROC simply will not be enforced
         * for the cgi-scripts.
         *
         * So the code tracks if between fork and exec if an operation
         * occurs that could cause the RLIMIT_NPROC check to fail.  If
         * such an operation has happened re-check RLIMIT_NPROC.
         */


Answered-Question: I was trying to ask if anyone knows of a reason why
we can't just sanitize the rlimits of the process during suid exec?
Linus your guideline would appear to allow that behavior.  Unfortunately
that looks like it would break current usage of apache suexec.

Eric

      parent reply	other threads:[~2022-02-24  1:33 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-07 12:17 [RFC PATCH 0/6] RLIMIT_NPROC in ucounts fixups Michal Koutný
2022-02-07 12:17 ` [RFC PATCH 1/6] set_user: Perform RLIMIT_NPROC capability check against new user credentials Michal Koutný
2022-02-10  1:14   ` Solar Designer
2022-02-10  1:57     ` Eric W. Biederman
2022-02-11 20:32     ` Eric W. Biederman
2022-02-12 22:14       ` Solar Designer
2022-02-15 11:55     ` Michal Koutný
2022-02-07 12:17 ` [RFC PATCH 2/6] set*uid: Check RLIMIT_PROC against new credentials Michal Koutný
2022-02-07 12:17 ` [RFC PATCH 3/6] cred: Count tasks by their real uid into RLIMIT_NPROC Michal Koutný
2022-02-07 12:17 ` [RFC PATCH 4/6] ucounts: Allow root to override RLIMIT_NPROC Michal Koutný
2022-02-10  0:21   ` Eric W. Biederman
2022-02-07 12:17 ` [RFC PATCH 5/6] selftests: Challenge RLIMIT_NPROC in user namespaces Michal Koutný
2022-02-10  1:22   ` Shuah Khan
2022-02-15  9:45     ` Michal Koutný
2022-02-07 12:18 ` [RFC PATCH 6/6] selftests: Test RLIMIT_NPROC in clone-created " Michal Koutný
2022-02-10  1:25   ` Shuah Khan
2022-02-15  9:34     ` Michal Koutný
2022-02-08 13:54 ` [RFC PATCH 0/6] RLIMIT_NPROC in ucounts fixups Eric W. Biederman
2022-02-11  2:01 ` [PATCH 0/8] ucounts: RLIMIT_NPROC fixes Eric W. Biederman
2022-02-11  2:13   ` [PATCH 1/8] ucounts: Fix RLIMIT_NPROC regression Eric W. Biederman
2022-02-14 18:37     ` Michal Koutný
2022-02-16 15:22       ` Eric W. Biederman
2022-02-11  2:13   ` [PATCH 2/8] ucounts: Fix set_cred_ucounts Eric W. Biederman
2022-02-15 11:10     ` Michal Koutný
2022-02-11  2:13   ` [PATCH 3/8] ucounts: Fix and simplify RLIMIT_NPROC handling during setuid()+execve Eric W. Biederman
2022-02-12 23:17     ` Solar Designer
2022-02-14 15:10       ` Eric W. Biederman
2022-02-14 17:43         ` Eric W. Biederman
2022-02-15 10:25         ` Michal Koutný
2022-02-16 15:35           ` Eric W. Biederman
2022-02-11  2:13   ` [PATCH 4/8] ucounts: Only except the root user in init_user_ns from RLIMIT_NPROC Eric W. Biederman
2022-02-15 10:54     ` Michal Koutný
2022-02-16 15:41       ` Eric W. Biederman
2022-02-11  2:13   ` [PATCH 5/8] ucounts: Handle wrapping in is_ucounts_overlimit Eric W. Biederman
2022-02-12 22:36     ` Solar Designer
2022-02-14 15:23       ` Eric W. Biederman
2022-02-14 15:23         ` Eric W. Biederman
2022-02-15 11:25         ` Michal Koutný
2022-02-14 17:16       ` David Laight
2022-02-11  2:13   ` [PATCH 6/8] ucounts: Handle inc_rlimit_ucounts wrapping in fork Eric W. Biederman
2022-02-11 11:34     ` Alexey Gladkov
2022-02-11 17:50       ` Eric W. Biederman
2022-02-11 18:32         ` Shuah Khan
2022-02-11 18:40         ` Alexey Gladkov
2022-02-11 19:56           ` Eric W. Biederman
2022-02-11  2:13   ` [PATCH 7/8] rlimit: For RLIMIT_NPROC test the child not the parent for capabilites Eric W. Biederman
2022-02-11  2:13   ` [PATCH 8/8] ucounts: Use the same code to enforce RLIMIT_NPROC in fork and exec Eric W. Biederman
2022-02-11 18:22   ` [PATCH 0/8] ucounts: RLIMIT_NPROC fixes Shuah Khan
2022-02-11 19:23     ` Eric W. Biederman
2022-02-15 11:37     ` Michal Koutný
2022-02-16 15:56   ` [PATCH v2 0/5] " Eric W. Biederman
2022-02-16 15:58     ` [PATCH v2 1/5] rlimit: Fix RLIMIT_NPROC enforcement failure caused by capability calls in set_user Eric W. Biederman
2022-02-16 17:42       ` Solar Designer
2022-02-16 15:58     ` [PATCH v2 2/5] ucounts: Enforce RLIMIT_NPROC not RLIMIT_NPROC+1 Eric W. Biederman
2022-02-16 15:58     ` [PATCH v2 3/5] ucounts: Base set_cred_ucounts changes on the real user Eric W. Biederman
2022-02-16 15:58     ` [PATCH v2 4/5] ucounts: Move RLIMIT_NPROC handling after set_user Eric W. Biederman
2022-02-16 15:58     ` [PATCH v2 5/5] ucounts: Handle wrapping in is_ucounts_overlimit Eric W. Biederman
2022-02-16 17:28       ` Shuah Khan
2022-02-18 15:34     ` [GIT PULL] ucounts: RLIMIT_NPROC fixes for v5.17 Eric W. Biederman
2022-02-20 19:05       ` pr-tracker-bot
2022-03-03  0:12       ` [GIT PULL] ucounts: Regression fix " Eric W. Biederman
2022-03-03  0:30         ` pr-tracker-bot
2022-02-12 15:32 ` [RFC PATCH 0/6] RLIMIT_NPROC in ucounts fixups Etienne Dechamps
2022-02-15 10:11   ` Michal Koutný
2022-02-23  0:57     ` Eric W. Biederman
2022-02-23 18:00       ` How should rlimits, suid exec, and capabilities interact? Eric W. Biederman
2022-02-23 19:44         ` Andy Lutomirski
2022-02-23 21:28           ` Willy Tarreau
2022-02-23 19:50         ` Linus Torvalds
2022-02-24  1:24           ` Eric W. Biederman
2022-02-24  1:41             ` Linus Torvalds
2022-02-24  2:12               ` Eric W. Biederman
2022-02-24 15:41                 ` [PATCH] ucounts: Fix systemd LimigtNPROC with private users regression Eric W. Biederman
2022-02-24 16:28                   ` Kees Cook
2022-02-24 18:53                     ` Michal Koutný
2022-02-25  0:29                     ` Eric W. Biederman
2022-02-24  3:00               ` How should rlimits, suid exec, and capabilities interact? David Laight
2022-02-24  1:32           ` Eric W. Biederman [this message]

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=87ee3toxqe.fsf@email.froward.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=brauner@kernel.org \
    --cc=containers@lists.linux-foundation.org \
    --cc=etienne@edechamps.fr \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=legion@kernel.org \
    --cc=linus@torvalds.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mkoutny@suse.com \
    --cc=neilb@cse.unsw.edu.au \
    --cc=neilb@suse.de \
    --cc=ran.xiaokai@zte.com.cn \
    --cc=security@kernel.org \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=solar@openwall.com \
    --cc=w@1wt.eu \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).