From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Willy Tarreau <w@1wt.eu>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Waiman Long <longman@redhat.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Kees Cook <keescook@chromium.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Laurent Vivier <laurent@vivier.eu>,
        YunQiang Su <ysu@wavecomp.com>, Helge Deller <deller@gmx.de>
References: <20211221021744.864115-1-longman@redhat.com>
        <87lf0e7y0k.fsf@email.froward.int.ebiederm.org>
        <4f67dc4c-7038-7dde-cad9-4feeaa6bc71b@redhat.com>
        <87czlp7tdu.fsf@email.froward.int.ebiederm.org>
        <e78085e4-74cd-52e1-bc0e-4709fac4458a@redhat.com>
        <CAHk-=wg+qpNvqcROndhRidOE1i7bQm93xM=jmre98-X4qkVkMw@mail.gmail.com>
        <7f0f8e71-cf62-4c0b-5f13-a41919c6cd9b@redhat.com>
        <20211221205635.GB30289@1wt.eu> <20211221221336.GC30289@1wt.eu>
Date:   Tue, 21 Dec 2021 17:35:29 -0600
In-Reply-To: <20211221221336.GC30289@1wt.eu> (Willy Tarreau's message of "Tue,
        21 Dec 2021 23:13:36 +0100")
Message-ID: <87o8594jlq.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries irrespective of invoking users
Precedence: bulk

Willy Tarreau <w@1wt.eu> writes:

> On Tue, Dec 21, 2021 at 09:56:35PM +0100, Willy Tarreau wrote:
>> > As it is all done within the kernel, there is no need to
>> > change any userspace code. We may need to add a flag bit in the task
>> > structure to indicate using the suid_dumpable setting so that it can be
>> > inherited across fork/exec.
>> 
>> Depending on what we change there can be some subtly visible changes.
>> In one of my servers I explicitly re-enable dumpable before setsid()
>> when a core dump is desired for debugging. But other deamons could do
>> the exact opposite. If setsid() systematically restores suid_dumpable,
>> a process that explicitly disables it before calling setsid() would
>> see it come back. But if we have a special "suid_in_progress" flag
>> to mask suid_dumpable and that's reset by setsid() and possibly
>> prctl(PR_SET_DUMPABLE) then I think it could even cover that unlikely
>> case.
>
> Would there be any interest in pursuing attempts like the untested patch
> below ? The intent is to set a new MMF_NOT_DUMPABLE on exec on setuid or
> setgid bit, but clear it on setrlimit(RLIMIT_CORE), prctl(SET_DUMPABLE),
> and setsid(). This flag makes get_dumpable() return SUID_DUMP_DISABLED
> when set. I think that in the spirit it could maintain the info that a
> suidexec happened and was not reset, without losing any tuning made by
> the application. I never feel at ease touching all this and I certainly
> did some mistakes but for now it's mostly to have a base to discuss
> around, so do not hesitate to suggest or criticize.


Yes.  This looks like a good place to start the conversation.

We need to do something like you are doing to separate dumpability
changes due to privilege gains during exec and dumpability changes due
to privilege shuffling with setresuid.

As long as we only impact processes descending from a binary that has
gained privileges during exec (like this patch) I think we have a lot
of latitude in how we make this happen.  Basically we only need to
test su and sudo and verify that whatever we do works reasonably
well for them.

On the one hand I believe of gaining privileges during exec while
letting the caller control some aspect of our environment is a dangerous
design flaw and I would love to remove gaining privileges during exec
entirely.

On the other hand we need to introduces as few regressions as possible
and make gaining privileges during exec as safe as possible.


I do agree that RLIMIT_CORE and prctl(SET_DUMPABLE) are good places
to clear the flag.


I don't know if setsid is the proper key to re-enabling dumpability.

I ran a quick test and simply doing "su" and then running a shell
as root does not change the session, nor does "su -" (which creates
a login shell).  Also "sudo -s" does not create a new session.

So session creation does not happen naturally.

Still setsid is part of the standard formula for starting a daemon,
so I don't think system services that run as daemons will be affected.


I don't think anything we do matters for systemd.  As I understand
it "systemctl start ..." causes pid 1 to fork and exec services,
which will ensure the started processes are not descendants of
the binary the gained privileges during exec.

Eric