linux-kernel - Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries irrespective of invoking users

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20211222062903.GA1720@1wt.eu>
Date:   Wed, 22 Dec 2021 07:29:03 +0100
From:   Willy Tarreau <w@....eu>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Waiman Long <longman@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Kees Cook <keescook@...omium.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Laurent Vivier <laurent@...ier.eu>,
        YunQiang Su <ysu@...ecomp.com>, Helge Deller <deller@....de>
Subject: Re: [PATCH] exec: Make suid_dumpable apply to SUID/SGID binaries
 irrespective of invoking users

On Tue, Dec 21, 2021 at 05:35:29PM -0600, Eric W. Biederman wrote:
> > 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.

OK thanks.

> 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.

Yes that's the idea. I think that fundamentally we ought to mark
that a chain of processes are potentially unsafe for dumps until the
application has shown that it could regain control of the code paths,
and hence is expected to deal properly with errors that might appear
so as not to dump a core anywhere with random permissions.

> 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.

You would like to postpone this ? It's not very clear to me how to do
that nor if it could reliably address this shortcoming.

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

Yep I think so. Also code that is designed to run under setuid (like sudo)
is usally well tested, and quite portable thanks to various OS-specific
tweaks that we definitely don't want to break.

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

There are probably other ones, but ideally we ought to avoid stuff
that could happen early in the dynamic linker.

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

It was a supposition emitted by Linus, which deserved being checked
at least given that it's part of the usual sequence when starting a
deamon.

> 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.

OK, so it will not help them. For them we could use setuid()/setreuid()
and setresuid() as good indicators that the application has taken control
of its fate. Sadly we cannot do that in set_user() because this one is
not called when the uid doesn't change.

> 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.

Good point, I hadn't thought about that, but I agree with you.

Willy